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

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(a)redhat.com&gt; >> 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(a)redhat.com&gt; >> 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')