[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
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