[libvirt] 2 patches: dead increment and 4 useless (always-false) disjuncts

[Daniel Veillard]

On Mon, Sep 07, 2009 at 06:33:13PM +0200, Jim Meyering wrote:
> Daniel Veillard wrote:
> > On Mon, Sep 07, 2009 at 05:37:24PM +0200, Jim Meyering wrote:
> >>
> >> >From 74f57af2010f9cbe2315d625c6502a0b259e6802 Mon Sep 17 00:00:00 2001
> >> From: Jim Meyering <meyering at redhat.com>
> >> Date: Mon, 7 Sep 2009 10:03:22 +0200
> >> Subject: [PATCH 1/2] xm_internal.c: remove dead increment of "data"
> >>
> >> * src/xm_internal.c (xenXMDomainConfigParse): Don't increment it.
> >> ---
> >>  src/xm_internal.c |    1 -
> >>  1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/src/xm_internal.c b/src/xm_internal.c
> >> index f206c8c..18613d4 100644
> >> --- a/src/xm_internal.c
> >> +++ b/src/xm_internal.c
> >> @@ -1314,7 +1314,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
> >>
> >>                  if (!(data = strchr(key, '=')) || (data[0] == '\0'))
> >>                      break;
> >> -                data++;
> >>
> >>                  if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> >>                      if (STRPREFIX(key, "vncunused=")) {
> >
> >   trivial, ACK
> >
> >> >From d4db2dfaafbf827d1c8b8626a3dbdaa9f371e479 Mon Sep 17 00:00:00 2001
> >> From: Jim Meyering <meyering at redhat.com>
> >> Date: Mon, 7 Sep 2009 10:09:20 +0200
> >> Subject: [PATCH 2/2] xm_internal.c: remove four useless comparisons after strchr
> >>
> >> * src/xm_internal.c (xenXMDomainConfigParse): After t=strchr...
> >> don't test *t; it's known.  This was *not* detected by clang,
> >> but I spotted it since once instance was in the vicinity of the
> >> dead increment of "data".
> >> ---
> >>  src/xm_internal.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/xm_internal.c b/src/xm_internal.c
> >> index 18613d4..e7f6a55 100644
> >> --- a/src/xm_internal.c
> >> +++ b/src/xm_internal.c
> >> @@ -862,7 +862,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
> >>               */
> >>
> >>              /* Extract the source file path*/
> >> -            if (!(offset = strchr(head, ',')) || offset[0] == '\0')
> >> +            if (!(offset = strchr(head, ',')))
> >>                  goto skipdisk;
> >>              if ((offset - head) >= (PATH_MAX-1))
> >>                  goto skipdisk;
> >> @@ -882,7 +882,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
> >>                  head = head + 6;
> >>
> >>              /* Extract the dest device name */
> >> -            if (!(offset = strchr(head, ',')) || offset[0] == '\0')
> >> +            if (!(offset = strchr(head, ',')))
> >>                  goto skipdisk;
> >>              if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0)
> >>                  goto no_memory;
> >> @@ -1018,7 +1018,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
> >>                  char *data;
> >>                  char *nextkey = strchr(key, ',');
> >>
> >> -                if (!(data = strchr(key, '=')) || (data[0] == '\0'))
> >> +                if (!(data = strchr(key, '=')))
> >>                      goto skipnic;
> >>                  data++;
> >>
> >> @@ -1312,7 +1312,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
> >>                      nextkey++;
> >>                  }
> >>
> >> -                if (!(data = strchr(key, '=')) || (data[0] == '\0'))
> >> +                if (!(data = strchr(key, '=')))
> >>                      break;
> >>
> >>                  if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> >
> >   Hum, I would like to understand the intent of the person who wrote
> > this first, it's like one s trying to handle a case where strchr( ,c)
> > could return the pointer to the end of string if c is not found or
> > something like that. Weird ...
> 
> Dan?
> 
> git show 1b0f541704d0172fdbc4c0e075b37dc2e196d4cc
> 
> or visit this:
>   http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=1b0f541704d0172fdb
> 
> e.g.,
> 
>   +            /* Extract the source */
>   +            if (!(offset = index(head, ',')) || offset[0] == '\0')

  Man index is a bit confusing as it uses NULL to designate the '\0'
character at the end of the string and the NULL pointer. Maybe this led
to some confusion,

  Your patch sounds right, and Dan probably don't remember the obscure
details of that patch 2.5 years ago :-)

  ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/