From patchwork Wed Feb 24 04:11:24 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jim Fehlig X-Patchwork-Id: 8399261 Return-Path: X-Original-To: patchwork-xen-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 646AF9F372 for ; Wed, 24 Feb 2016 04:15:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DFA1D202E6 for ; Wed, 24 Feb 2016 04:14:58 +0000 (UTC) Received: from lists.xen.org (lists.xenproject.org [50.57.142.19]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E402D2035B for ; Wed, 24 Feb 2016 04:14:55 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xen.org) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aYQnZ-0007sB-98; Wed, 24 Feb 2016 04:11:37 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aYQnX-0007s6-Ic for xen-devel@lists.xen.org; Wed, 24 Feb 2016 04:11:35 +0000 Received: from [85.158.139.211] by server-2.bemta-5.messagelabs.com id 6E/D0-03303-67D2DC65; Wed, 24 Feb 2016 04:11:34 +0000 X-Env-Sender: jfehlig@suse.com X-Msg-Ref: server-13.tower-206.messagelabs.com!1456287091!24265572!1 X-Originating-IP: [137.65.250.81] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 7.35.1; banners=-,-,- X-VirusChecked: Checked Received: (qmail 18749 invoked from network); 24 Feb 2016 04:11:33 -0000 Received: from smtp2.provo.novell.com (HELO smtp2.provo.novell.com) (137.65.250.81) by server-13.tower-206.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 24 Feb 2016 04:11:33 -0000 Received: from [192.168.0.28] (prv-ext-foundry1int.gns.novell.com [137.65.251.240]) by smtp2.provo.novell.com with ESMTP (TLS encrypted); Tue, 23 Feb 2016 21:11:26 -0700 Message-ID: <56CD2D6C.7060608@suse.com> Date: Tue, 23 Feb 2016 21:11:24 -0700 From: Jim Fehlig User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: =?windows-1252?Q?J=E1n_Tomko?= References: <1455755625-13329-1-git-send-email-jfehlig@suse.com> <1455755625-13329-5-git-send-email-jfehlig@suse.com> <20160222143523.GF2228@dnr.brq.redhat.com> In-Reply-To: <20160222143523.GF2228@dnr.brq.redhat.com> Cc: libvirt-list@redhat.com, xen-devel@lists.xen.org Subject: Re: [Xen-devel] [libvirt] [PATCH V2 4/4] libxl: add support for rbd qdisk X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, MIME_QP_LONG_LINE, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 >> 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 >> --- >> 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 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 --- 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