Re: [libvirt] [PATCHv5 1/3] libivrt/qemu - support persistent modification of devices

At 03/16/2011 02:38 PM, KAMEZAWA Hiroyuki Write: > Thank you for review and feedbacks. This is v5 onto the latest git tree. > Any comments are welcome. > == >>From ef85d268a0b553cdb784a1d1916ff8cf171adace Mon Sep 17 00:00:00 2001 > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; > Date: Wed, 16 Mar 2011 11:35:41 +0900 > Subject: [PATCHv5 1/3] libivrt/qemu - support 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. > > Changelog v4->v5: > - fixed error codes at reporting Error. > - fixed unlock code after calling qemuDomainObjBeginJobWithDriver() > - fixed virDomainFindByUUID() to be virDomainFindByName() > - fixed spelling, indent, etc.. > - removed unnecessary checks. > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; > --- > src/qemu/qemu_driver.c | 141 +++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 129 insertions(+), 12 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index dac2bf2..dbd5bd3 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4130,16 +4130,129 @@ cleanup: > return ret; > } > > -static int qemudDomainAttachDeviceFlags(virDomainPtr dom, > +/* > + * Attach a device given by XML, the change will be persistent > + * and domain XML definition file is updated. > + */ > +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, > + virDomainDeviceDefPtr newdev) > +{ > + > + switch(newdev->type) { > + default: > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Sorry, the device is not supported for now")); > + return -1; > + } > + > + return 0; > +} > + > +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, > + virDomainDeviceDefPtr device) > +{ > + switch(device->type) { > + default: > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Sorry, the device is not supported for now")); > + return -1; > + } > + return 0; > +} > + > +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, > const char *xml, Indentation is not consistent.

> - unsigned int flags) { > - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { > - qemuReportError(VIR_ERR_OPERATION_INVALID, > - "%s", _("cannot modify the persistent configuration of a domain")); > + unsigned int attach, unsigned int flags) > +{ > + struct qemud_driver *driver; > + virDomainDeviceDefPtr device; > + virDomainDefPtr vmdef; > + virDomainObjPtr vm; > + int ret = -1; > + > + if (!xml) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal error")); > return -1; > } xml can not be null, because remoteDomainAttachDevice()/remoteDomainAttachDeviceFlags() will fail on client when xml is null.

> + /* > + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified 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", > + _("Now, cannot modify alive 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)) { > + /* > + * For now, just allow updating inactive domains. Further development > + * will allow updating both active domain and its config file at > + * the same time. > + */ > + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("Now, it's unsupported to update active domain's definition.")); Indentation is not consistent. > + goto endjob; > + } > + > + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); > + > + if (!vmdef) > + goto endjob; > + > + device = virDomainDeviceDefParse(driver->caps, > + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); Indentation is not consistent. > + if (!device) > + goto endjob; > + > + if (attach) > + ret = qemuDomainAttachDevicePersistent(vmdef, device); > + else > + ret = qemuDomainDetachDevicePersistent(vmdef, device); > + > + if (!ret) /* save the result */ > + 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) > +{ > + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) > + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); > + > + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) > + return qemudDomainAttachDevice(dom, xml); > + > + qemuReportError(VIR_ERR_INVALID_ARG, > + _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), Indentation is not consistent. > + flags); > > - return qemudDomainAttachDevice(dom, xml); > + return -1; > } > > > @@ -4354,13 +4467,17 @@ 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; > - } > > - return qemudDomainDetachDevice(dom, xml); > + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) > + return qemuDomainModifyDevicePersistent(dom, xml, 0, flags); > + > + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) > + return qemudDomainDetachDevice(dom, xml); > + > + qemuReportError(VIR_ERR_INVALID_ARG, > + _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), Indentation is not consistent. > + flags); > + return -1; > } > > static int qemudDomainGetAutostart(virDomainPtr dom,