diff mbox

[libvirt,V2,4/4] libxl: add support for rbd qdisk

Message ID 56CD2D6C.7060608@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jim Fehlig Feb. 24, 2016, 4:11 a.m. UTC 
On 02/22/2016 07:35 AM, Ján Tomko wrote:
> On Wed, Feb 17, 2016 at 05:33:45PM -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring such
>> <disk>s for long time too. This patch adds support for rbd
>> disks in the libxl driver by generating a rbd device URL from
>> the virDomainDiskDef object. The URL is passed to libxl via the
>> pdev_path field of libxl_device_disk struct. libxl then passes
>> the URL to qemu for cosumption by the rbd backend.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>  src/libxl/libxl_conf.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 191 insertions(+), 1 deletion(-)
>>
> ACK with the whitespace fix.
>
>> +
>> +static int
>> +libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
>> +{
>> +    virConnectPtr conn = NULL;
>> +    char *secret = NULL;
>> +    char *username = NULL;
>> +    int ret = -1;
>> +
>> +    *srcstr = NULL;
>> +    if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> +        const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
>> +
>> +        username = src->auth->username;
>> +        if (!(conn = virConnectOpen("xen:///system")))
>> +            goto cleanup;
>> +
> Opening a connection feels out of place in this function, but I see it's
> already done for NICs.
>
> It would be nice to reuse it as is done in the qemu driver.

Heh, this has turned out to be a PITA. Something like the attached patch works,
but it still has the virConnectOpen in the libxlBuildDomainConfig call path. I
would prefer to use the virConnect object associated with the request, instead
of opening one in this code. I have some old, dusty patches (originally started
by danpb) that provide unit tests for libxlBuildDomainConfig. E.g. domXML ->
virDomainDef -> libxl_domain_config -> json representation -> compare with
expected json. Code in this path attempting to open "xen:///" connections is
likely to break many 'make check' setups :-).

But fixing this is not easy given the current code structure. There is one place
in particular that is rather troublesome - reboot. libxlDomainStart, and hence
libxlBuildDomainConfig, are called when a reboot event is received from libxl
(e.g. 'shutdown -r now' within a VM). As you might guess, there is no virConnect
object associated with such request. The code needs reworked quite a bit to
handle accessing a virConnect object while building a libxl_domain_config
object. I'll work on this when dusting off the unit test patches, although it is
not high in my queue. Maybe it would make a good libvirt GSoC project :-).

Regards,
Jim
diff mbox

Patch

From 869c76c029390ebb6e2b89b3bb0ccf9ac4610806 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
Date: Tue, 23 Feb 2016 16:20:29 -0700
Subject: [PATCH] libxl: use a common virConnect object

Depending on the type of NIC or disk, libxlMakeNic and libxlMakeDisk
may need a virConnect object, e.g. to obtain a port from the network
driver or a secret from the secret driver. Instead of opening a
virConnect object in each of these functions, have the callers provide
the object.

This approach provides benefits over opening individual virConnect
objects, e.g. already fixing a virConnect unref bug in libxlMakeNic.
The downside is changing several functions to include a virConnectPtr
parameter.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_conf.c   | 71 ++++++++++++++++++++++++++----------------------
 src/libxl/libxl_conf.h   |  7 +++--
 src/libxl/libxl_driver.c | 43 ++++++++++++++++++-----------
 3 files changed, 71 insertions(+), 50 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a109bb5..5d5c85e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1071,9 +1071,10 @@  libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
 }
 
 static int
-libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
+libxlMakeNetworkDiskSrc(virConnectPtr conn,
+                        virStorageSourcePtr src,
+                        char **srcstr)
 {
-    virConnectPtr conn = NULL;
     char *secret = NULL;
     char *username = NULL;
     int ret = -1;
@@ -1083,9 +1084,6 @@  libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
         const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
 
         username = src->auth->username;
-        if (!(conn = virConnectOpen("xen:///system")))
-            goto cleanup;
-
         if (!(secret = libxlGetSecretString(conn,
                                             protocol,
                                             true,
@@ -1101,12 +1099,13 @@  libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
 
  cleanup:
     VIR_FREE(secret);
-    virObjectUnref(conn);
     return ret;
 }
 
 int
-libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
+libxlMakeDisk(virConnectPtr conn,
+              virDomainDiskDefPtr l_disk,
+              libxl_device_disk *x_disk)
 {
     const char *driver;
     int format;
@@ -1115,7 +1114,7 @@  libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
     libxl_device_disk_init(x_disk);
 
     if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
-        if (libxlMakeNetworkDiskSrc(l_disk->src, &x_disk->pdev_path) < 0)
+        if (libxlMakeNetworkDiskSrc(conn, l_disk->src, &x_disk->pdev_path) < 0)
             return -1;
     } else {
         if (VIR_STRDUP(x_disk->pdev_path, virDomainDiskGetSource(l_disk)) < 0)
@@ -1252,7 +1251,9 @@  libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
 }
 
 static int
-libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeDiskList(virConnectPtr conn,
+                  virDomainDefPtr def,
+                  libxl_domain_config *d_config)
 {
     virDomainDiskDefPtr *l_disks = def->disks;
     int ndisks = def->ndisks;
@@ -1263,7 +1264,7 @@  libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
         return -1;
 
     for (i = 0; i < ndisks; i++) {
-        if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
+        if (libxlMakeDisk(conn, l_disks[i], &x_disks[i]) < 0)
             goto error;
     }
 
@@ -1280,7 +1281,8 @@  libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
 }
 
 int
-libxlMakeNic(virDomainDefPtr def,
+libxlMakeNic(virConnectPtr conn,
+             virDomainDefPtr def,
              virDomainNetDefPtr l_nic,
              libxl_device_nic *x_nic)
 {
@@ -1340,16 +1342,10 @@  libxlMakeNic(virDomainDefPtr def,
             bool fail = false;
             char *brname = NULL;
             virNetworkPtr network;
-            virConnectPtr conn;
-
-            if (!(conn = virConnectOpen("xen:///system")))
-                return -1;
 
             if (!(network =
-                  virNetworkLookupByName(conn, l_nic->data.network.name))) {
-                virObjectUnref(conn);
+                  virNetworkLookupByName(conn, l_nic->data.network.name)))
                 return -1;
-            }
 
             if (l_nic->nips > 0) {
                 x_nic->ip = virSocketAddrFormat(&l_nic->ips[0]->address);
@@ -1367,7 +1363,6 @@  libxlMakeNic(virDomainDefPtr def,
             VIR_FREE(brname);
 
             virObjectUnref(network);
-            virObjectUnref(conn);
             if (fail)
                 return -1;
             break;
@@ -1442,7 +1437,9 @@  libxlMakeNic(virDomainDefPtr def,
 }
 
 static int
-libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
+libxlMakeNicList(virConnectPtr conn,
+                 virDomainDefPtr def,
+                 libxl_domain_config *d_config)
 {
     virDomainNetDefPtr *l_nics = def->nets;
     size_t nnics = def->nnets;
@@ -1456,7 +1453,7 @@  libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
         if (l_nics[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
             continue;
 
-        if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics]))
+        if (libxlMakeNic(conn, def, l_nics[i], &x_nics[nvnics]))
             goto error;
         /*
          * The devid (at least right now) will not get initialized by
@@ -2118,28 +2115,34 @@  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
                        libxl_ctx *ctx,
                        libxl_domain_config *d_config)
 {
+    virConnectPtr conn = NULL;
+    int ret = -1;
+
     libxl_domain_config_init(d_config);
 
-    if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0)
+    if (!(conn = virConnectOpen("xen:///system")))
         return -1;
 
+     if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0)
+        goto cleanup;
+
     if (libxlMakeDomBuildInfo(def, ctx, d_config) < 0)
-        return -1;
+        goto cleanup;
 
-    if (libxlMakeDiskList(def, d_config) < 0)
-        return -1;
+    if (libxlMakeDiskList(conn, def, d_config) < 0)
+        goto cleanup;
 
-    if (libxlMakeNicList(def, d_config) < 0)
-        return -1;
+    if (libxlMakeNicList(conn, def, d_config) < 0)
+        goto cleanup;
 
     if (libxlMakeVfbList(graphicsports, def, d_config) < 0)
-        return -1;
+        goto cleanup;
 
     if (libxlMakeBuildInfoVfb(graphicsports, def, d_config) < 0)
-        return -1;
+        goto cleanup;
 
     if (libxlMakePCIList(def, d_config) < 0)
-        return -1;
+        goto cleanup;
 
     /*
      * Now that any potential VFBs are defined, update the build info with
@@ -2147,13 +2150,17 @@  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
      * so but as it does not right now, better be explicit.
      */
     if (libxlMakeVideo(def, d_config) < 0)
-        return -1;
+        goto cleanup;
 
     d_config->on_reboot = libxlActionFromVirLifecycle(def->onReboot);
     d_config->on_poweroff = libxlActionFromVirLifecycle(def->onPoweroff);
     d_config->on_crash = libxlActionFromVirLifecycleCrash(def->onCrash);
 
-    return 0;
+    ret = 0;
+
+ cleanup:
+    virObjectUnref(conn);
+    return ret;
 }
 
 virDomainXMLOptionPtr
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 3c0eafb..94fb866 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -192,9 +192,12 @@  int
 libxlDomainGetEmulatorType(const virDomainDef *def);
 
 int
-libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
+libxlMakeDisk(virConnectPtr conn,
+              virDomainDiskDefPtr l_dev,
+              libxl_device_disk *x_dev);
 int
-libxlMakeNic(virDomainDefPtr def,
+libxlMakeNic(virConnectPtr conn,
+             virDomainDefPtr def,
              virDomainNetDefPtr l_nic,
              libxl_device_nic *x_nic);
 int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 404016e..531e2d6 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2912,7 +2912,9 @@  libxlDomainUndefine(virDomainPtr dom)
 }
 
 static int
-libxlDomainChangeEjectableMedia(virDomainObjPtr vm, virDomainDiskDefPtr disk)
+libxlDomainChangeEjectableMedia(virConnectPtr conn,
+                                virDomainObjPtr vm,
+                                virDomainDiskDefPtr disk)
 {
     libxlDriverConfigPtr cfg = libxlDriverConfigGet(libxl_driver);
     virDomainDiskDefPtr origdisk = NULL;
@@ -2942,7 +2944,7 @@  libxlDomainChangeEjectableMedia(virDomainObjPtr vm, virDomainDiskDefPtr disk)
         return -1;
     }
 
-    if (libxlMakeDisk(disk, &x_disk) < 0)
+    if (libxlMakeDisk(conn, disk, &x_disk) < 0)
         goto cleanup;
 
     if ((ret = libxl_cdrom_insert(cfg->ctx, vm->def->id, &x_disk, NULL)) < 0) {
@@ -2966,7 +2968,9 @@  libxlDomainChangeEjectableMedia(virDomainObjPtr vm, virDomainDiskDefPtr disk)
 }
 
 static int
-libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
+libxlDomainAttachDeviceDiskLive(virConnectPtr conn,
+                                virDomainObjPtr vm,
+                                virDomainDeviceDefPtr dev)
 {
     libxlDriverConfigPtr cfg = libxlDriverConfigGet(libxl_driver);
     virDomainDiskDefPtr l_disk = dev->data.disk;
@@ -2975,7 +2979,7 @@  libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
 
     switch (l_disk->device)  {
         case VIR_DOMAIN_DISK_DEVICE_CDROM:
-            ret = libxlDomainChangeEjectableMedia(vm, l_disk);
+            ret = libxlDomainChangeEjectableMedia(conn, vm, l_disk);
             break;
         case VIR_DOMAIN_DISK_DEVICE_DISK:
             if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
@@ -2994,7 +2998,7 @@  libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
                 if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
                     goto cleanup;
 
-                if (libxlMakeDisk(l_disk, &x_disk) < 0)
+                if (libxlMakeDisk(conn, l_disk, &x_disk) < 0)
                     goto cleanup;
 
                 if (virDomainLockDiskAttach(libxl_driver->lockManager,
@@ -3119,7 +3123,9 @@  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
 }
 
 static int
-libxlDomainDetachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
+libxlDomainDetachDeviceDiskLive(virConnectPtr conn,
+                                virDomainObjPtr vm,
+                                virDomainDeviceDefPtr dev)
 {
     libxlDriverConfigPtr cfg = libxlDriverConfigGet(libxl_driver);
     virDomainDiskDefPtr l_disk = NULL;
@@ -3141,7 +3147,7 @@  libxlDomainDetachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
 
                 l_disk = vm->def->disks[idx];
 
-                if (libxlMakeDisk(l_disk, &x_disk) < 0)
+                if (libxlMakeDisk(conn, l_disk, &x_disk) < 0)
                     goto cleanup;
 
                 if ((ret = libxl_device_disk_remove(cfg->ctx, vm->def->id,
@@ -3180,6 +3186,7 @@  libxlDomainDetachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
 
 static int
 libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
+                           virConnectPtr conn,
                            virDomainObjPtr vm,
                            virDomainNetDefPtr net)
 {
@@ -3221,7 +3228,7 @@  libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
     }
 
     libxl_device_nic_init(&nic);
-    if (libxlMakeNic(vm->def, net, &nic) < 0)
+    if (libxlMakeNic(conn, vm->def, net, &nic) < 0)
         goto cleanup;
 
     if (libxl_device_nic_add(cfg->ctx, vm->def->id, &nic, 0)) {
@@ -3243,6 +3250,7 @@  libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
 
 static int
 libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
+                            virConnectPtr conn,
                             virDomainObjPtr vm,
                             virDomainDeviceDefPtr dev)
 {
@@ -3250,13 +3258,13 @@  libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
 
     switch (dev->type) {
         case VIR_DOMAIN_DEVICE_DISK:
-            ret = libxlDomainAttachDeviceDiskLive(vm, dev);
+            ret = libxlDomainAttachDeviceDiskLive(conn, vm, dev);
             if (!ret)
                 dev->data.disk = NULL;
             break;
 
         case VIR_DOMAIN_DEVICE_NET:
-            ret = libxlDomainAttachNetDevice(driver, vm,
+            ret = libxlDomainAttachNetDevice(driver, conn, vm,
                                              dev->data.net);
             if (!ret)
                 dev->data.net = NULL;
@@ -3514,6 +3522,7 @@  libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
 
 static int
 libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
+                            virConnectPtr conn,
                             virDomainObjPtr vm,
                             virDomainDeviceDefPtr dev)
 {
@@ -3521,7 +3530,7 @@  libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
 
     switch (dev->type) {
         case VIR_DOMAIN_DEVICE_DISK:
-            ret = libxlDomainDetachDeviceDiskLive(vm, dev);
+            ret = libxlDomainDetachDeviceDiskLive(conn, vm, dev);
             break;
 
         case VIR_DOMAIN_DEVICE_NET:
@@ -3595,7 +3604,9 @@  libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
 }
 
 static int
-libxlDomainUpdateDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
+libxlDomainUpdateDeviceLive(virConnectPtr conn,
+                            virDomainObjPtr vm,
+                            virDomainDeviceDefPtr dev)
 {
     virDomainDiskDefPtr disk;
     int ret = -1;
@@ -3605,7 +3616,7 @@  libxlDomainUpdateDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev)
             disk = dev->data.disk;
             switch (disk->device) {
                 case VIR_DOMAIN_DISK_DEVICE_CDROM:
-                    ret = libxlDomainChangeEjectableMedia(vm, disk);
+                    ret = libxlDomainChangeEjectableMedia(conn, vm, disk);
                     if (ret == 0)
                         dev->data.disk = NULL;
                     break;
@@ -3733,7 +3744,7 @@  libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
                                             VIR_DOMAIN_DEF_PARSE_INACTIVE)))
             goto endjob;
 
-        if (libxlDomainAttachDeviceLive(driver, vm, dev) < 0)
+        if (libxlDomainAttachDeviceLive(driver, dom->conn, vm, dev) < 0)
             goto endjob;
 
         /*
@@ -3841,7 +3852,7 @@  libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
                                             VIR_DOMAIN_DEF_PARSE_INACTIVE)))
             goto endjob;
 
-        if (libxlDomainDetachDeviceLive(driver, vm, dev) < 0)
+        if (libxlDomainDetachDeviceLive(driver, dom->conn, vm, dev) < 0)
             goto endjob;
 
         /*
@@ -3948,7 +3959,7 @@  libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
                                             VIR_DOMAIN_DEF_PARSE_INACTIVE)))
             goto cleanup;
 
-        if ((ret = libxlDomainUpdateDeviceLive(vm, dev)) < 0)
+        if ((ret = libxlDomainUpdateDeviceLive(dom->conn, vm, dev)) < 0)
             goto cleanup;
 
         /*
-- 
2.6.1