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

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



On Tue, Sep 08, 2009 at 10:35:07AM +0200, Daniel Veillard wrote:
> 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 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 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 :-)

With the horrible 'xm' driver my policy is always that the test cases
are correct and the driver code is broken unless proven otherwise.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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