[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: mountlist: support windows

From: Bruno Haible
Subject: Re: mountlist: support windows
Date: Thu, 11 Oct 2018 03:28:11 +0200
User-agent: KMail/5.1.3 (Linux/4.4.0-137-generic; KDE/5.18.0; x86_64; ; )

Hi Assaf,

> Attached 2 patches:
>    mountlist tests: add mountlist-tests module
>    mountlist: add support for windows DOS-style drives

Two great contributions!!

Comments about 0001-mountlist-tests-add-mountlist-tests-module.patch:

It's your first gnulib test. I appreciate it!

In the include sequence
 #include <config.h>
 #include <stdio.h>
 #include <mountlist.h>
it's better to put <mountlist.h> directly after <config.h>. This
adds a verification that the header file is self-contained.
(Otherwise, if the header file accidentally uses e.g. the FILE type
without including <stdio.h>, you wouldn't notice it.)

Common indentation is by 2 spaces, not 3 spaces.

In the module description, you can drop the dependency on 'mountlist',
because this dependency is already implicit. This is documented in
https://www.gnu.org/software/gnulib/manual/html_node/Module-description.html .

These comments
+       Call read_file_system_list, print the list, and free the entries.
+       Typical output example:
+         $ ./gnulib-tool -create-testdir -dir mnt-list mountlist-tests
+         $ cd mnt-list
+         $ ./configure && make
+         $ ./gltests/test-mountlist
+         devname:  proc
+         mountdir: /proc
+         mntdoor:  /
+         type:     proc
+         dev:      4
+         dummy:    1
+         remote:   0
+         type_alc: 1
+         devname:  /dev/sda1
+         mountdir: /
+         mntdoor:  /
+         type:     ext4
+         dev:      2049
+         dummy:    0
+         remote:   0
+         type_alc: 1
+         [...]
don't belong in the ChangeLog entry or commit log. They belong
in the source code (tests/test-mountlist.c) instead. The ChangeLog
should only tell what has changed. Even the "why" of a change is
often better stored in the source code than in the ChangeLog.

Please show the gnulib-tool options in their documentated form, with
two minus signs in each option: ./gnulib-tool --create-testdir --dir=...

Please consider using ASSERT in order to verify assertions about the
values returned in the mount_entry struct. For example, if you expect
mnt_ent->me_devname to be non-NULL, write
  ASSERT (mnt_ent->me_devname != NULL);
It's more reliable this way, than to expect that printf will crash when
you pass it a NULL argument. Some printf implementations do, some don't.
The ASSERT macro is defined in "macros.h". Add tests/macros.h in the
module description.

Comments about 0002-mountlist-add-support-for-windows-DOS-style-drives.patch:

Here too, text that explains the functioning of code or what result
to expect from a test belongs in the source code, not in the ChangeLog

"Win32 API" is an old name; Microsoft calls it the "Windows API" for
several years already.

It's better to #include <windows.h> just once (lines 139 and 203).

The code computes 'A'+i and stores it in a string. Probably the loop
should go over 0..25, not 0..31 ?

The first argument to GetVolumeInformation must be NUL-terminated.
It is confusing if you construct the string through snprintf. I would
  char RootPathName[3+1];
  sprintf (RootPathName, "%c:\\", (char)('A'+i));
This is clearer.

Why do you write '\x0' where most C programmers write '\0' ?

In ls-mntd-fs.m4 you don't need the 'return 0;' to make an ANSI C
compliant main() function - AC_LANG_PROGRAM takes care of it already.

In ls-mntd-fs.m4 please try to use an indentation that reflects the
logical nesting level:
+                    [[ DWORD dw = GetLogicalDrives(); return 0; ]])],
+                    [fu_cv_getlogicaldrives=yes],
The second line should have less indentation that the first line.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]