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

Re: [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 1

You 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_strcmp

This is getting long.
I'll continue in another message.


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