[libvirt] [PATCHv9 0/4] persistent device modification for qemu
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
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@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
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@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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
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
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
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.
On Mon, 18 Apr 2011 12:59:56 +0800 Wen Congyang <wency@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
On Fri, 15 Apr 2011 15:25:01 -0600 Eric Blake <eblake@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@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
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@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
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@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
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@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
Sorry, CC list was wrong. remove "hugh.dickins@tiscali.co.uk" <hugh.dickins@tiscali.co.uk> from CC if you reply.. Thanks, -Kame
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
participants (3)
-
Eric Blake -
KAMEZAWA Hiroyuki -
Wen Congyang