9:47 p.m.
Updated against the latest git tree, no major changes since a week ago.
Thank you for reviews in previous ones.
Purpose of patches:
Now, virsh at(de)tach-device/disk/...etc.. doesn't support to update
inactive domain's definition even with the --persistent flag.
To update persistent modification of inactive domains, we have to use
virsh edit.
So, if we want to update inactive domain definition with scripts, we need to
use other command/libs than libvirt. I'd like to use libvirt/virsh in scripts.
This patches adds to support for updating domain definition via virsh with
scripts. This series just includes 'disk' updates but I'll add more.
Thanks,
-Kame
9:49 p.m.
New subject: [libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
at(de)tatch-device --persistent cannot modify qemu config.
(Xen allows it.)
This patch is a base patch for adding support of devices in
step by step manner. Following patches will add some device
support.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Changelog: v8->v9
- removed unnecessary comments.
---
src/qemu/qemu_driver.c | 134 +++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 120 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 04a5f65..49af487 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3918,16 +3918,117 @@ cleanup:
return ret;
}
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
- const char *xml,
- unsigned int flags) {
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
+static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr newdev)
+{
+
+ switch(newdev->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("the device is not supported for now"));
return -1;
}
- return qemudDomainAttachDevice(dom, xml);
+ return 0;
+}
+
+static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr device)
+{
+ switch(device->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("the device is not supported for now"));
+ return -1;
+ }
+ return 0;
+}
+
+static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
+ const char *xml,
+ unsigned int attach,
+ unsigned int flags)
+{
+ struct qemud_driver *driver;
+ virDomainDeviceDefPtr device;
+ virDomainDefPtr vmdef;
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ /*
+ * When both of MODIFY_CONFIG and MODIFY_LIVE are passed at the same time,
+ * return error for now. We should support this later.
+ */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify active domain and its definition "
+ "at the same time."));
+ return -1;
+ }
+
+ driver = dom->conn->privateData;
+ qemuDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, dom->name);
+ if (!vm) {
+ qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name :
'%s'"),
+ dom->name);
+ goto unlock_out;
+ }
+
+ if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+ goto unlock_out;
+
+ if (virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify active domain's definition"));
+ goto endjob;
+ }
+
+ vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
+
+ if (!vmdef)
+ goto endjob;
+
+ device = virDomainDeviceDefParse(driver->caps,
+ vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
+ if (!device)
+ goto endjob;
+
+ if (attach)
+ ret = qemuDomainAttachDevicePersistent(vmdef, device);
+ else
+ ret = qemuDomainDetachDevicePersistent(vmdef, device);
+
+ if (!ret)
+ ret = virDomainSaveConfig(driver->configDir, vmdef);
+
+ virDomainDeviceDefFree(device);
+
+endjob:
+ if (qemuDomainObjEndJob(vm) == 0)
+ vm = NULL;
+unlock_out:
+ if (vm)
+ virDomainObjUnlock(vm);
+ qemuDriverUnlock(driver);
+ return ret;
+}
+
+static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ int ret = -1;
+
+ virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ ret = qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
+ else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ ret = qemudDomainAttachDevice(dom, xml);
+
+ return ret;
}
@@ -4141,14 +4242,19 @@ cleanup:
static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
const char *xml,
- unsigned int flags) {
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
+ unsigned int flags)
+{
+ int ret = -1;
+
+ virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
- return qemudDomainDetachDevice(dom, xml);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ ret = qemuDomainModifyDevicePersistent(dom, xml, 0, flags);
+ else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ ret = qemudDomainDetachDevice(dom, xml);
+
+ return ret;
}
static int qemudDomainGetAutostart(virDomainPtr dom,
--
1.7.4.1
4:25 p.m.
New subject: [libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.
On 04/12/2011 08:49 PM, KAMEZAWA Hiroyuki wrote:
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
at(de)tatch-device --persistent cannot modify qemu config.
(Xen allows it.)
This patch is a base patch for adding support of devices in
step by step manner. Following patches will add some device
support.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Changelog: v8->v9
- removed unnecessary comments.
It's taken quite a few iterations, but I've finally scheduled time to
start reviewing it.
+static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
+ const char *xml,
+ unsigned int attach,
You missed one. You changed attach and detach, but not update. This
parameter can usefully forward to all three types of updates.
In fact, I think I'd like to go one further, and refactor things further
before we start adding persistent devices.
It seems like there are two steps in all three categories of functions -
obtain the locks, then do the work. I like how you've factored things
into one function that obtains the locks then forwards on to other
functions that do the work. Furthermore, I don't see any reason why we
can't support LIVE|CONFIG at once, provided we know for which devices we
can make a successful CONFIG change.
+ * When both of MODIFY_CONFIG and MODIFY_LIVE are passed at the
same time,
+ * return error for now. We should support this later.
+ */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify active domain and its definition "
+ "at the same time."));
+ return -1;
+ }
You are right that it's not implemented yet, but no worse than what we
already have, so it's not a regression. But I think it is doable, by
rewriting this into one giant function:
1. obtain lock
2. if CURRENT, convert to LIVE or CONFIG
3. if LIVE but not active, error
4. if CONFIG but not persistent, error
5. if CONFIG call, make temporary vmdef and modify that, or quit if that
device is not supported yet
6. if LIVE, make live modification; if that errors out, quit
7. if CONFIG, make temporary vmdef permanent (hopefully it succeeds,
because we don't roll back the LIVE portion of LIVE|CONFIG)
8. clean up locks
I'll propose a followup patch that tries to capture this idiom in code.
+static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ int ret = -1;
+
+ virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
This causes a change in behavior. Previously, we silently ignored
VIR_DOMAIN_DEVICE_MODIFY_FORCE, now we error out. But overall, that's a
good change (FORCE only made sense for ModifyDevice, not Attach), and it
goes to show that we don't use virCheckFlags on nearly enough APIs.
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ ret = qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
+ else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ ret = qemudDomainAttachDevice(dom, xml);
For example, here your logic is wrong. You covered the CONFIG|LIVE case
with qemuDomainModifyDevicePersistent (by erroring out), but you _don't_
cover the CURRENT case (which should be either CONFIG or LIVE). But to
learn if the vm is active, you have to obtain the lock, and the way
you've written this, you've already forwarded into one of the two
routines. Rather, all the public entries should forward into the one
giant helper routine which grabs the locks, checks the flags, then calls
into the right callbacks.
@@ -4141,14 +4242,19 @@ cleanup:
static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
const char *xml,
- unsigned int flags) {
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
+ unsigned int flags)
+{
+ int ret = -1;
+
+ virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
Another change in noisily rejecting FORCE, but I think it's good.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:28 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: implement older APIs by forwarding to flags version
Centralize device modification in the more flexible APIs, to allow
future honoring of additional flags. Explicitly reject the
VIR_DOMAIN_DEVICE_MODIFY_FORCE flag on attach/detach.
* src/qemu/qemu_driver.c (qemudDomainAttachDevice)
(qemudDomainAttachDeviceFlags): Swap bodies, and rename...
(qemuDomainAttachDevice, qemuDomainAttachDeviceFlags): to this.
(qemudDomainDetachDevice, qemudDomainDetachDeviceFlags):
Likewise.
---
src/qemu/qemu_driver.c | 72 +++++++++++++++++++++++++++--------------------
1 files changed, 41 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c1a44c9..ca06dd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3776,8 +3776,9 @@ cleanup:
}
-static int qemudDomainAttachDevice(virDomainPtr dom,
- const char *xml)
+static int
+qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
@@ -3786,6 +3787,14 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
virCgroupPtr cgroup = NULL;
int ret = -1;
+ virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot modify the persistent
configuration of a domain"));
+ return -1;
+ }
+
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@@ -3933,22 +3942,17 @@ cleanup:
return ret;
}
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
- const char *xml,
- unsigned int flags) {
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
-
- return qemudDomainAttachDevice(dom, xml);
+static int
+qemuDomainAttachDevice(virDomainPtr dom, const char *xml)
+{
+ return qemuDomainAttachDeviceFlags(dom, xml,
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE);
}
-static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
- const char *xml,
- unsigned int flags)
+static int
+qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
@@ -4068,14 +4072,25 @@ cleanup:
}
-static int qemudDomainDetachDevice(virDomainPtr dom,
- const char *xml) {
+static int
+qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags)
+{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
virBitmapPtr qemuCaps = NULL;
virDomainDeviceDefPtr dev = NULL;
int ret = -1;
+ virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot modify the persistent
configuration of a domain"));
+ return -1;
+ }
+
+
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@@ -4154,16 +4169,11 @@ cleanup:
return ret;
}
-static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
- const char *xml,
- unsigned int flags) {
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
-
- return qemudDomainDetachDevice(dom, xml);
+static int
+qemuDomainDetachDevice(virDomainPtr dom, const char *xml)
+{
+ return qemuDomainDetachDeviceFlags(dom, xml,
+ VIR_DOMAIN_DEVICE_MODIFY_LIVE);
}
static int qemudDomainGetAutostart(virDomainPtr dom,
@@ -6965,10 +6975,10 @@ static virDriver qemuDriver = {
qemudDomainStartWithFlags, /* domainCreateWithFlags */
qemudDomainDefine, /* domainDefineXML */
qemudDomainUndefine, /* domainUndefine */
- qemudDomainAttachDevice, /* domainAttachDevice */
- qemudDomainAttachDeviceFlags, /* domainAttachDeviceFlags */
- qemudDomainDetachDevice, /* domainDetachDevice */
- qemudDomainDetachDeviceFlags, /* domainDetachDeviceFlags */
+ qemuDomainAttachDevice, /* domainAttachDevice */
+ qemuDomainAttachDeviceFlags, /* domainAttachDeviceFlags */
+ qemuDomainDetachDevice, /* domainDetachDevice */
+ qemuDomainDetachDeviceFlags, /* domainDetachDeviceFlags */
qemuDomainUpdateDeviceFlags, /* domainUpdateDeviceFlags */
qemudDomainGetAutostart, /* domainGetAutostart */
qemudDomainSetAutostart, /* domainSetAutostart */
--
1.7.4.2
10:28 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: refactor device modification functions
The device modify functions (Attach, Update, Detach) are rather
long, with boilerplate to obtain locks surrounding a case statement
for acting on particular device types. In order for a future patch
to share the boilerplate and allow persistent modification, factor
out the device actions.
* src/qemu/qemu_driver.c (qemuDomainAttachDeviceLive)
(qemuDomainUpdateDeviceLive, qemuDomainDetachDeviceLive): New
helper methods.
(qemuDomainAttachDeviceFlags, qemuDomainUpdateDeviceFlags)
(qemuDomainDetachDeviceFlags): Use it to separate case statements
from locking setup.
---
src/qemu/qemu_driver.c | 385 ++++++++++++++++++++++++++++--------------------
1 files changed, 225 insertions(+), 160 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ca06dd6..c1a5ebb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3776,72 +3776,36 @@ cleanup:
}
+/* Helper called on active vm while job condition is held */
static int
-qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
- unsigned int flags)
+qemuDomainAttachDeviceLive(virDomainPtr dom, struct qemud_driver *driver,
+ virDomainObjPtr vm, virDomainDeviceDefPtr dev,
+ virBitmapPtr qemuCaps,
+ bool force ATTRIBUTE_UNUSED)
{
- struct qemud_driver *driver = dom->conn->privateData;
- virDomainObjPtr vm;
- virDomainDeviceDefPtr dev = NULL;
- virBitmapPtr qemuCaps = NULL;
virCgroupPtr cgroup = NULL;
int ret = -1;
- virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
-
- qemuDriverLock(driver);
- vm = virDomainFindByUUID(&driver->domains, dom->uuid);
- if (!vm) {
- char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(dom->uuid, uuidstr);
- qemuReportError(VIR_ERR_NO_DOMAIN,
- _("no domain with matching uuid '%s'"),
uuidstr);
- goto cleanup;
- }
-
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot attach device on inactive
domain"));
- goto endjob;
- }
-
- dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
- VIR_DOMAIN_XML_INACTIVE);
- if (dev == NULL)
- goto endjob;
-
- if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch,
- NULL,
- &qemuCaps) < 0)
- goto endjob;
-
- if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
if (dev->data.disk->driverName != NULL &&
!STREQ(dev->data.disk->driverName, "qemu")) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_("unsupported driver name '%s' for disk
'%s'"),
dev->data.disk->driverName,
dev->data.disk->src);
- goto endjob;
+ goto cleanup;
}
if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
- if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup,
0) !=0 ) {
+ if (virCgroupForDomain(driver->cgroup, vm->def->name,
+ &cgroup, 0) !=0 ) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to find cgroup for %s"),
vm->def->name);
- goto endjob;
+ goto cleanup;
}
if (qemuSetupDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
- goto endjob;
+ goto cleanup;
}
switch (dev->data.disk->device) {
@@ -3856,69 +3820,256 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
- if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+ switch (dev->data.disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_USB:
ret = qemuDomainAttachUsbMassstorageDevice(driver, vm,
- dev->data.disk, qemuCaps);
- if (ret == 0)
- dev->data.disk = NULL;
- } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
+ dev->data.disk,
+ qemuCaps);
+ break;
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
ret = qemuDomainAttachPciDiskDevice(driver, vm,
dev->data.disk, qemuCaps);
- if (ret == 0)
- dev->data.disk = NULL;
- } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
ret = qemuDomainAttachSCSIDisk(driver, vm,
dev->data.disk, qemuCaps);
- if (ret == 0)
- dev->data.disk = NULL;
- } else {
+ break;
+ default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk bus '%s' cannot be
hotplugged."),
virDomainDiskBusTypeToString(dev->data.disk->bus));
- /* fallthrough */
+ goto cleanup;
}
+ if (ret == 0)
+ dev->data.disk = NULL;
break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk device type '%s' cannot be
hotplugged"),
virDomainDiskDeviceTypeToString(dev->data.disk->device));
- /* Fallthrough */
- }
- if (ret != 0 && cgroup) {
- if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
- VIR_WARN("Failed to teardown cgroup for disk path %s",
- NULLSTR(dev->data.disk->src));
+ goto cleanup;
}
- } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
ret = qemuDomainAttachPciControllerDevice(driver, vm,
- dev->data.controller,
qemuCaps);
+ dev->data.controller,
+ qemuCaps);
if (ret == 0)
dev->data.controller = NULL;
} else {
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk controller bus '%s' cannot be
hotplugged."),
virDomainControllerTypeToString(dev->data.controller->type));
- /* fallthrough */
+ goto cleanup;
}
- } else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
ret = qemuDomainAttachNetDevice(dom->conn, driver, vm,
dev->data.net, qemuCaps);
if (ret == 0)
dev->data.net = NULL;
- } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
ret = qemuDomainAttachHostDevice(driver, vm,
dev->data.hostdev, qemuCaps);
if (ret == 0)
dev->data.hostdev = NULL;
- } else {
+ break;
+ default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("device type '%s' cannot be attached"),
virDomainDeviceTypeToString(dev->type));
+ goto cleanup;
+ }
+
+cleanup:
+ if (ret != 0 && cgroup) {
+ if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
+ VIR_WARN("Failed to teardown cgroup for disk path %s",
+ NULLSTR(dev->data.disk->src));
+ }
+ if (cgroup)
+ virCgroupFree(&cgroup);
+ return ret;
+}
+
+/* Helper called on active vm while job condition is held */
+static int
+qemuDomainUpdateDeviceLive(virDomainPtr dom ATTRIBUTE_UNUSED,
+ struct qemud_driver *driver,
+ virDomainObjPtr vm, virDomainDeviceDefPtr dev,
+ virBitmapPtr qemuCaps, bool force)
+{
+ virCgroupPtr cgroup = NULL;
+ int ret = -1;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
+ if (virCgroupForDomain(driver->cgroup, vm->def->name,
+ &cgroup, 0) !=0 ) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find cgroup for %s"),
+ vm->def->name);
+ goto cleanup;
+ }
+ if (qemuSetupDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
+ goto cleanup;
+ }
+
+ switch (dev->data.disk->device) {
+ case VIR_DOMAIN_DISK_DEVICE_CDROM:
+ case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
+ ret = qemuDomainChangeEjectableMedia(driver, vm,
+ dev->data.disk,
+ qemuCaps,
+ force);
+ if (ret == 0)
+ dev->data.disk = NULL;
+ break;
+
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk bus '%s' cannot be updated."),
+ virDomainDiskBusTypeToString(dev->data.disk->bus));
+ goto cleanup;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics);
+ break;
+
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("device type '%s' cannot be updated"),
+ virDomainDeviceTypeToString(dev->type));
+ goto cleanup;
+ }
+
+cleanup:
+ if (ret != 0 && cgroup) {
+ if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
+ VIR_WARN("Failed to teardown cgroup for disk path %s",
+ NULLSTR(dev->data.disk->src));
+ }
+ if (cgroup)
+ virCgroupFree(&cgroup);
+ return ret;
+}
+
+/* Helper called on active vm while job condition is held */
+static int
+qemuDomainDetachDeviceLive(virDomainPtr dom ATTRIBUTE_UNUSED,
+ struct qemud_driver *driver,
+ virDomainObjPtr vm, virDomainDeviceDefPtr dev,
+ virBitmapPtr qemuCaps,
+ bool force ATTRIBUTE_UNUSED)
+{
+ int ret = -1;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ if (dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+ switch (dev->data.disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ ret = qemuDomainDetachPciDiskDevice(driver, vm, dev, qemuCaps);
+ break;
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps);
+ break;
+ case VIR_DOMAIN_DISK_BUS_USB:
+ ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps);
+ break;
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This type of disk cannot be hot
unplugged"));
+ goto cleanup;
+ }
+ } else {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This type of disk cannot be hot unplugged"));
+ goto cleanup;
+ }
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ ret = qemuDomainDetachNetDevice(driver, vm, dev, qemuCaps);
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ ret = qemuDomainDetachPciControllerDevice(driver, vm, dev,
+ qemuCaps);
+ } else {
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk controller bus '%s' cannot be
hotunplugged."),
+
virDomainControllerTypeToString(dev->data.controller->type));
+ goto cleanup;
+ }
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ ret = qemuDomainDetachHostDevice(driver, vm, dev, qemuCaps);
+ break;
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("This type of device cannot be hot
unplugged"));
+ goto cleanup;
+ }
+
+cleanup:
+ return ret;
+}
+
+static int
+qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags)
+{
+ struct qemud_driver *driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ virDomainDeviceDefPtr dev = NULL;
+ virBitmapPtr qemuCaps = NULL;
+ int ret = -1;
+
+ virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot modify the persistent
configuration of a domain"));
+ return -1;
+ }
+
+ qemuDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ qemuReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot attach device on inactive
domain"));
goto endjob;
}
+ dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
+ VIR_DOMAIN_XML_INACTIVE);
+ if (dev == NULL)
+ goto endjob;
+
+ if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch,
+ NULL,
+ &qemuCaps) < 0)
+ goto endjob;
+
+ ret = qemuDomainAttachDeviceLive(dom, driver, vm, dev, qemuCaps, false);
+
/* update domain status forcibly because the domain status may be changed
* even if we attach the device failed. For example, a new controller may
* be created.
@@ -3931,9 +4082,6 @@ endjob:
vm = NULL;
cleanup:
- if (cgroup)
- virCgroupFree(&cgroup);
-
qemuCapsFree(qemuCaps);
virDomainDeviceDefFree(dev);
if (vm)
@@ -3958,7 +4106,6 @@ qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
virDomainObjPtr vm;
virDomainDeviceDefPtr dev = NULL;
virBitmapPtr qemuCaps = NULL;
- virCgroupPtr cgroup = NULL;
int ret = -1;
bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
@@ -4002,55 +4149,7 @@ qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
&qemuCaps) < 0)
goto endjob;
- switch (dev->type) {
- case VIR_DOMAIN_DEVICE_DISK:
- if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
- if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup,
0) !=0 ) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to find cgroup for %s"),
- vm->def->name);
- goto endjob;
- }
- if (qemuSetupDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
- goto endjob;
- }
-
- switch (dev->data.disk->device) {
- case VIR_DOMAIN_DISK_DEVICE_CDROM:
- case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- ret = qemuDomainChangeEjectableMedia(driver, vm,
- dev->data.disk,
- qemuCaps,
- force);
- if (ret == 0)
- dev->data.disk = NULL;
- break;
-
-
- default:
- qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk bus '%s' cannot be updated."),
- virDomainDiskBusTypeToString(dev->data.disk->bus));
- break;
- }
-
- if (ret != 0 && cgroup) {
- if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0)
- VIR_WARN("Failed to teardown cgroup for disk path %s",
- NULLSTR(dev->data.disk->src));
- }
- break;
-
- case VIR_DOMAIN_DEVICE_GRAPHICS:
- ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics);
- break;
-
- default:
- qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("device type '%s' cannot be updated"),
- virDomainDeviceTypeToString(dev->type));
- break;
- }
+ ret = qemuDomainUpdateDeviceLive(dom, driver, vm, dev, qemuCaps, force);
if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm)
< 0)
ret = -1;
@@ -4060,9 +4159,6 @@ endjob:
vm = NULL;
cleanup:
- if (cgroup)
- virCgroupFree(&cgroup);
-
qemuCapsFree(qemuCaps);
virDomainDeviceDefFree(dev);
if (vm)
@@ -4120,38 +4216,7 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
&qemuCaps) < 0)
goto endjob;
- if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
- dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
- if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
- ret = qemuDomainDetachPciDiskDevice(driver, vm, dev, qemuCaps);
- }
- else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
- ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps);
- } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
- ret = qemuDomainDetachDiskDevice(driver, vm, dev, qemuCaps);
- }
- else {
- qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("This type of disk cannot be hot unplugged"));
- }
- } else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
- ret = qemuDomainDetachNetDevice(driver, vm, dev, qemuCaps);
- } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
- if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
- ret = qemuDomainDetachPciControllerDevice(driver, vm, dev,
- qemuCaps);
- } else {
- qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk controller bus '%s' cannot be
hotunplugged."),
-
virDomainControllerTypeToString(dev->data.controller->type));
- /* fallthrough */
- }
- } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
- ret = qemuDomainDetachHostDevice(driver, vm, dev, qemuCaps);
- } else {
- qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("This type of device cannot be hot
unplugged"));
- }
+ ret = qemuDomainDetachDeviceLive(dom, driver, vm, dev, qemuCaps, false);
if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm)
< 0)
ret = -1;
--
1.7.4.2
10:28 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: consolidate device modification functions
No functional change, but now adding support for persistent
modifications can be done at one point to affect 3 APIs.
* src/qemu/qemu_driver.c (qemuDomainModifyDeviceCallback): New
typedef.
(qemuDomainModifyDeviceFlags) New function.
(qemuDomainAttachDeviceFlags, qemuDomainUpdateDeviceFlags)
(qemuDomainDetachDeviceFlags): Use it.
---
src/qemu/qemu_driver.c | 158 +++++++++--------------------------------------
1 files changed, 31 insertions(+), 127 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c1a5ebb..4f0a057 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3775,6 +3775,12 @@ cleanup:
return ret;
}
+typedef int (*qemuDomainModifyDeviceCallback) (virDomainPtr dom,
+ struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ virBitmapPtr qemuCaps,
+ bool force);
/* Helper called on active vm while job condition is held */
static int
@@ -4021,21 +4027,22 @@ cleanup:
return ret;
}
+/* Common boilerplate around modifying a device. */
static int
-qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
- unsigned int flags)
+qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags,
+ qemuDomainModifyDeviceCallback cb)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
virDomainDeviceDefPtr dev = NULL;
virBitmapPtr qemuCaps = NULL;
int ret = -1;
+ bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
- virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify domain persistent configuration"));
return -1;
}
@@ -4054,7 +4061,7 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
if (!virDomainObjIsActive(vm)) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot attach device on inactive
domain"));
+ "%s", _("cannot modify device on inactive
domain"));
goto endjob;
}
@@ -4068,7 +4075,7 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
&qemuCaps) < 0)
goto endjob;
- ret = qemuDomainAttachDeviceLive(dom, driver, vm, dev, qemuCaps, false);
+ ret = (cb)(dom, driver, vm, dev, qemuCaps, force);
/* update domain status forcibly because the domain status may be changed
* even if we attach the device failed. For example, a new controller may
@@ -4090,6 +4097,17 @@ cleanup:
return ret;
}
+
+static int
+qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
+ unsigned int flags)
+{
+ virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
+ return qemuDomainModifyDeviceFlags(dom, xml, flags,
+ qemuDomainAttachDeviceLive);
+}
+
static int
qemuDomainAttachDevice(virDomainPtr dom, const char *xml)
{
@@ -4102,69 +4120,11 @@ static int
qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
unsigned int flags)
{
- struct qemud_driver *driver = dom->conn->privateData;
- virDomainObjPtr vm;
- virDomainDeviceDefPtr dev = NULL;
- virBitmapPtr qemuCaps = NULL;
- int ret = -1;
- bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
-
- virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT |
- VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
-
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
-
- qemuDriverLock(driver);
- vm = virDomainFindByUUID(&driver->domains, dom->uuid);
- if (!vm) {
- char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(dom->uuid, uuidstr);
- qemuReportError(VIR_ERR_NO_DOMAIN,
- _("no domain with matching uuid '%s'"),
uuidstr);
- goto cleanup;
- }
-
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot attach device on inactive
domain"));
- goto endjob;
- }
-
- dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
- VIR_DOMAIN_XML_INACTIVE);
- if (dev == NULL)
- goto endjob;
-
- if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch,
- NULL,
- &qemuCaps) < 0)
- goto endjob;
-
- ret = qemuDomainUpdateDeviceLive(dom, driver, vm, dev, qemuCaps, force);
-
- if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm)
< 0)
- ret = -1;
-
-endjob:
- if (qemuDomainObjEndJob(vm) == 0)
- vm = NULL;
-
-cleanup:
- qemuCapsFree(qemuCaps);
- virDomainDeviceDefFree(dev);
- if (vm)
- virDomainObjUnlock(vm);
- qemuDriverUnlock(driver);
- return ret;
+ return qemuDomainModifyDeviceFlags(dom, xml, flags,
+ qemuDomainUpdateDeviceLive);
}
@@ -4172,66 +4132,10 @@ static int
qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
unsigned int flags)
{
- struct qemud_driver *driver = dom->conn->privateData;
- virDomainObjPtr vm;
- virBitmapPtr qemuCaps = NULL;
- virDomainDeviceDefPtr dev = NULL;
- int ret = -1;
-
virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot modify the persistent
configuration of a domain"));
- return -1;
- }
-
-
- qemuDriverLock(driver);
- vm = virDomainFindByUUID(&driver->domains, dom->uuid);
- if (!vm) {
- char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(dom->uuid, uuidstr);
- qemuReportError(VIR_ERR_NO_DOMAIN,
- _("no domain with matching uuid '%s'"),
uuidstr);
- goto cleanup;
- }
-
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cannot detach device on inactive
domain"));
- goto endjob;
- }
-
- dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
- VIR_DOMAIN_XML_INACTIVE);
- if (dev == NULL)
- goto endjob;
-
- if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch,
- NULL,
- &qemuCaps) < 0)
- goto endjob;
-
- ret = qemuDomainDetachDeviceLive(dom, driver, vm, dev, qemuCaps, false);
-
- if (!ret && virDomainSaveStatus(driver->caps, driver->stateDir, vm)
< 0)
- ret = -1;
-
-endjob:
- if (qemuDomainObjEndJob(vm) == 0)
- vm = NULL;
-
-cleanup:
- qemuCapsFree(qemuCaps);
- virDomainDeviceDefFree(dev);
- if (vm)
- virDomainObjUnlock(vm);
- qemuDriverUnlock(driver);
- return ret;
+ return qemuDomainModifyDeviceFlags(dom, xml, flags,
+ qemuDomainDetachDeviceLive);
}
static int
--
1.7.4.2
10:28 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: add plumbing for persistent device changes
There's still work to add persistent callback functions, and to
make sure this all works, but this demonstrates how having a
single function makes it easy to support flags for all three
types of device modifications.
* src/qemu/qemu_driver.c (qemuDomainModifyDeviceFlags): Add
parameter, and support VIR_DOMAIN_DEVICE_MODIFY_CURRENT.
(qemuDomainAttachDeviceFlags, qemuDomainUpdateDeviceFlags)
(qemuDomainDetachDeviceFlags): Update callers.
---
After this point, we can use Kame's patch 1/4 to add in the
persistent function callbacks (with slight tweaks to their
signatures), and 2/4 to make it easier to to temporary
modifications to a config:
if (flags & CONFIG) {
create temporary def
call persistent callback
}
if (flags & LIVE) {
call live callback
}
if (no errors)
commit temporary def and live state changes, as needed
But with the benefit that CURRENT support is now provided.
src/qemu/qemu_driver.c | 47 +++++++++++++++++++++++++++++++++++------------
1 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4f0a057..8c978be 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4031,7 +4031,8 @@ cleanup:
static int
qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
unsigned int flags,
- qemuDomainModifyDeviceCallback cb)
+ qemuDomainModifyDeviceCallback live,
+ qemuDomainModifyDeviceCallback persistent)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
@@ -4039,12 +4040,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
virBitmapPtr qemuCaps = NULL;
int ret = -1;
bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
-
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify domain persistent configuration"));
- return -1;
- }
+ bool active;
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -4059,12 +4055,32 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
+ active = virDomainObjIsActive(vm);
+
+ if ((flags & (VIR_DOMAIN_DEVICE_MODIFY_LIVE
+ | VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) == 0)
+ flags |= (active ? VIR_DOMAIN_DEVICE_MODIFY_LIVE
+ : VIR_DOMAIN_DEVICE_MODIFY_CONFIG);
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) && !active) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot modify device on inactive
domain"));
goto endjob;
}
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot modify device on transient
domain"));
+ goto endjob;
+ }
+
+ /* XXX add persistent support */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify domain persistent configuration"));
+ return -1;
+ }
+
dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE);
if (dev == NULL)
@@ -4075,7 +4091,11 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
&qemuCaps) < 0)
goto endjob;
- ret = (cb)(dom, driver, vm, dev, qemuCaps, force);
+ ret = 0;
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ ret = (live)(dom, driver, vm, dev, qemuCaps, force);
+ if (ret == 0 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+ ret = (persistent)(dom, driver, vm, dev, qemuCaps, force);
/* update domain status forcibly because the domain status may be changed
* even if we attach the device failed. For example, a new controller may
@@ -4105,7 +4125,8 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainAttachDeviceLive);
+ qemuDomainAttachDeviceLive,
+ NULL);
}
static int
@@ -4124,7 +4145,8 @@ qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainUpdateDeviceLive);
+ qemuDomainUpdateDeviceLive,
+ NULL);
}
@@ -4135,7 +4157,8 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainDetachDeviceLive);
+ qemuDomainDetachDeviceLive,
+ NULL);
}
static int
--
1.7.4.2
11:59 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: add plumbing for persistent device changes
At 04/16/2011 11:28 PM, Eric Blake Write:
There's still work to add persistent callback functions, and to
make sure this all works, but this demonstrates how having a
single function makes it easy to support flags for all three
types of device modifications.
* src/qemu/qemu_driver.c (qemuDomainModifyDeviceFlags): Add
parameter, and support VIR_DOMAIN_DEVICE_MODIFY_CURRENT.
(qemuDomainAttachDeviceFlags, qemuDomainUpdateDeviceFlags)
(qemuDomainDetachDeviceFlags): Update callers.
---
After this point, we can use Kame's patch 1/4 to add in the
persistent function callbacks (with slight tweaks to their
signatures), and 2/4 to make it easier to to temporary
modifications to a config:
if (flags & CONFIG) {
create temporary def
call persistent callback
}
if (flags & LIVE) {
call live callback
}
if (no errors)
commit temporary def and live state changes, as needed
But with the benefit that CURRENT support is now provided.
src/qemu/qemu_driver.c | 47 +++++++++++++++++++++++++++++++++++------------
1 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4f0a057..8c978be 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4031,7 +4031,8 @@ cleanup:
static int
qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
unsigned int flags,
- qemuDomainModifyDeviceCallback cb)
+ qemuDomainModifyDeviceCallback live,
+ qemuDomainModifyDeviceCallback persistent)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
@@ -4039,12 +4040,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
virBitmapPtr qemuCaps = NULL;
int ret = -1;
bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
-
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
- qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify domain persistent configuration"));
- return -1;
- }
+ bool active;
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -4059,12 +4055,32 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
goto cleanup;
- if (!virDomainObjIsActive(vm)) {
+ active = virDomainObjIsActive(vm);
+
+ if ((flags & (VIR_DOMAIN_DEVICE_MODIFY_LIVE
+ | VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) == 0)
+ flags |= (active ? VIR_DOMAIN_DEVICE_MODIFY_LIVE
+ : VIR_DOMAIN_DEVICE_MODIFY_CONFIG);
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) && !active) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot modify device on inactive
domain"));
goto endjob;
}
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot modify device on transient
domain"));
+ goto endjob;
+ }
+
+ /* XXX add persistent support */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
kamezawa-san is writing patch to support persistent device modification(attach/detach)
and Hu Tao is writing patch to support persistent device modification(update).
We can detect whether persistent device modification is supported like this:
if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !persistent) {
So the caller can pass NULL when persistent device modification is not supported safely.
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot modify domain persistent configuration"));
+ return -1;
We should goto endjob to do some cleanup.
+ }
+
dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE);
if (dev == NULL)
@@ -4075,7 +4091,11 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
&qemuCaps) < 0)
goto endjob;
- ret = (cb)(dom, driver, vm, dev, qemuCaps, force);
+ ret = 0;
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ ret = (live)(dom, driver, vm, dev, qemuCaps, force);
+ if (ret == 0 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+ ret = (persistent)(dom, driver, vm, dev, qemuCaps, force);
/* update domain status forcibly because the domain status may be changed
* even if we attach the device failed. For example, a new controller may
@@ -4105,7 +4125,8 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainAttachDeviceLive);
+ qemuDomainAttachDeviceLive,
+ NULL);
}
static int
@@ -4124,7 +4145,8 @@ qemuDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainUpdateDeviceLive);
+ qemuDomainUpdateDeviceLive,
+ NULL);
}
@@ -4135,7 +4157,8 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
return qemuDomainModifyDeviceFlags(dom, xml, flags,
- qemuDomainDetachDeviceLive);
+ qemuDomainDetachDeviceLive,
+ NULL);
}
static int
The other modification and the other 3 patches look good to me.
3:25 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: add plumbing for persistent device changes
On Mon, 18 Apr 2011 12:59:56 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
At 04/16/2011 11:28 PM, Eric Blake Write:
-
qemuDomainDetachDeviceLive);
> +
qemuDomainDetachDeviceLive,
> + NULL);
> }
>
> static int
The other modification and the other 3 patches look good to me.
I'll include Eric's series to the next v10 (with some modification).
Next one will includes all Attach/Detach/Update consolidation and persistent
modifications.
Thanks,
-Kame
6:52 p.m.
New subject: [libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.
On Fri, 15 Apr 2011 15:25:01 -0600
Eric Blake <eblake(a)redhat.com> wrote:
On 04/12/2011 08:49 PM, KAMEZAWA Hiroyuki wrote:
>
> Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
> doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
> at(de)tatch-device --persistent cannot modify qemu config.
> (Xen allows it.)
>
> This patch is a base patch for adding support of devices in
> step by step manner. Following patches will add some device
> support.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>
> Changelog: v8->v9
> - removed unnecessary comments.
It's taken quite a few iterations, but I've finally scheduled time to
start reviewing it.
Thank you.
> +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
> + const char *xml,
> + unsigned int attach,
You missed one. You changed attach and detach, but not update. This
parameter can usefully forward to all three types of updates.
In fact, I think I'd like to go one further, and refactor things further
before we start adding persistent devices.
Hmm.
It seems like there are two steps in all three categories of
functions -
obtain the locks, then do the work. I like how you've factored things
into one function that obtains the locks then forwards on to other
functions that do the work. Furthermore, I don't see any reason why we
can't support LIVE|CONFIG at once, provided we know for which devices we
can make a successful CONFIG change.
ok.
> + * When both of MODIFY_CONFIG and MODIFY_LIVE are passed at
the same time,
> + * return error for now. We should support this later.
> + */
> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot modify active domain and its definition
"
> + "at the same time."));
> + return -1;
> + }
You are right that it's not implemented yet, but no worse than what we
already have, so it's not a regression. But I think it is doable, by
rewriting this into one giant function:
1. obtain lock
2. if CURRENT, convert to LIVE or CONFIG
3. if LIVE but not active, error
4. if CONFIG but not persistent, error
5. if CONFIG call, make temporary vmdef and modify that, or quit if that
device is not supported yet
6. if LIVE, make live modification; if that errors out, quit
7. if CONFIG, make temporary vmdef permanent (hopefully it succeeds,
because we don't roll back the LIVE portion of LIVE|CONFIG)
8. clean up locks
Sure, I'll include that logic.
I'll propose a followup patch that tries to capture this idiom in
code.
> +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
> + const char *xml,
> + unsigned int flags)
> +{
> + int ret = -1;
> +
> + virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> + VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
This causes a change in behavior. Previously, we silently ignored
VIR_DOMAIN_DEVICE_MODIFY_FORCE, now we error out. But overall, that's a
good change (FORCE only made sense for ModifyDevice, not Attach), and it
goes to show that we don't use virCheckFlags on nearly enough APIs.
will fix.
> +
> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
> + ret = qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
> + else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
> + ret = qemudDomainAttachDevice(dom, xml);
For example, here your logic is wrong. You covered the CONFIG|LIVE case
with qemuDomainModifyDevicePersistent (by erroring out), but you _don't_
cover the CURRENT case (which should be either CONFIG or LIVE). But to
learn if the vm is active, you have to obtain the lock, and the way
you've written this, you've already forwarded into one of the two
routines. Rather, all the public entries should forward into the one
giant helper routine which grabs the locks, checks the flags, then calls
into the right callbacks.
> @@ -4141,14 +4242,19 @@ cleanup:
>
> static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
> const char *xml,
> - unsigned int flags) {
> - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> - qemuReportError(VIR_ERR_OPERATION_INVALID,
> - "%s", _("cannot modify the persistent
configuration of a domain"));
> - return -1;
> - }
> + unsigned int flags)
> +{
> + int ret = -1;
> +
> + virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> + VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
Another change in noisily rejecting FORCE, but I think it's good.
I'll learn your patch and merge it to mine. It seems longer way than I thought.
Thanks,
-Kame
9:50 p.m.
New subject: [libvirt] [PATCHv9 2/4] libvirt/qemu - keep consistency at persistent modification.
At persistent modification of inactive domains, we go several steps as
- insert disk to vmdef
- assign controller if necessary
- assign pci address if necessary
- save it to file
If failure happens in above sequence, we need to keep consistency between
vmdef on cache and XML in the file. This patch adds support for
consistent modification of vmdef.
This patch adds virDomainObjCopyPersistentDef(). This will create a
copy of persistent def. The caller can update this and later replace
current one as:
copy = virDomainObjCopyPersistentDef()
.....update....
if (error)
virDomainObjAssignDef(dom, copy);
else
virDomainDefFree(copy).
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/conf/domain_conf.c | 18 ++++++++++++++++++
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 13 +++++++++++--
4 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 90a1317..e644af0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9370,3 +9370,21 @@ cleanup:
return ret;
}
+
+virDomainDefPtr
+virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom)
+{
+ char *xml;
+ virDomainDefPtr cur, ret;
+
+ cur = virDomainObjGetPersistentDef(caps, dom);
+
+ xml = virDomainDefFormat(cur, VIR_DOMAIN_XML_WRITE_FLAGS);
+ if (!xml)
+ return NULL;
+
+ ret = virDomainDefParseString(caps, xml, VIR_DOMAIN_XML_READ_FLAGS);
+
+ VIR_FREE(xml);
+ return ret;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 95bd11e..9b97f26 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1377,6 +1377,9 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
virDomainDiskDefPathIterator iter,
void *opaque);
+virDomainDefPtr
+virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom);
+
typedef const char* (*virLifecycleToStringFunc)(int type);
typedef int (*virLifecycleFromStringFunc)(const char *type);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 54e4482..f464951 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -278,6 +278,7 @@ virDomainMemballoonModelTypeToString;
virDomainNetDefFree;
virDomainNetTypeToString;
virDomainObjAssignDef;
+virDomainObjCopyPersistentDef;
virDomainObjSetDefTransient;
virDomainObjGetPersistentDef;
virDomainObjIsDuplicate;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 49af487..b568382 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3983,8 +3983,11 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
_("cannot modify active domain's definition"));
goto endjob;
}
-
- vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
+ /*
+ * Here, create a copy of the current definition and update it.
+ * We'll finally replace the definition at success.
+ */
+ vmdef = virDomainObjCopyPersistentDef(driver->caps, vm);
if (!vmdef)
goto endjob;
@@ -4002,6 +4005,12 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
if (!ret)
ret = virDomainSaveConfig(driver->configDir, vmdef);
+ /* At success, replace it. this never fails. */
+ if (!ret)
+ virDomainObjAssignDef(vm, vmdef, false);
+ else /* At failure, discard copy. */
+ virDomainDefFree(vmdef);
+
virDomainDeviceDefFree(device);
endjob:
--
1.7.4.1
9:51 p.m.
New subject: [libvirt] [PATCHv9 3/4] libvirt/qemu - support persistent disk modification
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG
for qemu.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Changelog: v8->v9
updated comments
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 2 ++
src/qemu/qemu_driver.c | 30 ++++++++++++++++++++++++++++++
4 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e644af0..a39da7e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4879,6 +4879,19 @@ virVirtualPortProfileFormat(virBufferPtr buf,
virBufferVSprintf(buf, "%s</virtualport>\n", indent);
}
+int virDomainDiskIndexByName(virDomainDefPtr def, const char *name)
+{
+ virDomainDiskDefPtr vdisk;
+ int i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ vdisk = def->disks[i];
+ if (STREQ(vdisk->dst, name))
+ return i;
+ }
+ return -1;
+}
+
int virDomainDiskInsert(virDomainDefPtr def,
virDomainDiskDefPtr disk)
{
@@ -4950,6 +4963,15 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i)
}
}
+int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
+{
+ int i = virDomainDiskIndexByName(def, name);
+ if (i < 0)
+ return -1;
+ virDomainDiskRemove(def, i);
+ return 0;
+}
+
int virDomainControllerInsert(virDomainDefPtr def,
virDomainControllerDefPtr controller)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9b97f26..6b7cfe7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1282,6 +1282,7 @@ int virDomainVcpupinAdd(virDomainDefPtr def,
int maplen,
int vcpu);
+int virDomainDiskIndexByName(virDomainDefPtr def, const char *name);
int virDomainDiskInsert(virDomainDefPtr def,
virDomainDiskDefPtr disk);
void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
@@ -1289,6 +1290,7 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def);
void virDomainDiskRemove(virDomainDefPtr def, size_t i);
+int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
int virDomainControllerInsert(virDomainDefPtr def,
virDomainControllerDefPtr controller);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f464951..5c241aa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -245,11 +245,13 @@ virDomainDiskDefFree;
virDomainDiskDeviceTypeToString;
virDomainDiskErrorPolicyTypeFromString;
virDomainDiskErrorPolicyTypeToString;
+virDomainDiskIndexByName;
virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
virDomainDiskRemove;
+virDomainDiskRemoveByName;
virDomainDiskTypeFromString;
virDomainDiskTypeToString;
virDomainFSDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b568382..9c4f290 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3921,8 +3921,28 @@ cleanup:
static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
virDomainDeviceDefPtr newdev)
{
+ virDomainDiskDefPtr disk;
switch(newdev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ disk = newdev->data.disk;
+ if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("target %s already exists."), disk->dst);
+ return -1;
+ }
+ if (virDomainDiskInsert(vmdef, disk)) {
+ virReportOOMError();
+ return -1;
+ }
+ /* vmdef has the pointer. Generic codes for vmdef will do all jobs */
+ newdev->data.disk = NULL;
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
+ if (virDomainDefAddImplicitControllers(vmdef) < 0)
+ return -1;
+ if (qemuDomainAssignPCIAddresses(vmdef) < 0)
+ return -1;
+ break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the device is not supported for now"));
@@ -3935,7 +3955,17 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
virDomainDeviceDefPtr device)
{
+ virDomainDiskDefPtr disk;
+
switch(device->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ disk = device->data.disk;
+ if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("no target device %s"), disk->dst);
+ return -1;
+ }
+ break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the device is not supported for now"));
--
1.7.4.1
9:53 p.m.
New subject: [libvirt] [PATCHv9 4/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are
PCI - confliction will be found but error report is not verbose.
Drive - never done.
For example, when adding a device which has already used address.
error: Failed to attach device from /home/kamezawa/testc.xml
error: An error occurred, but the cause is unknown
After patch, the message will be
error: Failed to attach device from /home/kamezawa/testc.xml
error: invalid argument in device address conflict
This error report is better. And new code aslo checks devices other
than PCI devices.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Changelog:
- typo fix.
---
src/conf/domain_conf.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 3 ++
4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a39da7e..1de4c7a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1305,6 +1305,71 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainDeviceInfoPtr checked = opaque;
+ /* skip to check confliction of alias */
+ if (info->type != checked->type)
+ return 0;
+ if (info->alias && checked->alias && STREQ(info->alias,
checked->alias))
+ return -1;
+ /* addr is zero cleared before filled */
+ if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
+ return -1;
+ return 0;
+}
+
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev)
+{
+ virDomainDeviceInfoPtr checked;
+ int ret;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ checked = &dev->data.disk->info;
+ break;
+ case VIR_DOMAIN_DEVICE_FS:
+ checked = &dev->data.fs->info;
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ checked = &dev->data.net->info;
+ break;
+ case VIR_DOMAIN_DEVICE_INPUT:
+ checked = &dev->data.input->info;
+ break;
+ case VIR_DOMAIN_DEVICE_SOUND:
+ checked = &dev->data.sound->info;
+ break;
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ checked = &dev->data.video->info;
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ checked = &dev->data.hostdev->info;
+ break;
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ checked = &dev->data.watchdog->info;
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ checked = &dev->data.controller->info;
+ break;
+ case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
+ return 0;
+ default: /* internal error */
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Unknown device type"));
+ return -1;
+ }
+ if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+ return 0;
+ ret = virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked);
+ if (ret)
+ virDomainReportError(VIR_ERR_INVALID_ARG,
+ "%s", _("device address conflict"));
+ return ret;
+}
/* Generate a string representation of a device address
* @param address Device address to stringify
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6b7cfe7..a33c60f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1215,6 +1215,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
void virDomainDefClearPCIAddresses(virDomainDefPtr def);
void virDomainDefClearDeviceAliases(virDomainDefPtr def);
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
virDomainDeviceInfoPtr dev,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5c241aa..9eb0d59 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -222,6 +222,7 @@ virDomainCpuSetParse;
virDomainDefAddImplicitControllers;
virDomainDefClearDeviceAliases;
virDomainDefClearPCIAddresses;
+virDomainDefFindDeviceAddressConflict;
virDomainDefFormat;
virDomainDefFree;
virDomainDefParseFile;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9c4f290..aada6be 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3923,6 +3923,9 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
{
virDomainDiskDefPtr disk;
+ if (virDomainDefFindDeviceAddressConflict(vmdef, newdev))
+ return -1;
+
switch(newdev->type) {
case VIR_DOMAIN_DEVICE_DISK:
disk = newdev->data.disk;
--
1.7.4.1
9:54 p.m.
Sorry, CC list was wrong.
remove "hugh.dickins(a)tiscali.co.uk" <hugh.dickins(a)tiscali.co.uk> from CC
if you reply..
Thanks,
-Kame
4:22 a.m.
At 04/13/2011 10:47 AM, KAMEZAWA Hiroyuki Write:
Updated against the latest git tree, no major changes since a week ago.
Thank you for reviews in previous ones.
Purpose of patches:
Now, virsh at(de)tach-device/disk/...etc.. doesn't support to update
inactive domain's definition even with the --persistent flag.
To update persistent modification of inactive domains, we have to use
virsh edit.
So, if we want to update inactive domain definition with scripts, we need to
use other command/libs than libvirt. I'd like to use libvirt/virsh in scripts.
This patches adds to support for updating domain definition via virsh with
scripts. This series just includes 'disk' updates but I'll add more.
This series patches look good to me.
Eric, will you have some time to have a review on this?
Thanks,
-Kame
5003
days inactive
5008
days old
14 comments
3 participants
participants (3)
-
Eric Blake -
KAMEZAWA Hiroyuki -
Wen Congyang