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.