Richard W.M. Jones
2012-Sep-16 17:31 UTC
[Libguestfs] Remaining syntax-check errors in libguestfs
... and why they are (probably) not errors.> bindtextdomain > daemon/guestfsd.c > erlang/erl-guestfs-proto.c > 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-add-drive-opts.c > tests/c-api/test-add-libvirt-dom.c > tests/c-api/test-command.c > tests/c-api/test-config.c > tests/c-api/test-create-handle.c > tests/c-api/test-debug-to-file.c > tests/c-api/test-just-header.c > tests/c-api/test-last-errno.c > tests/c-api/test-private-data.c > tests/c-api/test-user-cancel.c > tests/charsets/test-charset-fidelity.c > tests/mount-local/test-parallel-mount-local.c > tests/regressions/rhbz501893.c > tests/regressions/rhbz790721.c > maint.mk: the above files do not call bindtextdomain > make: *** [sc_bindtextdomain] Error 1I think for examples, tests and the rather special Erlang case, it's wrong to demand calls to bindtextdomain. The daemon is arguable, but we don't include any locale data in the appliance.> cast_of_argument_to_free > generator/erlang.ml:403: pr " free ((char *) optargs_s.%s);\n" n > generator/ocaml.ml:588: pr " free ((char *) optargs_s.%s);\n" n > generator/python.ml:437: pr " free ((char **) optargs_s.%s);\n" n > maint.mk: don't cast free argument > make: *** [sc_cast_of_argument_to_free] Error 1It seems to me this is another shortcoming of 'const' in C. We currently define the optargs structs as having const char * string fields, eg: struct guestfs_add_domain_argv { //... const char *libvirturi; //... } I tried to change this so that the fields are plain 'char *' but the structs are const, but that doesn't work. ie. The following code would not work: some_func (const char *uri) { const struct guestfs_add_domain_argv optargs = { libvirturi = uri }; guestfs_add_domain_argv (g, dom, &optargs); } Apparently const-ness of a struct does not get inherited by fields inside it, probably because const-ness of a struct just indicates the pointer to the string is const, not the string itself. (Phantom types solve this problem properly and much more generally - having a const keyword in C is an unnecessary mistake IMHO). Anyway, the cast seems necessary in the three cases above.> error_message_period > php/README-PHP:33: echo ("Error: ".guestfs_last_error ($g)."\n"); > php/README-PHP:44: echo ("Error: ".guestfs_last_error ($g)."\n"); > php/extension/guestfs_php_002.phpt:30: die ("Error: ".guestfs_last_error ($g)."\n"); > php/extension/guestfs_php_002.phpt:35: echo ("Error: ".guestfs_last_error ($g)."\n"); > po-docs/ja.po-37794-"force>)." > po-docs/libguestfs-docs.pot-33647-"I<--resize-force>)." > po-docs/uk.po-35901-"force>)." > resize/resize.ml:285: error (f_"%s: unknown partition table type\nvirt-resize only supports MBR (DOS) and GPT partition tables.") infile > resize/resize.ml:685: error (f_"You cannot use --expand when there is no surplus space to expand into. You need to make the target disk larger by at least %s.") > resize/resize.ml:697: error (f_"You cannot use --shrink when there is no deficit (see 'deficit' in the virt-resize(1) man page)."); > resize/resize.ml:714: error (f_"There is a deficit of %Ld bytes (%s). You need to make the target disk larger by at least this amount or adjust your resizing requests.") > src/launch-appliance.c:758: error (g, _("command failed: %s\nerrno: %s\n\nIf qemu is located on a non-standard path, try setting the LIBGUESTFS_QEMU\nenvironment variable. There may also be errors printed above."), > src/launch-libvirt.c-841- "'--format' option, or via the optional 'format' argument to 'add-drive'."), > src/launch.c-196- "This is a limitation of qemu.")); > src/launch.c-284- "This is a limitation of qemu.")); > src/libvirtdomain.c-571- "See ATTACHING TO RUNNING DAEMONS in guestfs(3) for further information.")); > maint.mk: found error message ending in period > make: *** [sc_error_message_period] Error 1The PHP warning seems to be a bug in the script. The other cases are acceptable because the error messages are proper sentences.> makefile_at_at_check > erlang/examples/Makefile.am:1248: --verbatim $(srcdir)/create_disk.erl:@EXAMPLE1@ \ > erlang/examples/Makefile.am:1249: --verbatim $(srcdir)/inspect_vm.erl:@EXAMPLE2@ \ > examples/Makefile.am:1375: --verbatim $(srcdir)/create_disk.c:@EXAMPLE1@ \ > examples/Makefile.am:1376: --verbatim $(srcdir)/inspect_vm.c:@EXAMPLE2@ \ > fish/Makefile.am:1611: --insert $(srcdir)/guestfish-actions.pod:@ACTIONS@ \ > fish/Makefile.am:1612: --insert $(srcdir)/guestfish-commands.pod:@FISH_COMMANDS@ \ > java/examples/Makefile.am:2570: --verbatim $(srcdir)/CreateDisk.java:@EXAMPLE1@ \ > java/examples/Makefile.am:2571: --verbatim $(srcdir)/InspectVM.java:@EXAMPLE2@ \ > ocaml/examples/Makefile.am:2848: --verbatim $(srcdir)/create_disk.ml:@EXAMPLE1@ \ > ocaml/examples/Makefile.am:2849: --verbatim $(srcdir)/inspect_vm.ml:@EXAMPLE2@ \ > perl/examples/Makefile.am:2985: --verbatim $(srcdir)/create_disk.pl:@EXAMPLE1@ \ > perl/examples/Makefile.am:2986: --verbatim $(srcdir)/inspect_vm.pl:@EXAMPLE2@ \ > po-docs/ja/Makefile.am:3215: --insert $(srcdir)/guestfs-actions.pod:@ACTIONS@ \ > po-docs/ja/Makefile.am:3216: --insert $(srcdir)/guestfs-availability.pod:@AVAILABILITY@ \ > po-docs/ja/Makefile.am:3217: --insert $(srcdir)/guestfs-structs.pod:@STRUCTS@ \ > po-docs/ja/Makefile.am:3225: --insert $(srcdir)/guestfish-actions.pod:@ACTIONS@ \ > po-docs/ja/Makefile.am:3226: --insert $(srcdir)/guestfish-commands.pod:@FISH_COMMANDS@ \ > po-docs/ja/Makefile.am:3234: --insert sysprep-extra-options.pod:@EXTRA_OPTIONS@ \ > po-docs/ja/Makefile.am:3235: --insert sysprep-operations.pod:@OPERATIONS@ \ > po-docs/uk/Makefile.am:3353: --insert $(srcdir)/guestfs-actions.pod:@ACTIONS@ \ > po-docs/uk/Makefile.am:3354: --insert $(srcdir)/guestfs-availability.pod:@AVAILABILITY@ \ > po-docs/uk/Makefile.am:3355: --insert $(srcdir)/guestfs-structs.pod:@STRUCTS@ \ > po-docs/uk/Makefile.am:3363: --insert $(srcdir)/guestfish-actions.pod:@ACTIONS@ \ > po-docs/uk/Makefile.am:3364: --insert $(srcdir)/guestfish-commands.pod:@FISH_COMMANDS@ \ > po-docs/uk/Makefile.am:3372: --insert sysprep-extra-options.pod:@EXTRA_OPTIONS@ \ > po-docs/uk/Makefile.am:3373: --insert sysprep-operations.pod:@OPERATIONS@ \ > python/examples/Makefile.am:3611: --verbatim $(srcdir)/create_disk.py:@EXAMPLE1@ \ > python/examples/Makefile.am:3612: --verbatim $(srcdir)/inspect_vm.py:@EXAMPLE2@ \ > ruby/examples/Makefile.am:3931: --verbatim $(srcdir)/create_disk.rb:@EXAMPLE1@ \ > ruby/examples/Makefile.am:3932: --verbatim $(srcdir)/inspect_vm.rb:@EXAMPLE2@ \ > src/Makefile.am:4293: --insert $(srcdir)/guestfs-actions.pod:@ACTIONS@ \ > src/Makefile.am:4294: --insert $(srcdir)/guestfs-availability.pod:@AVAILABILITY@ \ > src/Makefile.am:4295: --insert $(srcdir)/guestfs-structs.pod:@STRUCTS@ \ > sysprep/Makefile.am:4460: --insert sysprep-extra-options.pod:@EXTRA_OPTIONS@ \ > sysprep/Makefile.am:4461: --insert sysprep-operations.pod:@OPERATIONS@ \ > maint.mk: use $(...), not @...@ > make: *** [sc_makefile_at_at_check] Error 1Real bug (in libguestfs). Podwrapper should be changed to use a different character for its substitutions. There is a possibility that these could be accidentally substituted by autoconf.> prohibit_always-defined_macros > cat/virt-ls.c:#define O_CLOEXEC 0 > daemon/daemon.h:#define O_CLOEXEC 0 > daemon/guestfsd.c:# define O_CLOEXEC 0 > examples/mount_local.c:#define O_CLOEXEC 0 > fish/fish.h:#define O_CLOEXEC 0 > generator/tests_c_api.ml:#define O_CLOEXEC 0 > src/guestfs-internal.h:#define O_CLOEXEC 0 > src/info.c:#define O_CLOEXEC 0 > tests/c-api/test-last-errno.c:#define O_CLOEXEC 0 > tests/c-api/test-user-cancel.c:#define O_CLOEXEC 0 > tests/xml/fake-libvirt-xml.c:#define O_CLOEXEC 0 > maint.mk: define the above via some gnulib .h file > make: *** [sc_prohibit_always-defined_macros] Error 1I don't think I understand what's wrong about this. Does gnulib define O_CLOEXEC now?> prohibit_doubled_word > po/pl.po:2947:do do > maint.mk: doubled words > make: *** [sc_prohibit_doubled_word] Error 1Apparently 'do do' is OK in Polish ...> prohibit_empty_lines_at_EOF > contrib/intro/overview.png > contrib/intro/tools.png > contrib/intro/virt-manager-t.png > contrib/intro/virt-manager.png > contrib/intro/vmm-icons-t.png > contrib/intro/vmm-icons.png > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch > erlang/tests/010-load.erl > guestfs-release-notes.txt > html/draft.png > logo/fish.png > po/libguestfs.pot > tests/data/bin-i586-dynamic > tests/data/bin-sparc-dynamic > tests/data/bin-win32.exe > tests/data/bin-win64.exe > tests/data/bin-x86_64-dynamic > tests/data/filesanddirs-100M.tar.xz > tests/data/filesanddirs-10M.tar.xz > tests/data/helloworld.tar > tests/data/helloworld.tar.gz > tests/data/helloworld.tar.xz > tests/data/known-4 > tests/data/known-5 > tests/data/lib-i586.so > tests/data/lib-sparc.so > tests/data/lib-win32.dll > tests/data/lib-win64.dll > tests/data/lib-x86_64.so > tests/data/mbr-ext2-empty.img.gz > tests/guests/guest-aux/debian-packages > tests/guests/guest-aux/minimal-hive > tests/guests/guest-aux/windows-software > tests/guests/guest-aux/windows-system > tests/regressions/rhbz576879.sh > maint.mk: empty line(s) or no newline at EOF > make: *** [sc_prohibit_empty_lines_at_EOF] Error 1One problem with this test is it's matching binaries.> prohibit_magic_number_exit > po-docs/ja.po:59861:"might be called indirectly from L<exit(3)>, which can cause unexpected " > po-docs/libguestfs-docs.pot:50902:"might be called indirectly from L<exit(3)>, which can cause unexpected " > po-docs/uk.po:57345:"might be called indirectly from L<exit(3)>, which can cause unexpected " > src/guestfs.pod:2028:callback might be called indirectly from L<exit(3)>, which can cause > tests/mount-local/test-parallel-mount-local.c:104: exit (77); > tests/mount-local/test-parallel-mount-local.c:110: exit (77); > tests/regressions/rhbz790721.c:79: exit (77); > maint.mk: use EXIT_* values rather than magic number > make: *** [sc_prohibit_magic_number_exit] Error 1I think exit (77) is acceptable. To automake, this means that the test has been skipped, and there is no constant for it. However writing a regexp that matches all numbers except "77" is quite hard.> prohibit_path_max_allocation > daemon/initrd.c:41: char filename[PATH_MAX]; > daemon/initrd.c:126: char fullpath[PATH_MAX]; > daemon/inotify.c:310: char buf[PATH_MAX]; > daemon/link.c:38: char link[PATH_MAX]; > daemon/link.c:63: char link[PATH_MAX]; > daemon/realpath.c:86: char ret[PATH_MAX+1] = "/"; > daemon/xattr.c:272: char pathname[PATH_MAX]; > src/launch-appliance.c:179: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; > src/launch-libvirt.c:200: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; > src/launch-libvirt.c:224: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; > src/launch-unix.c:64: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; > maint.mk: Avoid stack allocations of size PATH_MAX > make: *** [sc_prohibit_path_max_allocation] Error 1The daemon ones are bugs in libguestfs. I haven't fixed them yet. However the use of UNIX_PATH_MAX looks OK to me. I think the regexp is over-matching.> 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 1We want to use strcmp in examples. They don't follow the ordinary code conventions used in the rest of libguestfs. There's something wrong with check matching test-tool.c.> prohibit_strcmp_and_strncmp > 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) { > maint.mk: use STREQ(LEN) in place of the above uses of strcmp(strncmp) > make: *** [sc_prohibit_strcmp_and_strncmp] Error 1Examples should be excluded.> 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 1I think use of strncpy is justified here.> 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 1What 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.> 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 1guestfs-release-notes.txt and po/libguestfs.pot are generated files. debian-packages is effectively a binary file.> prohibit_undesirable_word_seq > BUGS:147:can not > guestfs-release-notes.pod:561:can not > guestfs-release-notes.pod:1630:can not > guestfs-release-notes.pod:1657:can not > guestfs-release-notes.pod:1665:can not > guestfs-release-notes.txt:495:can not > guestfs-release-notes.txt:1550:can not > guestfs-release-notes.txt:1577:can not > guestfs-release-notes.txt:1585:can not > po-docs/ja.po:31860:can not > po-docs/ja.po:34550:can not > po-docs/ja.po:34577:can not > po-docs/ja.po:34585:can not > po-docs/libguestfs-docs.pot:28130:can not > po-docs/libguestfs-docs.pot:30835:can not > po-docs/libguestfs-docs.pot:30871:can not > po-docs/libguestfs-docs.pot:30882:can not > po-docs/uk.po:30301:can not > po-docs/uk.po:32978:can not > po-docs/uk.po:33005:can not > po-docs/uk.po:33013:can not > maint.mk: undesirable word sequence > make: *** [sc_prohibit_undesirable_word_seq] Error 1> 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 1Examples shouldn't include <config.h>. The test-just-header.c test needs to not include it, because of what it is testing.> 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 contrib/intro/overview.png matches > Binary file contrib/intro/tools.png matches > Binary file contrib/intro/virt-manager-t.png matches > Binary file contrib/intro/virt-manager.png matches > Binary file contrib/intro/vmm-icons-t.png matches > Binary file contrib/intro/vmm-icons.png matches > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:34: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:58: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:75: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:86: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:95: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:101: > contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch:102:-- > guestfs-release-notes.txt:300: > guestfs-release-notes.txt:303: > guestfs-release-notes.txt:305: > guestfs-release-notes.txt:308: > guestfs-release-notes.txt:313: > guestfs-release-notes.txt:316: > guestfs-release-notes.txt:319: > guestfs-release-notes.txt:322: > guestfs-release-notes.txt:325: > guestfs-release-notes.txt:327: > guestfs-release-notes.txt:330: > guestfs-release-notes.txt:332: > guestfs-release-notes.txt:335: > guestfs-release-notes.txt:337: > guestfs-release-notes.txt:344: > guestfs-release-notes.txt:346: > guestfs-release-notes.txt:348: > guestfs-release-notes.txt:356: > guestfs-release-notes.txt:360: > guestfs-release-notes.txt:364: > guestfs-release-notes.txt:367: > guestfs-release-notes.txt:374: > guestfs-release-notes.txt:376: > guestfs-release-notes.txt:419: > guestfs-release-notes.txt:422: > guestfs-release-notes.txt:425: > guestfs-release-notes.txt:428: > guestfs-release-notes.txt:431: > guestfs-release-notes.txt:434: > guestfs-release-notes.txt:436: > guestfs-release-notes.txt:438: > guestfs-release-notes.txt:441: > guestfs-release-notes.txt:444: > guestfs-release-notes.txt:446: > guestfs-release-notes.txt:449: > guestfs-release-notes.txt:451: > guestfs-release-notes.txt:453: > guestfs-release-notes.txt:455: > guestfs-release-notes.txt:457: > guestfs-release-notes.txt:459: > guestfs-release-notes.txt:461: > guestfs-release-notes.txt:464: > guestfs-release-notes.txt:466: > guestfs-release-notes.txt:468: > guestfs-release-notes.txt:470: > guestfs-release-notes.txt:473: > guestfs-release-notes.txt:475: > guestfs-release-notes.txt:477: > guestfs-release-notes.txt:479: > guestfs-release-notes.txt:481: > guestfs-release-notes.txt:483: > guestfs-release-notes.txt:536: > guestfs-release-notes.txt:540: > guestfs-release-notes.txt:542: > guestfs-release-notes.txt:545: > guestfs-release-notes.txt:547: > guestfs-release-notes.txt:556: > guestfs-release-notes.txt:558: > guestfs-release-notes.txt:560: > guestfs-release-notes.txt:562: > guestfs-release-notes.txt:564: > guestfs-release-notes.txt:571: > guestfs-release-notes.txt:578: > guestfs-release-notes.txt:584: > guestfs-release-notes.txt:587: > guestfs-release-notes.txt:589: > guestfs-release-notes.txt:592: > guestfs-release-notes.txt:595: > guestfs-release-notes.txt:601: > guestfs-release-notes.txt:623: > guestfs-release-notes.txt:626: > guestfs-release-notes.txt:629: > guestfs-release-notes.txt:631: > guestfs-release-notes.txt:634: > guestfs-release-notes.txt:637: > guestfs-release-notes.txt:639: > guestfs-release-notes.txt:642: > guestfs-release-notes.txt:644: > guestfs-release-notes.txt:647: > guestfs-release-notes.txt:671: > guestfs-release-notes.txt:674: > guestfs-release-notes.txt:677: > guestfs-release-notes.txt:684: > guestfs-release-notes.txt:686: > guestfs-release-notes.txt:711: > guestfs-release-notes.txt:715: > guestfs-release-notes.txt:718: > guestfs-release-notes.txt:732: > guestfs-release-notes.txt:735: > guestfs-release-notes.txt:737: > guestfs-release-notes.txt:743: > guestfs-release-notes.txt:750: > guestfs-release-notes.txt:752: > guestfs-release-notes.txt:754: > guestfs-release-notes.txt:765: > guestfs-release-notes.txt:778: > guestfs-release-notes.txt:780: > guestfs-release-notes.txt:788: > guestfs-release-notes.txt:790: > guestfs-release-notes.txt:793: > guestfs-release-notes.txt:795: > guestfs-release-notes.txt:797: > guestfs-release-notes.txt:800: > guestfs-release-notes.txt:804: > guestfs-release-notes.txt:841: > guestfs-release-notes.txt:843: > guestfs-release-notes.txt:846: > guestfs-release-notes.txt:849: > guestfs-release-notes.txt:855: > guestfs-release-notes.txt:857: > guestfs-release-notes.txt:860: > guestfs-release-notes.txt:867: > guestfs-release-notes.txt:877: > guestfs-release-notes.txt:879: > guestfs-release-notes.txt:881: > guestfs-release-notes.txt:885: > guestfs-release-notes.txt:892: > guestfs-release-notes.txt:894: > guestfs-release-notes.txt:896: > guestfs-release-notes.txt:898: > guestfs-release-notes.txt:900: > guestfs-release-notes.txt:903: > guestfs-release-notes.txt:905: > guestfs-release-notes.txt:908: > guestfs-release-notes.txt:910: > guestfs-release-notes.txt:913: > guestfs-release-notes.txt:932: > guestfs-release-notes.txt:935: > guestfs-release-notes.txt:937: > guestfs-release-notes.txt:939: > guestfs-release-notes.txt:941: > guestfs-release-notes.txt:943: > guestfs-release-notes.txt:945: > guestfs-release-notes.txt:947: > guestfs-release-notes.txt:972: > guestfs-release-notes.txt:975: > guestfs-release-notes.txt:977: > guestfs-release-notes.txt:979: > guestfs-release-notes.txt:982: > guestfs-release-notes.txt:984: > guestfs-release-notes.txt:987: > guestfs-release-notes.txt:989: > guestfs-release-notes.txt:993: > guestfs-release-notes.txt:998: > guestfs-release-notes.txt:1001: > guestfs-release-notes.txt:1004: > guestfs-release-notes.txt:1006: > guestfs-release-notes.txt:1009: > guestfs-release-notes.txt:1012: > guestfs-release-notes.txt:1016: > guestfs-release-notes.txt:1020: > guestfs-release-notes.txt:1022: > guestfs-release-notes.txt:1024: > guestfs-release-notes.txt:1026: > guestfs-release-notes.txt:1028: > guestfs-release-notes.txt:1030: > guestfs-release-notes.txt:1033: > guestfs-release-notes.txt:1035: > guestfs-release-notes.txt:1038: > guestfs-release-notes.txt:1041: > guestfs-release-notes.txt:1044: > guestfs-release-notes.txt:1048: > guestfs-release-notes.txt:1066: > guestfs-release-notes.txt:1069: > guestfs-release-notes.txt:1072: > guestfs-release-notes.txt:1074: > guestfs-release-notes.txt:1109: > guestfs-release-notes.txt:1112: > guestfs-release-notes.txt:1114: > guestfs-release-notes.txt:1116: > guestfs-release-notes.txt:1122: > guestfs-release-notes.txt:1126: > guestfs-release-notes.txt:1130: > guestfs-release-notes.txt:1135: > guestfs-release-notes.txt:1137: > guestfs-release-notes.txt:1140: > guestfs-release-notes.txt:1142: > guestfs-release-notes.txt:1145: > guestfs-release-notes.txt:1147: > guestfs-release-notes.txt:1150: > guestfs-release-notes.txt:1153: > guestfs-release-notes.txt:1156: > guestfs-release-notes.txt:1159: > guestfs-release-notes.txt:1161: > guestfs-release-notes.txt:1164: > guestfs-release-notes.txt:1167: > guestfs-release-notes.txt:1169: > guestfs-release-notes.txt:1175: > guestfs-release-notes.txt:1177: > guestfs-release-notes.txt:1180: > guestfs-release-notes.txt:1199: > guestfs-release-notes.txt:1202: > guestfs-release-notes.txt:1204: > guestfs-release-notes.txt:1207: > guestfs-release-notes.txt:1210: > guestfs-release-notes.txt:1212: > guestfs-release-notes.txt:1214: > guestfs-release-notes.txt:1216: > guestfs-release-notes.txt:1219: > guestfs-release-notes.txt:1222: > guestfs-release-notes.txt:1224: > guestfs-release-notes.txt:1227: > guestfs-release-notes.txt:1251: > guestfs-release-notes.txt:1255: > guestfs-release-notes.txt:1259: > guestfs-release-notes.txt:1261: > guestfs-release-notes.txt:1263: > guestfs-release-notes.txt:1266: > guestfs-release-notes.txt:1269: > guestfs-release-notes.txt:1272: > guestfs-release-notes.txt:1275: > guestfs-release-notes.txt:1278: > guestfs-release-notes.txt:1280: > guestfs-release-notes.txt:1286: > guestfs-release-notes.txt:1288: > guestfs-release-notes.txt:1291: > guestfs-release-notes.txt:1293: > guestfs-release-notes.txt:1296: > guestfs-release-notes.txt:1298: > guestfs-release-notes.txt:1300: > guestfs-release-notes.txt:1302: > guestfs-release-notes.txt:1304: > guestfs-release-notes.txt:1306: > guestfs-release-notes.txt:1308: > guestfs-release-notes.txt:1310: > guestfs-release-notes.txt:1312: > guestfs-release-notes.txt:1314: > guestfs-release-notes.txt:1316: > guestfs-release-notes.txt:1319: > guestfs-release-notes.txt:1322: > guestfs-release-notes.txt:1325: > guestfs-release-notes.txt:1328: > guestfs-release-notes.txt:1337: > guestfs-release-notes.txt:1415: > guestfs-release-notes.txt:1417: > guestfs-release-notes.txt:1420: > guestfs-release-notes.txt:1422: > guestfs-release-notes.txt:1424: > guestfs-release-notes.txt:1426: > guestfs-release-notes.txt:1428: > guestfs-release-notes.txt:1430: > guestfs-release-notes.txt:1435: > guestfs-release-notes.txt:1438: > guestfs-release-notes.txt:1442: > guestfs-release-notes.txt:1445: > guestfs-release-notes.txt:1448: > guestfs-release-notes.txt:1451: > guestfs-release-notes.txt:1455: > guestfs-release-notes.txt:1458: > guestfs-release-notes.txt:1460: > guestfs-release-notes.txt:1463: > guestfs-release-notes.txt:1465: > guestfs-release-notes.txt:1467: > guestfs-release-notes.txt:1469: > guestfs-release-notes.txt:1471: > guestfs-release-notes.txt:1473: > guestfs-release-notes.txt:1475: > guestfs-release-notes.txt:1477: > guestfs-release-notes.txt:1479: > guestfs-release-notes.txt:1481: > Binary file html/draft.png matches > Binary file logo/fish.png matches > Binary file tests/data/bin-i586-dynamic matches > Binary file tests/data/bin-sparc-dynamic matches > Binary file tests/data/bin-win32.exe matches > Binary file tests/data/bin-x86_64-dynamic matches > Binary file tests/data/filesanddirs-100M.tar.xz matches > Binary file tests/data/filesanddirs-10M.tar.xz matches > Binary file tests/data/helloworld.tar matches > Binary file tests/data/lib-i586.so matches > Binary file tests/data/lib-sparc.so matches > Binary file tests/data/lib-win32.dll matches > Binary file tests/data/lib-win64.dll matches > Binary file tests/data/lib-x86_64.so matches > Binary file tests/guests/guest-aux/minimal-hive matches > 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 1Binary files should probably not be matched. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Alex Nelson
2012-Sep-17 03:26 UTC
[Libguestfs] Remaining syntax-check errors in libguestfs
On Sep 16, 2012, at 10:31 , Richard W.M. Jones wrote:> ... and why they are (probably) not errors. >[snip]> > >> prohibit_magic_number_exit >> po-docs/ja.po:59861:"might be called indirectly from L<exit(3)>, which can cause unexpected " >> po-docs/libguestfs-docs.pot:50902:"might be called indirectly from L<exit(3)>, which can cause unexpected " >> po-docs/uk.po:57345:"might be called indirectly from L<exit(3)>, which can cause unexpected " >> src/guestfs.pod:2028:callback might be called indirectly from L<exit(3)>, which can cause >> tests/mount-local/test-parallel-mount-local.c:104: exit (77); >> tests/mount-local/test-parallel-mount-local.c:110: exit (77); >> tests/regressions/rhbz790721.c:79: exit (77); >> maint.mk: use EXIT_* values rather than magic number >> make: *** [sc_prohibit_magic_number_exit] Error 1 > > I think exit (77) is acceptable. To automake, this means that the > test has been skipped, and there is no constant for it. However > writing a regexp that matches all numbers except "77" is quite hard.[snip] Out of curiosity, why exit code 77 (permission denied) instead of exit code 75 (temporary failure, user is invited to retry) [1]? Is this just an automake-ism? --Alex [1] For reference, here's a sysexits listing, consistent with what I saw in /usr/include/sysexits.h on a Fedora 17 VM: <http://www.opensource.apple.com/source/Libc/Libc-320/include/sysexits.h>
Jim Meyering
2012-Sep-17 08:23 UTC
[Libguestfs] Remaining syntax-check errors in libguestfs
Richard W.M. Jones wrote:> ... and why they are (probably) not errors. > >> bindtextdomain >> daemon/guestfsd.c >> erlang/erl-guestfs-proto.c >> 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-add-drive-opts.c >> tests/c-api/test-add-libvirt-dom.c >> tests/c-api/test-command.c >> tests/c-api/test-config.c >> tests/c-api/test-create-handle.c >> tests/c-api/test-debug-to-file.c >> tests/c-api/test-just-header.c >> tests/c-api/test-last-errno.c >> tests/c-api/test-private-data.c >> tests/c-api/test-user-cancel.c >> tests/charsets/test-charset-fidelity.c >> tests/mount-local/test-parallel-mount-local.c >> tests/regressions/rhbz501893.c >> tests/regressions/rhbz790721.c >> maint.mk: the above files do not call bindtextdomain >> make: *** [sc_bindtextdomain] Error 1You may want to disable that test, or to exempt all file names under examples/ and tests/. To exempt those two directories, you can add this to cfg.mk: exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/> I think for examples, tests and the rather special Erlang case, it's > wrong to demand calls to bindtextdomain. The daemon is arguable, but > we don't include any locale data in the appliance. > >> cast_of_argument_to_free >> generator/erlang.ml:403: pr " free ((char *) optargs_s.%s);\n" n >> generator/ocaml.ml:588: pr " free ((char *) optargs_s.%s);\n" n >> generator/python.ml:437: pr " free ((char **) optargs_s.%s);\n" n >> maint.mk: don't cast free argument >> make: *** [sc_cast_of_argument_to_free] Error 1 > > It seems to me this is another shortcoming of 'const' in C.The value of that syntax-check rule is decreasing rapidly, with the trend in some projects to compile C code with a C++ compiler. There, the cast is required, and not anachronistic at all. You might want to simply disable that test (by adding its name to the list in cfg.mk's local-checks-to-skip).> Anyway, the cast seems necessary in the three cases above.An alternative is to exempt those three files by adding this to cfg.mk: exclude_file_name_regexp--sc_cast_of_argument_to_free = \ ^generator/(erlang|ocaml|python).ml$$>> error_message_period >> php/README-PHP:33: echo ("Error: ".guestfs_last_error ($g)."\n"); >> php/README-PHP:44: echo ("Error: ".guestfs_last_error ($g)."\n");style guidelines for diagnostics in GNU programs recommend against periods at end of diagnostics. The above looks like a false positive. The combination of regexps we use to match those uses heuristics that (by definition) will not always work. Exempt or disable. This is definitely a low-value syntax-check rule.>> php/extension/guestfs_php_002.phpt:30: die ("Error: > ".guestfs_last_error ($g)."\n"); >> php/extension/guestfs_php_002.phpt:35: echo ("Error: > ".guestfs_last_error ($g)."\n"); >> po-docs/ja.po-37794-"force>)." >> po-docs/libguestfs-docs.pot-33647-"I<--resize-force>)." >> po-docs/uk.po-35901-"force>)." >> resize/resize.ml:285: error (f_"%s: unknown partition table > type\nvirt-resize only supports MBR (DOS) and GPT partition tables.") > infile >> resize/resize.ml:685: error (f_"You cannot use --expand when there > is no surplus space to expand into. You need to make the target disk > larger by at least %s.") >> resize/resize.ml:697: error (f_"You cannot use --shrink when there > is no deficit (see 'deficit' in the virt-resize(1) man page)."); >> resize/resize.ml:714: error (f_"There is a deficit of %Ld bytes > (%s). You need to make the target disk larger by at least this amount > or adjust your resizing requests.") >> src/launch-appliance.c:758: error (g, _("command failed: %s\nerrno: > %s\n\nIf qemu is located on a non-standard path, try setting the > LIBGUESTFS_QEMU\nenvironment variable. There may also be errors > printed above."), >> src/launch-libvirt.c-841- "'--format' option, or via the optional > format' argument to 'add-drive'."), >> src/launch.c-196- "This is a limitation of qemu.")); >> src/launch.c-284- "This is a limitation of qemu.")); >> src/libvirtdomain.c-571- "See ATTACHING TO RUNNING DAEMONS in > guestfs(3) for further information.")); >> maint.mk: found error message ending in period >> make: *** [sc_error_message_period] Error 1 > > The PHP warning seems to be a bug in the script. The other cases are > acceptable because the error messages are proper sentences. >...> sysprep-operations.pod:@OPERATIONS@ \ >> maint.mk: use $(...), not @...@ >> make: *** [sc_makefile_at_at_check] Error 1 > > Real bug (in libguestfs). Podwrapper should be changed to use a > different character for its substitutions. There is a possibility > that these could be accidentally substituted by autoconf. > >> prohibit_always-defined_macros >> cat/virt-ls.c:#define O_CLOEXEC 0 >> daemon/daemon.h:#define O_CLOEXEC 0 >> daemon/guestfsd.c:# define O_CLOEXEC 0 >> examples/mount_local.c:#define O_CLOEXEC 0 >> fish/fish.h:#define O_CLOEXEC 0 >> generator/tests_c_api.ml:#define O_CLOEXEC 0 >> src/guestfs-internal.h:#define O_CLOEXEC 0 >> src/info.c:#define O_CLOEXEC 0 >> tests/c-api/test-last-errno.c:#define O_CLOEXEC 0 >> tests/c-api/test-user-cancel.c:#define O_CLOEXEC 0 >> tests/xml/fake-libvirt-xml.c:#define O_CLOEXEC 0 >> maint.mk: define the above via some gnulib .h file >> make: *** [sc_prohibit_always-defined_macros] Error 1 > > I don't think I understand what's wrong about this. Does gnulib > define O_CLOEXEC now?Yes.>> prohibit_doubled_word >> po/pl.po:2947:do do >> maint.mk: doubled words >> make: *** [sc_prohibit_doubled_word] Error 1 > > Apparently 'do do' is OK in Polish ...I'd exempt *.po files.>> prohibit_empty_lines_at_EOF >> contrib/intro/overview.png >> contrib/intro/tools.png >> contrib/intro/virt-manager-t.png >> contrib/intro/virt-manager.png >> contrib/intro/vmm-icons-t.png >> contrib/intro/vmm-icons.png >> contrib/visualize-alignment/qemu-0.13-trace-block-device-access.patch >> erlang/tests/010-load.erl >> guestfs-release-notes.txt >> html/draft.png >> logo/fish.png >> po/libguestfs.pot >> tests/data/bin-i586-dynamic >> tests/data/bin-sparc-dynamic >> tests/data/bin-win32.exe >> tests/data/bin-win64.exe >> tests/data/bin-x86_64-dynamic >> tests/data/filesanddirs-100M.tar.xz >> tests/data/filesanddirs-10M.tar.xz >> tests/data/helloworld.tar >> tests/data/helloworld.tar.gz >> tests/data/helloworld.tar.xz >> tests/data/known-4 >> tests/data/known-5 >> tests/data/lib-i586.so >> tests/data/lib-sparc.so >> tests/data/lib-win32.dll >> tests/data/lib-win64.dll >> tests/data/lib-x86_64.so >> tests/data/mbr-ext2-empty.img.gz >> tests/guests/guest-aux/debian-packages >> tests/guests/guest-aux/minimal-hive >> tests/guests/guest-aux/windows-software >> tests/guests/guest-aux/windows-system >> tests/regressions/rhbz576879.sh >> maint.mk: empty line(s) or no newline at EOF >> make: *** [sc_prohibit_empty_lines_at_EOF] Error 1 > > One problem with this test is it's matching binaries.Right. It'd be good to have an automatic way to avoid considering those, but I tend to VC so few binary files that it hasn't reached that threshold.>> prohibit_magic_number_exit >> po-docs/ja.po:59861:"might be called indirectly from L<exit(3)>, > which can cause unexpected " >> po-docs/libguestfs-docs.pot:50902:"might be called indirectly from > L<exit(3)>, which can cause unexpected " >> po-docs/uk.po:57345:"might be called indirectly from L<exit(3)>, > which can cause unexpected " >> src/guestfs.pod:2028:callback might be called indirectly from > L<exit(3)>, which can cause >> tests/mount-local/test-parallel-mount-local.c:104: exit (77); >> tests/mount-local/test-parallel-mount-local.c:110: exit (77); >> tests/regressions/rhbz790721.c:79: exit (77); >> maint.mk: use EXIT_* values rather than magic number >> make: *** [sc_prohibit_magic_number_exit] Error 1 > > I think exit (77) is acceptable. To automake, this means that the > test has been skipped, and there is no constant for it. However > writing a regexp that matches all numbers except "77" is quite hard.But we already have a secondary filtering mechanism, so the patch to exempt "77" is pretty easy. If you confirm that this works for you, I'll push it to gnulib: diff --git a/top/maint.mk b/top/maint.mk index 4627bc5..1722eb7 100644 --- a/top/maint.mk +++ b/top/maint.mk @@ -354,7 +354,7 @@ sc_prohibit_strncpy: # perl -pi -e 's/(^|[^.])\b(exit ?)\(0\)/$1$2(EXIT_SUCCESS)/' sc_prohibit_magic_number_exit: @prohibit='(^|[^.])\<(usage|exit|error) ?\(-?[0-9]+[,)]' \ - exclude='error ?\((0,|[^,]*)' \ + exclude='exit \(77\)|error ?\(((0|77),|[^,]*)' \ halt='use EXIT_* values rather than magic number' \ $(_sc_search_regexp)>> prohibit_path_max_allocation >> daemon/initrd.c:41: char filename[PATH_MAX]; >> daemon/initrd.c:126: char fullpath[PATH_MAX]; >> daemon/inotify.c:310: char buf[PATH_MAX]; >> daemon/link.c:38: char link[PATH_MAX]; >> daemon/link.c:63: char link[PATH_MAX]; >> daemon/realpath.c:86: char ret[PATH_MAX+1] = "/"; >> daemon/xattr.c:272: char pathname[PATH_MAX]; >> src/launch-appliance.c:179: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; >> src/launch-libvirt.c:200: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; >> src/launch-libvirt.c:224: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; >> src/launch-unix.c:64: addr.sun_path[UNIX_PATH_MAX-1] = '\0'; >> maint.mk: Avoid stack allocations of size PATH_MAX >> make: *** [sc_prohibit_path_max_allocation] Error 1 > > The daemon ones are bugs in libguestfs. I haven't fixed them yet. > > However the use of UNIX_PATH_MAX looks OK to me. I think the regexp > is over-matching.Good catch. The regexp in that test is too loose. This fixes it: diff --git a/top/maint.mk b/top/maint.mk index 4627bc5..a6d1324 100644 --- a/top/maint.mk +++ b/top/maint.mk @@ -1216,7 +1216,7 @@ sc_Wundef_boolean: # not be constant, or might overflow a stack. In general, use PATH_MAX as # a limit, not an array or alloca size. sc_prohibit_path_max_allocation: - @prohibit='(\balloca *\([^)]*|\[[^]]*)PATH_MAX' \ + @prohibit='(\balloca *\([^)]*|\[[^]]*)\bPATH_MAX' \ halt='Avoid stack allocations of size PATH_MAX' \ $(_sc_search_regexp)>> prohibit_strcmpThis is getting long. I'll continue in another message.
Jim Meyering
2012-Sep-17 08:47 UTC
[Libguestfs] Remaining syntax-check errors in libguestfs
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 1Pet 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!
Possibly Parallel Threads
- [PATCH 03/13] syntax-check: fix makefile_at_at_check
- [PATCH 0/14] hivex: update gnulib and make "syntax-check" tests pass
- [PATCH 1/6] out-of-tree build: fix documentation generation
- Remaining syntax-check errors
- [PATCH] maint.mk: remove error_message_period check