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