Chengwei Yang has sent various patches to the mailing list. Opening this bug to keep track of the review process.
I applied "doc: fix a little bit for dbus-send" for 1.7.4. I applied "[v3] Fix build error: unused-result" for 1.7.4.
Created attachment 80354 [details] [review] [configure.ac]: two small fixes From: Chengwei Yang <chengwei.yang@intel.com> This commit does polish a little to configure.ac. 1. add configure option for coverage-profiling 2. assign the default value to with_xml Both of them are to avoid null variable in configure summary like below: ... gcc coverage profiling: ... Using XML parser: ...
Created attachment 80355 [details] [review] dbus-send: check usage a bit strictly From: Chengwei Yang <chengwei.yang@intel.com> 1. align --print-reply=, --reply-timeout=, --dest= with --address=. 2. check --print-reply more strictly as the usage says, only =literal is a valid optional value.
Comment on attachment 80354 [details] [review] [configure.ac]: two small fixes Review of attachment 80354 [details] [review]: ----------------------------------------------------------------- > Both of them are to avoid null variable in configure summary like below: severity: enhancement, priority: lowest :-) ::: configure.ac @@ +155,4 @@ > AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support]),enable_kqueue=$enableval,enable_kqueue=auto) > AC_ARG_ENABLE(console-owner-file, AS_HELP_STRING([--enable-console-owner-file],[enable console owner file]),enable_console_owner_file=$enableval,enable_console_owner_file=auto) > AC_ARG_ENABLE(userdb-cache, AS_HELP_STRING([--enable-userdb-cache],[build with userdb-cache support]),enable_userdb_cache=$enableval,enable_userdb_cache=yes) > +AC_ARG_ENABLE(coverage-profiling, AS_HELP_STRING([--enable-coverage-profiling],[build with coverage profiling]),[enable_compiler_coverage=$enableval],[enable_compiler_coverage=no]) This already exists, in m4/compiler.m4. If you want to set an explicit default, that's the place. Alternatively, you could give this block: > if test x$enable_compiler_coverage = xyes; then > ## so that config.h changes when you toggle gcov support > AC_DEFINE_UNQUOTED(DBUS_GCOV_ENABLED, 1, [Defined if gcov is enabled to force a rebuild due to config.h changing]) > fi an "else" branch that sets it to "no"; or indeed, you could modify m4/compiler.m4 so that *it* does the AC_DEFINE_UNQUOTED to force the rebuild, which would mean all our coverage-related stuff was in that file. @@ +159,4 @@ > AC_ARG_ENABLE(launchd, AS_HELP_STRING([--enable-launchd],[build with launchd auto-launch support]),enable_launchd=$enableval,enable_launchd=auto) > AC_ARG_ENABLE(systemd, AS_HELP_STRING([--enable-systemd],[build with systemd at_console support]),enable_systemd=$enableval,enable_systemd=auto) > > +AC_ARG_WITH(xml, AS_HELP_STRING([--with-xml=[libxml/expat]],[XML library to use (libxml may be named libxml2 on some systems), @<:@default=expat@:>@]),[with_xml=$withval],[with_xml=expat]) The default is more like "auto" than "expat", although "auto" currently means "check for expat" because the libxml code path doesn't really work. I would also accept a patch that removed the libxml2 option completely (it's been broken for at least 2.5 years, see Bug #20253) and just made expat a hard requirement. Probably best to do that on Bug #20253.
Comment on attachment 80355 [details] [review] dbus-send: check usage a bit strictly Review of attachment 80355 [details] [review]: ----------------------------------------------------------------- > 1. align --print-reply=, --reply-timeout=, --dest= with --address=. > > 2. check --print-reply more strictly as the usage says, only =literal is a valid optional value. When you find yourself writing bullet points in a commit message, it's often a sign that it should be more than one commit. ::: tools/dbus-send.c @@ +270,2 @@ > { > + if (*(strchr (arg, '=') + 1) == '\0') As far as I can see, the practical effect of this part of the patch is that dbus-send --address= used to fail later, when it tried to connect to "" and found that it couldn't, whereas now it fails immediately; and that dbus-send --addressbook (or anything else starting with --address) would give an error message about "requires an ADDRESS", whereas now it will give the correct "unknown argument" or something? Not particularly important, but OK. @@ +283,5 @@ > print_reply_literal = TRUE; > + else if (*(arg + 13) != '\0') > + { > + fprintf (stderr, "invalid value (%s) of \"--print-reply\"\n", arg + 13); > + usage (1); Fair enough. @@ +290,5 @@ > else if (strstr (arg, "--reply-timeout=") == arg) > { > + if (*(strchr (arg, '=') + 1) == '\0') > + { > + fprintf (stderr, "\"--reply-timeout=\" requires an MSEC\n"); The practical change here appears to be that dbus-send --reply-timeout= ... would previously set a timeout of 0 milliseconds, but now it's an error. Wouldn't it be better to set reply_timeout = strtol(...), then consider it to be an error if reply_timeout == 0? Setting it to 0 is meaningless, and this would catch usages like dbus-send --reply-timeout=not-an-integer ... @@ +300,5 @@ > else if (strstr (arg, "--dest=") == arg) > + { > + if (*(strchr (arg, '=') + 1) == '\0') > + { > + fprintf (stderr, "\"--dest=\" requires an NAME\n"); If you want to validate it, instead of testing whether it's "", test whether it's a valid bus name (using dbus_validate_bus_name()). @@ +309,5 @@ > else if (strstr (arg, "--type=") == arg) > + { > + if (*(strchr (arg, '=') + 1) == '\0') > + { > + fprintf (stderr, "\"--type=\" requires an TYPE\n"); There's no need for this check. When we convert it to an integer, we check that; if you do dbus-send --type= ... you'll get Message type "" is not supported which is perfectly reasonable.
Nothing here seems particularly important, but if you want to make these changes, there are some review comments.
Created attachment 80385 [details] [review] [PATCH v2] Assign default value to enable compiler coverage This is the part for compiler coverage, the another part to delete libxml attached to https://bugs.freedesktop.org/show_bug.cgi?id=20253
(In reply to comment #4) > Comment on attachment 80354 [details] [review] [review] > [configure.ac]: two small fixes > > Review of attachment 80354 [details] [review] [review]: > ----------------------------------------------------------------- > > > Both of them are to avoid null variable in configure summary like below: > > severity: enhancement, priority: lowest :-) > > ::: configure.ac > @@ +155,4 @@ > > AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support]),enable_kqueue=$enableval,enable_kqueue=auto) > > AC_ARG_ENABLE(console-owner-file, AS_HELP_STRING([--enable-console-owner-file],[enable console owner file]),enable_console_owner_file=$enableval,enable_console_owner_file=auto) > > AC_ARG_ENABLE(userdb-cache, AS_HELP_STRING([--enable-userdb-cache],[build with userdb-cache support]),enable_userdb_cache=$enableval,enable_userdb_cache=yes) > > +AC_ARG_ENABLE(coverage-profiling, AS_HELP_STRING([--enable-coverage-profiling],[build with coverage profiling]),[enable_compiler_coverage=$enableval],[enable_compiler_coverage=no]) > > This already exists, in m4/compiler.m4. If you want to set an explicit > default, that's the place. Done, see attachment. > > Alternatively, you could give this block: > > > if test x$enable_compiler_coverage = xyes; then > > ## so that config.h changes when you toggle gcov support > > AC_DEFINE_UNQUOTED(DBUS_GCOV_ENABLED, 1, [Defined if gcov is enabled to force a rebuild due to config.h changing]) > > fi > > an "else" branch that sets it to "no"; or indeed, you could modify > m4/compiler.m4 so that *it* does the AC_DEFINE_UNQUOTED to force the > rebuild, which would mean all our coverage-related stuff was in that file. > > @@ +159,4 @@ > > AC_ARG_ENABLE(launchd, AS_HELP_STRING([--enable-launchd],[build with launchd auto-launch support]),enable_launchd=$enableval,enable_launchd=auto) > > AC_ARG_ENABLE(systemd, AS_HELP_STRING([--enable-systemd],[build with systemd at_console support]),enable_systemd=$enableval,enable_systemd=auto) > > > > +AC_ARG_WITH(xml, AS_HELP_STRING([--with-xml=[libxml/expat]],[XML library to use (libxml may be named libxml2 on some systems), @<:@default=expat@:>@]),[with_xml=$withval],[with_xml=expat]) > > The default is more like "auto" than "expat", although "auto" currently > means "check for expat" because the libxml code path doesn't really work. > > I would also accept a patch that removed the libxml2 option completely (it's > been broken for at least 2.5 years, see Bug #20253) and just made expat a > hard requirement. Probably best to do that on Bug #20253. Done, tracked at there.
Created attachment 80387 [details] [review] [PATCH v2] dbus-send: check usage a bit strictly
(In reply to comment #5) > Comment on attachment 80355 [details] [review] [review] > dbus-send: check usage a bit strictly > > Review of attachment 80355 [details] [review] [review]: > ----------------------------------------------------------------- > > > 1. align --print-reply=, --reply-timeout=, --dest= with --address=. > > > > 2. check --print-reply more strictly as the usage says, only =literal is a valid optional value. > > When you find yourself writing bullet points in a commit message, it's often > a sign that it should be more than one commit. Yes, fair point, however, I think it's OK since the patch is dedicate to polish dbus-send usage checking. > > ::: tools/dbus-send.c > @@ +270,2 @@ > > { > > + if (*(strchr (arg, '=') + 1) == '\0') > > As far as I can see, the practical effect of this part of the patch is that > > dbus-send --address= > > used to fail later, when it tried to connect to "" and found that it > couldn't, whereas now it fails immediately; and that > > dbus-send --addressbook > > (or anything else starting with --address) would give an error message about > "requires an ADDRESS", whereas now it will give the correct "unknown > argument" or something? Yes. Just align with the other options usage. > > Not particularly important, but OK. > > @@ +283,5 @@ > > print_reply_literal = TRUE; > > + else if (*(arg + 13) != '\0') > > + { > > + fprintf (stderr, "invalid value (%s) of \"--print-reply\"\n", arg + 13); > > + usage (1); > > Fair enough. > > @@ +290,5 @@ > > else if (strstr (arg, "--reply-timeout=") == arg) > > { > > + if (*(strchr (arg, '=') + 1) == '\0') > > + { > > + fprintf (stderr, "\"--reply-timeout=\" requires an MSEC\n"); > > The practical change here appears to be that > > dbus-send --reply-timeout= ... > > would previously set a timeout of 0 milliseconds, but now it's an error. Yes. > > Wouldn't it be better to set reply_timeout = strtol(...), then consider it > to be an error if reply_timeout == 0? Setting it to 0 is meaningless, and > this would catch usages like > > dbus-send --reply-timeout=not-an-integer ... Yes, I treat them different errors 1. --reply-timout= will through a "--reply-timeout=" requires an MSEC error 2. --reply-timeout=<invalid number> will through a "invalid value (<invalid number) of --reply-timeout" Because if we through out error 1 in error2, then the invalid value is nothing, not as helpful as "--reply-timeout=" requires an MSEC. > > @@ +300,5 @@ > > else if (strstr (arg, "--dest=") == arg) > > + { > > + if (*(strchr (arg, '=') + 1) == '\0') > > + { > > + fprintf (stderr, "\"--dest=\" requires an NAME\n"); > > If you want to validate it, instead of testing whether it's "", test whether > it's a valid bus name (using dbus_validate_bus_name()). Done. > > @@ +309,5 @@ > > else if (strstr (arg, "--type=") == arg) > > + { > > + if (*(strchr (arg, '=') + 1) == '\0') > > + { > > + fprintf (stderr, "\"--type=\" requires an TYPE\n"); > > There's no need for this check. When we convert it to an integer, we check > that; if you do > > dbus-send --type= ... > > you'll get > > Message type "" is not supported > > which is perfectly reasonable. Agreed.
I see these patches applied, so close this issue.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.