3:23 a.m.
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.