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@xxxxxxxx>
>> ---
>> 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
>From 869c76c029390ebb6e2b89b3bb0ccf9ac4610806 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@xxxxxxxx>
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@xxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list