[Libguestfs] Remaining syntax-check errors in libguestfs

[Jim Meyering]

Richard W.M. Jones wrote:
...
cont'd

>> prohibit_strcmp
>> examples/mount_local.c:150:      if (p && strcmp (p+1, "bash") == 0) {
>> examples/virt-dhcp-address.c:129:  if (strcmp (guest_type, "linux") == 0) {
>> examples/virt-dhcp-address.c:134:    if (strcmp (guest_distro, "fedora") == 0 ||
>> examples/virt-dhcp-address.c:135:        strcmp (guest_distro, "rhel") == 0 ||
>> examples/virt-dhcp-address.c:136: strcmp (guest_distro,
> "redhat-based") == 0) {
>> examples/virt-dhcp-address.c:139: else if (strcmp (guest_distro,
> "debian") == 0 ||
>> examples/virt-dhcp-address.c:140: strcmp (guest_distro, "ubuntu") ==
> 0) {
>> examples/virt-dhcp-address.c:151: else if (strcmp (guest_type,
> "windows") == 0) {
>> test-tool/test-tool.c:43://#define STRNEQ(a,b) (strcmp((a),(b)) != 0)
>> maint.mk: maint.mk: replace strcmp calls above with STREQ/STRNEQ
>> make: *** [sc_prohibit_strcmp] Error 1
>
> We want to use strcmp in examples.  They don't follow the ordinary
> code conventions used in the rest of libguestfs.

I would exempt ^examples/ from this check.

> There's something wrong with check matching test-tool.c.

Thank you.  That is another regexp that can be improved:
[the ":" was trying to anchor that "#" to beginning of line
in the grep-style "FILE:...text" output that it's filtering,
but that is wrong for two reasons: your example is one, but the
other is that the "#" need not be at the beginning of the line.]

diff --git a/top/maint.mk b/top/maint.mk
index 4627bc5..f77d0c1 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -330,7 +330,7 @@ sc_prohibit_atoi_atof:
 sp_ = strcmp *\(.+\)
 sc_prohibit_strcmp:
 	@prohibit='! *strcmp *\(|\<$(sp_) *[!=]=|[!=]= *$(sp_)'		\
-	exclude=':# *define STRN?EQ\('					\
+	exclude='# *define STRN?EQ\('					\
 	halt='replace strcmp calls above with STREQ/STRNEQ'		\
 	  $(_sc_search_regexp)

>> prohibit_strcmp_and_strncmp
>> examples/mount_local.c:150:      if (p && strcmp (p+1, "bash") == 0) {
...
>> maint.mk: use STREQ(LEN) in place of the above uses of strcmp(strncmp)
>> make: *** [sc_prohibit_strcmp_and_strncmp] Error 1
>
> Examples should be excluded.

You got that right ;-)

>> prohibit_strncpy
>> src/launch-appliance.c:178: strncpy (addr.sun_path, guestfsd_sock,
> UNIX_PATH_MAX);
>> src/launch-libvirt.c:199: strncpy (addr.sun_path, guestfsd_sock,
> UNIX_PATH_MAX);
>> src/launch-libvirt.c:223:  strncpy (addr.sun_path, console_sock, UNIX_PATH_MAX);
>> src/launch-unix.c:63:  strncpy (addr.sun_path, sockpath, UNIX_PATH_MAX);
>> maint.mk: do not use strncpy, period
>> make: *** [sc_prohibit_strncpy] Error 1
>
> I think use of strncpy is justified here.

I agree.  I would exempt those two files.

>> prohibit_test_minus_ao
>> appliance/init:26:if [ ! -L /etc/init.d/udev -a -x /etc/init.d/udev ]; then
>> configure.ac:1276: [test "x$GOBJECT_LIBS" != "x" -a "x$GIO_LIBS" !=
> "x"])
>> edit/test-virt-edit.sh:32:if [ "$(../cat/virt-cat -a test.qcow2
> /etc/test3)" != "a
>> edit/test-virt-edit.sh:47: if [ "$(../cat/virt-cat -a test.qcow2
> /etc/test3)" != "1
>> edit/test-virt-edit.sh:61:if [ "$(../fish/guestfish -i -a test.qcow2
> --ro lstat /etc/test3 | grep -E '^(mode|uid|gid):' | sort)" != "gid:
> 11
>> fish/test-mount-local.sh:36:if [ $# -gt 0 -a "$1" = "--run-test" ]; then
>> format/test-virt-format.sh:29:if [ "$(../cat/virt-filesystems -a
> test1.img)" != "/dev/sda1" ]; then
>> fuse/test-fuse-umount-race.sh:75:if [ "$(../fish/guestfish -a
> test-copy.qcow2 --ro -i is-file /test-umount)" != "true" ]; then
>> fuse/test-fuse.sh:55:if [ ! -x "$guestfish" -o ! -x "$guestmount" ]; then
>> src/api-support/update-from-tarballs.sh:39: if [ $v != "1.2.0" -a $v
> != "1.3.0" -a ! -f $v ]; then
>> sysprep/test-virt-sysprep-script.sh:40:if [ ! -f stamp-script1.sh -o
> ! -f stamp-script2.sh ]; then
>> tests/md/test-inspect-fstab-md.sh:33:if [ -z "$f" -o ! -f "$f" ]; then
>> tools/test-virt-make-fs.sh:48:if [ "$ntfs3g_available" = "yes" -a
> "$ntfsprogs_available" = "yes" ]; then
>> maint.mk: use "test C1 && test C2", not "test C1 -a C2"; use "test
> C1 || test C2", not "test C1 -o C2"
>> make: *** [sc_prohibit_test_minus_ao] Error 1
>
> What was wrong with -a and -o?  Perhaps we should exempt this test for
> scripts that explicitly begin #!/bin/bash, since presumably these will
> use bash's built-in test.

This is well documented in autoconf's "Limitations of Builtins"
documentation:

`test'
     The `test' program is the way to perform many file and string
     tests.  It is often invoked by the alternate name `[', but using
     that name in Autoconf code is asking for trouble since it is an M4
     quote character.

     The `-a', `-o', `(', and `)' operands are not present in all
     implementations, and have been marked obsolete by Posix 2008.
     This is because there are inherent ambiguities in using them.  For
     example, `test "$1" -a "$2"' looks like a binary operator to check
     whether two strings are both non-empty, but if `$1' is the literal
     `!', then some implementations of `test' treat it as a negation of
     the unary operator `-a'.

     Thus, portable uses of `test' should never have more than four
     arguments, and scripts should use shell constructs like `&&' and
     `||' instead.  If you combine `&&' and `||' in the same statement,
     keep in mind that they have equal precedence, so it is often
     better to parenthesize even when this is redundant.  For example:

          # Not portable:
          test "X$a" = "X$b" -a \
            '(' "X$c" != "X$d" -o "X$e" = "X$f" ')'

          # Portable:
          test "X$a" = "X$b" &&
            { test "X$c" != "X$d" || test "X$e" = "X$f"; }

     `test' does not process options like most other commands do; for
     example, it does not recognize the `--' argument as marking the
     end of options.

     It is safe to use `!' as a `test' operator.  For example, `if test
     ! -d foo; ...' is portable even though `if ! test -d foo; ...' is
     not.

>> prohibit_trailing_blank_lines
>> contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch
>> guestfs-release-notes.txt
>> po/libguestfs.pot
>> tests/guests/guest-aux/debian-packages
>> maint.mk: found trailing blank line(s)
>> make: *** [sc_prohibit_trailing_blank_lines] Error 1
>
> guestfs-release-notes.txt and po/libguestfs.pot are generated files.

I would exempt those files or
change the generation rules to filter out trailing blanks.
Of course, it's even better not to VC generated files,
but I suppose you have no choice because the tools to
perform the generation are not guaranteed to be available.

> debian-packages is effectively a binary file.

exempt it, I suppose,
though it doesn't look like a binary to me.

>> prohibit_undesirable_word_seq
>> BUGS:147:can not
...
>> maint.mk: undesirable word sequence
>> make: *** [sc_prohibit_undesirable_word_seq] Error 1

Pet peeve ;-)

>> require_config_h
>> examples/copy_over.c
>> examples/create_disk.c
>> examples/display_icon.c
>> examples/inspect_vm.c
>> examples/mount_local.c
>> examples/virt-dhcp-address.c
>> tests/c-api/test-just-header.c
>> maint.mk: the above files do not include <config.h>
>> make: *** [sc_require_config_h] Error 1
>
> Examples shouldn't include <config.h>.  The test-just-header.c test
> needs to not include it, because of what it is testing.

Yes, another case of exemption-required.
Yeah, this can be a pain (especially if you do it all at once), but the
consolation is that you have to do it only once.  Any subsequent
failures typically indicate real problems or are quickly dispatched
via another exemption.

>> require_config_h_first
>> examples/copy_over.c
>> examples/create_disk.c
>> examples/display_icon.c
>> examples/inspect_vm.c
>> examples/mount_local.c
>> examples/virt-dhcp-address.c
>> tests/c-api/test-just-header.c
>> maint.mk: the above files include some other header before <config.h>
>> make: *** [sc_require_config_h_first] Error 1
>
>> trailing_blank
>> TODO:405: - swap devices (both of block device and file) should be
> wiped. This may
>> Binary file tests/guests/guest-aux/windows-software matches
>> Binary file tests/guests/guest-aux/windows-system matches
>> tools/virt-win-reg:711:
>> maint.mk: found trailing blank(s)
>> make: *** [sc_trailing_blank] Error 1
>
> Binary files should probably not be matched.

Hey!
I've just realized that I can easily filter out those lines.
Here's yet another patch.
We should be able to use a similar change for a few other rules.

diff --git a/top/maint.mk b/top/maint.mk
index 4627bc5..ccf09a2 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -724,6 +724,7 @@ sc_require_test_exit_idiom:
 sc_trailing_blank:
 	@prohibit='[	 ]$$'						\
 	halt='found trailing blank(s)'					\
+	exclude='^Binary file .* matches$$'				\
 	  $(_sc_search_regexp)

 # Match lines like the following, but where there is only one space


Thanks for the detailed report!