Bug 65424 - assorted patches from the mailing list
Summary: assorted patches from the mailing list
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: low enhancement
Assignee: Simon McVittie
QA Contact:
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-06-05 15:58 UTC by Simon McVittie
Modified: 2013-06-07 04:43 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[configure.ac]: two small fixes (4.69 KB, patch)
2013-06-05 16:02 UTC, Simon McVittie
Details | Splinter Review
dbus-send: check usage a bit strictly (4.85 KB, patch)
2013-06-05 16:03 UTC, Simon McVittie
Details | Splinter Review
[PATCH v2] Assign default value to enable compiler coverage (695 bytes, patch)
2013-06-06 08:15 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v2] dbus-send: check usage a bit strictly (3.13 KB, patch)
2013-06-06 09:15 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2013-06-05 15:58:05 UTC
Chengwei Yang has sent various patches to the mailing list. Opening this bug to keep track of the review process.
Comment 1 Simon McVittie 2013-06-05 16:01:28 UTC
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.
Comment 2 Simon McVittie 2013-06-05 16:02:30 UTC
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:
    ...
Comment 3 Simon McVittie 2013-06-05 16:03:22 UTC
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 4 Simon McVittie 2013-06-05 16:12:25 UTC
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 5 Simon McVittie 2013-06-05 16:25:24 UTC
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.
Comment 6 Simon McVittie 2013-06-05 16:26:38 UTC
Nothing here seems particularly important, but if you want to make these changes, there are some review comments.
Comment 7 Chengwei Yang 2013-06-06 08:15:57 UTC
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
Comment 8 Chengwei Yang 2013-06-06 08:17:28 UTC
(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.
Comment 9 Chengwei Yang 2013-06-06 09:15:43 UTC
Created attachment 80387 [details] [review]
[PATCH v2] dbus-send: check usage a bit strictly
Comment 10 Chengwei Yang 2013-06-06 09:24:34 UTC
(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.
Comment 11 Chengwei Yang 2013-06-07 04:43:06 UTC
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.