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

[Jim Meyering]

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 at 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.