11 a.m.
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?
From 3d2b32ad3309ee10ae48f438f61134d2fa00a63a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 15 Mar 2010 21:42:01 +0100
Subject: [PATCH] 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 fb23c52..95d2e47 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -4766,7 +4766,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++) {
@@ -5574,7 +5580,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.455.g91132
11:54 a.m.
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.
>From 3d2b32ad3309ee10ae48f438f61134d2fa00a63a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 15 Mar 2010 21:42:01 +0100
Subject: [PATCH] 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;
This should be '&& virDomainDiskDefAssignAddress(def) < 0'
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 fb23c52..95d2e47 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -4766,7 +4766,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" */
+ }
I prefer it that we use 'XXXX < 0' for error checks, since we sometimes
use positive values for non-error scenarios.
cleanup:
for (i = 0 ; i < nkeywords ; i++) {
@@ -5574,7 +5580,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
goto no_memory;
}
- virDomainDiskDefAssignAddress(disk);
+ if (virDomainDiskDefAssignAddress(disk) != 0)
+ goto error;
Likewise here.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:31 p.m.
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.
From cafce01c52f7f365b31d109e117aaeff021dd6ac Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Fri, 19 Mar 2010 18:26:09 +0100
Subject: [PATCH] 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.455.g91132
> Subject: [PATCH] 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,
...
> + if (def->info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
> + && virDomainDiskDefAssignAddress(def))
> + goto error;
This should be '&& virDomainDiskDefAssignAddress(def) < 0'
Definitely.
...
> - 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" */
> + }
I prefer it that we use 'XXXX < 0' for error checks, since we sometimes
use positive values for non-error scenarios.
Ok.
...
> - virDomainDiskDefAssignAddress(disk);
> + if (virDomainDiskDefAssignAddress(disk) != 0)
> + goto error;
Likewise here.
Ok.
3:38 p.m.
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(a)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(a)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(a)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(a)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
4:09 p.m.
On 03/22/2010 02:38 PM, Jim Meyering wrote:
Here's the final piece.
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.
ACK from me on the series.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:43 a.m.
Eric Blake wrote:
On 03/22/2010 02:38 PM, Jim Meyering wrote:
> Here's the final piece.
>
> 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.
ACK from me on the series.
Thanks. Pushed.
5394
days inactive
5398
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering