[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]