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@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list