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

Re: [libvirt] [PATCH] virDomainDiskDefAssignAddress: return int, not void



Jim Meyering wrote:
> Daniel P. Berrange wrote:
>> On Fri, Mar 19, 2010 at 05:00:21PM +0100, Jim Meyering wrote:
>>> Per discussion a week or so ago,
>>> here's a fix for virDomainDiskDefAssignAddress.
>>> However, this change is incomplete, because making that function
>>> reject erroneous input has exposed problems elsewhere.
>>> For starters, this change causes three previously passing tests to fail:
>>>
>>>
>>>       TEST: virshtest
>>>             .!.!!!!!!!!!!!!!                         16  FAIL
>>>       FAIL: virshtest
>>>
>>>       TEST: int-overflow
>>>       --- err 2010-03-19 16:50:29.869979267 +0100
>>>       +++ exp 2010-03-19 16:50:29.847854045 +0100
>>>       @@ -1,2 +1 @@
>>>       -error: Unknown failure
>>>       -error: failed to connect to the hypervisor
>>>       +error: failed to get domain '4294967298'
>>>       FAIL: int-overflow
>>>
>>>       TEST: xml2sexprtest
>>>             .................!.....!................ 40
>>>             ...                                      43  FAIL
>>>       FAIL: xml2sexprtest
>>>
>>>
>>> Those fail because virDomainDiskDefAssignAddress ends
>>> up processing a "def" with def->dst values of "xvda:disk"
>>> and "sda1", both of which make virDiskNameToIndex(def->dst) return -1.
>>>
>>> I confirmed that simply removing any ":disk" suffix
>>> and mapping "sda1" to "sda" would solve the problem,
>>> but the question is where to make that change.
>>>
>>> There are numerous input XML files that mention "sda1",
>>> so changing test inputs is probably not an option.
>>>
>>> There is already code to remove the :disk suffix, e.e., here:
>>>
>>>     src/xen/xend_internal.c:  } else if (STREQ (offset, ":disk")) {
>>>     ...
>>>     src/xen/xend_internal.c-  offset[0] = '\0';
>>>
>>> Suggestions?
>>
>> Need to figure out why the virDomainDefPtr  object ends up with
>> a ':disk' suffix. This should definitely be stripped by the SEXPR
>> parser before it gets into the virDomainDefPtr object.
>>
>> The 'sda1' is a valid device (unfortunately). So for that we'll need
>> to make the virDiskNameToIndex method strip/ignore any trailing
>> digits.
>
> Ok.  Here's an independent patch to address that case.
> With it (on top of the preceding patch) only the xml2sexprtest fails.
...
> Subject: [PATCH] virDiskNameToIndex: ignore trailing digits

Here's the final piece.
With this and the other two patches, all tests pass once again.

At first, I changed the target string in virDomainDiskDefParseXML,
(removing the ":disk" suffix) but that resulted in these two failures:

  TEST: xml2sexprtest
   1) Xen XML-2-SEXPR pv -> pv                                          ... OK
  ...
  18) Xen XML-2-SEXPR curmem -> curmem                                  ...
  Expect [:disk']
  Actual [']
                                                                        ... FAILED
  19) Xen XML-2-SEXPR net-routed -> net-routed                          ... OK
  ...
  24) Xen XML-2-SEXPR pv-localtime -> pv-localtime                      ...
  Expect [:disk']
  Actual [']
                                                                        ... FAILED

Then Daniel Berrange mentioned that
we don't need to change parsing at all, since
there's no need to handle the ":disk" suffix in XML.

Hence, the solution is simply to fix the test .xml input files
and corresponding expected-output .sexpr files to match.

Here's the final series.
I'm including the result of Dan's review as a separate patch
only for review.  I'll fold it into the 3/4 before pushing.
The new test-modifying patch is 2/4:

  [PATCH 1/4] virDiskNameToIndex: ignore trailing digits
  [PATCH 2/4] tests: do not use the ":disk" suffix in sample xml input
  [PATCH 3/4] virDomainDiskDefAssignAddress: return int, not void
  [PATCH 4/4] review feedback from danpb

>From 220752af49183d195f428a81fda3b631f2b8ada3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Fri, 19 Mar 2010 18:26:09 +0100
Subject: [PATCH 1/4] virDiskNameToIndex: ignore trailing digits

* src/util/util.c (virDiskNameToIndex): Accept sda1, and map it to "sda".
I.e., accept and ignore any string of trailing digits.
---
 src/util/util.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c
index 87b0714..b29caa5 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2211,8 +2211,9 @@ const char *virEnumToString(const char *const*types,
     return types[type];
 }

-/* Translates a device name of the form (regex) "[fhv]d[a-z]+" into
- * the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26)
+/* Translates a device name of the form (regex) /^[fhv]d[a-z]+[0-9]*$/
+ * into the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26)
+ * Note that any trailing string of digits is simply ignored.
  * @param name The name of the device
  * @return name's index, or -1 on failure
  */
@@ -2236,12 +2237,17 @@ int virDiskNameToIndex(const char *name) {
         idx = (idx + (i < 1 ? 0 : 1)) * 26;

         if (!c_islower(*ptr))
-            return -1;
+            break;

         idx += *ptr - 'a';
         ptr++;
     }

+    /* Count the trailing digits.  */
+    size_t n_digits = strspn(ptr, "0123456789");
+    if (ptr[n_digits] != '\0')
+        return -1;
+
     return idx;
 }

--
1.7.0.2.486.gfdfcd


>From e20a2bf255940997771da85ae6980297b2a54c13 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Mon, 22 Mar 2010 20:21:18 +0100
Subject: [PATCH 2/4] tests: do not use the ":disk" suffix in sample xml input

* tests/xml2sexprdata/xml2sexpr-curmem.xml: Remove ":disk" suffix from
"<target dev=" value.
* tests/xml2sexprdata/xml2sexpr-pv-localtime.xml: Likewise.
* tests/xml2sexprdata/xml2sexpr-curmem.sexpr: Update expected output
to match.
* tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr: Likewise.
---
 tests/xml2sexprdata/xml2sexpr-curmem.sexpr       |    2 +-
 tests/xml2sexprdata/xml2sexpr-curmem.xml         |    2 +-
 tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr |    2 +-
 tests/xml2sexprdata/xml2sexpr-pv-localtime.xml   |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/xml2sexprdata/xml2sexpr-curmem.sexpr b/tests/xml2sexprdata/xml2sexpr-curmem.sexpr
index 321af9b..4b9c9a7 100644
--- a/tests/xml2sexprdata/xml2sexpr-curmem.sexpr
+++ b/tests/xml2sexprdata/xml2sexpr-curmem.sexpr
@@ -1 +1 @@
-(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2-3019-58e8-3bab-6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(device (tap (dev 'xvda:disk')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
\ No newline at end of file
+(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2-3019-58e8-3bab-6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(device (tap (dev 'xvda')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
\ No newline at end of file
diff --git a/tests/xml2sexprdata/xml2sexpr-curmem.xml b/tests/xml2sexprdata/xml2sexpr-curmem.xml
index 3347a7d..6d1741d 100644
--- a/tests/xml2sexprdata/xml2sexpr-curmem.xml
+++ b/tests/xml2sexprdata/xml2sexpr-curmem.xml
@@ -17,7 +17,7 @@
     <disk type='file' device='disk'>
       <driver name='tap' type='aio'/>
       <source file='/xen/rhel5.img'/>
-      <target dev='xvda:disk'/>
+      <target dev='xvda'/>
     </disk>
     <graphics type='vnc' port='5905'/>
   </devices>
diff --git a/tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr b/tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr
index af4582f..16bbdfd 100644
--- a/tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr
+++ b/tests/xml2sexprdata/xml2sexpr-pv-localtime.sexpr
@@ -1 +1 @@
-(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2-3019-58e8-3bab-6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(localtime 1)(device (tap (dev 'xvda:disk')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
\ No newline at end of file
+(vm (name 'rhel5')(memory 175)(maxmem 385)(vcpus 1)(uuid '4f77abd2-3019-58e8-3bab-6fbf2118f880')(bootloader '/usr/bin/pygrub')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(localtime 1)(device (tap (dev 'xvda')(uname 'tap:aio:/xen/rhel5.img')(mode 'w')))(device (vif (mac '00:16:3e:1d:06:15')(bridge 'xenbr0')(script 'vif-bridge'))))
\ No newline at end of file
diff --git a/tests/xml2sexprdata/xml2sexpr-pv-localtime.xml b/tests/xml2sexprdata/xml2sexpr-pv-localtime.xml
index e78c09c..2ce1e44 100644
--- a/tests/xml2sexprdata/xml2sexpr-pv-localtime.xml
+++ b/tests/xml2sexprdata/xml2sexpr-pv-localtime.xml
@@ -18,7 +18,7 @@
     <disk type='file' device='disk'>
       <driver name='tap' type='aio'/>
       <source file='/xen/rhel5.img'/>
-      <target dev='xvda:disk'/>
+      <target dev='xvda'/>
     </disk>
     <graphics type='vnc' port='5905'/>
   </devices>
--
1.7.0.2.486.gfdfcd


>From 3e14469bfd069e748c7742334492270f2130b975 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Mon, 15 Mar 2010 21:42:01 +0100
Subject: [PATCH 3/4] virDomainDiskDefAssignAddress: return int, not void

Before, this function would blindly accept an invalid def->dst
and then abuse the idx=-1 it would get from virDiskNameToIndex,
when passing it invalid strings like "xvda:disk" and "sda1".
Now, this function returns -1 upon failure.
* src/conf/domain_conf.c (virDomainDiskDefAssignAddress): as above.
Update callers.
* src/conf/domain_conf.h: Update prototype.
* src/qemu/qemu_conf.c: Update callers.
---
 src/conf/domain_conf.c |   11 ++++++++---
 src/conf/domain_conf.h |    4 ++--
 src/qemu/qemu_conf.c   |   11 +++++++++--
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 56c5239..5968405 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1233,10 +1233,12 @@ cleanup:
 }


-void
+int
 virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
 {
     int idx = virDiskNameToIndex(def->dst);
+    if (idx < 0)
+        return -1;

     switch (def->bus) {
     case VIR_DOMAIN_DISK_BUS_SCSI:
@@ -1270,6 +1272,8 @@ virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
         /* Other disk bus's aren't controller based */
         break;
     }
+
+    return 0;
 }

 /* Parse the XML definition for a disk
@@ -1498,8 +1502,9 @@ virDomainDiskDefParseXML(xmlNodePtr node,
     def->serial = serial;
     serial = NULL;

-    if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
-        virDomainDiskDefAssignAddress(def);
+    if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
+        && virDomainDiskDefAssignAddress(def))
+        goto error;

 cleanup:
     VIR_FREE(bus);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0540a77..44fff0c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1,7 +1,7 @@
 /*
  * domain_conf.h: domain XML processing
  *
- * Copyright (C) 2006-2008 Red Hat, Inc.
+ * Copyright (C) 2006-2008, 2010 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -855,7 +855,7 @@ int virDomainDiskInsert(virDomainDefPtr def,
                         virDomainDiskDefPtr disk);
 void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
                                    virDomainDiskDefPtr disk);
-void virDomainDiskDefAssignAddress(virDomainDiskDefPtr def);
+int virDomainDiskDefAssignAddress(virDomainDiskDefPtr def);

 int virDomainControllerInsert(virDomainDefPtr def,
                               virDomainControllerDefPtr controller);
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index f2d36f7..c5d7b9a 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -4815,7 +4815,13 @@ qemuParseCommandLineDisk(const char *val,
     else
         def->dst[2] = 'a' + idx;

-    virDomainDiskDefAssignAddress(def);
+    if (virDomainDiskDefAssignAddress(def) != 0) {
+        qemuReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("invalid device name '%s'"), def->dst);
+        virDomainDiskDefFree(def);
+        def = NULL;
+        /* fall through to "cleanup" */
+    }

 cleanup:
     for (i = 0 ; i < nkeywords ; i++) {
@@ -5623,7 +5629,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
                 goto no_memory;
             }

-            virDomainDiskDefAssignAddress(disk);
+            if (virDomainDiskDefAssignAddress(disk) != 0)
+                goto error;

             if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) {
                 virDomainDiskDefFree(disk);
--
1.7.0.2.486.gfdfcd


>From a1a74f29092b852dd2935b4df24847ab781b1fa6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Fri, 19 Mar 2010 18:05:23 +0100
Subject: [PATCH 4/4] review feedback from danpb

---
 src/conf/domain_conf.c |    2 +-
 src/qemu/qemu_conf.c   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5968405..79c7ea3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1503,7 +1503,7 @@ virDomainDiskDefParseXML(xmlNodePtr node,
     serial = NULL;

     if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
-        && virDomainDiskDefAssignAddress(def))
+        && virDomainDiskDefAssignAddress(def) < 0)
         goto error;

 cleanup:
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c5d7b9a..902eecb 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -4815,7 +4815,7 @@ qemuParseCommandLineDisk(const char *val,
     else
         def->dst[2] = 'a' + idx;

-    if (virDomainDiskDefAssignAddress(def) != 0) {
+    if (virDomainDiskDefAssignAddress(def) < 0) {
         qemuReportError(VIR_ERR_INTERNAL_ERROR,
                         _("invalid device name '%s'"), def->dst);
         virDomainDiskDefFree(def);
@@ -5629,7 +5629,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
                 goto no_memory;
             }

-            if (virDomainDiskDefAssignAddress(disk) != 0)
+            if (virDomainDiskDefAssignAddress(disk) < 0)
                 goto error;

             if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) {
--
1.7.0.2.486.gfdfcd


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