Re: mountlist: support windows

[Bruno Haible]

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
entry.

"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
write
  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.


Bruno