Re: [libvirt] [PATCH 3/5] libvirt/qemu - support persistent modification of devices.

At 03/03/2011 09:11 AM, KAMEZAWA Hiroyuki Write: >>From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; > Date: Thu, 3 Mar 2011 09:43:07 +0900 > Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices. > > Now, virsh attach-***/detach-*** has --persistent option and > it can update XML definition of inactive domains. > Now, disk and nic are supported in Xen, but none in qemu. > > This patch adds a function for permanent modification for qemu. > Following patches will add disk/network suport. > > TODO: > - finally, need to do hotplug + XML modification for active domain. > > Changelog v3->v4 > - fixed trailing white spaces. > - splitted into 2 part and this patch only contains wrappers. > Changelog v2->v3: > - clean ups. > - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. > --- > src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 131 insertions(+), 12 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index c9095bb..3248cdc 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4099,16 +4099,131 @@ 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) > +{ > + > + /* At first, check device confliction */ > + switch(newdev->type) { > + default: > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", When the device is not supported, we should use VIR_ERR_CONFIG_UNSUPPORTED instead of VIR_ERR_INVALID_ARG.

> + _("Sorry, the device is not suppored for now")); > + return -1; > + } > + > + return 0; > +} > + > +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, > + virDomainDeviceDefPtr device) The coding style is wrong: virDomainDeviceDefPtr shoulde be left justified with virDomainDefPtr.

> +{ > + switch(device->type) { > + default: > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("Sorry, the device is not suppored for now")); > + return -1; > + } > + return 0; > +} > + > +static int qemuDomainModifyDevicePersistent(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")); > + unsigned int attach, int flags) Why do you modify the type of flag?

> +{ > + struct qemud_driver *driver; > + virDomainDeviceDefPtr device; > + virDomainDefPtr vmdef; > + virDomainObjPtr vm; > + int ret = -1; > + > + if (!dom || !dom->conn || !dom->name || !xml) { dom and dom->conn does not need check, they are always not null.

> + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("internal error : %s"), __FUNCTION__); Do not use __FUNCTION__ here, the macro qemuReportError will do it. > + return -1; > + } > + /* > + * 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_INVALID_ARG, "%s", Please use VIR_ERR_OPERATION_INVALID instead of VIR_ERR_INVALID_ARG.

> + _("Now, cannot modify alive domain and its definition " > + "at the same time.")); > return -1; > } > > - return qemudDomainAttachDevice(dom, xml); > + driver = dom->conn->privateData; > + qemuDriverLock(driver); > + vm = virDomainFindByUUID(&driver->domains, dom->uuid); > + if (!vm) { > + qemuReportError(VIR_ERR_NO_DOMAIN, _("cannot find domain '%s'"), > + dom->name); You use UUID to search the domain, so the error message should be: no domain with matching uuid '%s'

> + 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.")); > + 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) /* save the result */ > + ret = virDomainSaveConfig(driver->configDir, vmdef); > + > + virDomainDeviceDefFree(device); > + > +endjob: > + if (qemuDomainObjEndJob(vm) == 0) > + vm = NULL; > + if (vm) > + virDomainObjUnlock(vm); When qemuDomainObjBeginJobWithDriver() failed, we should unlock the domain. So we should do it when we goto unlock_out.

> +unlock_out: > + 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"), > + flags); > + > + return -1; > } > > > @@ -4322,13 +4437,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"), > + flags); > + return -1; > } > > static int qemudDomainGetAutostart(virDomainPtr dom,