[libvirt] 2 patches: dead increment and 4 useless (always-false) disjuncts
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=")) { -- 1.6.4.2.419.gab238
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) { -- 1.6.4.2.419.gab238
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 ... Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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')
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 :-) ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
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
Thanks. Pushed.
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 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering