Re: [libvirt] [Patch v7 1/4] libvirt/qemu - persistent modification of domain.

On Tue, 29 Mar 2011 11:24:23 +0800 Wen Congyang <wency(a)cn.fujitsu.com&gt; wrote: > At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write: >> This is v7. Dropped patches for Nics and added 2 sanity checks and >> show correct error messages. >> >> = >> >From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001 >> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; >> Date: Fri, 25 Mar 2011 16:59:55 +0900 >> Subject: [PATCHv7 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&gt; >> --- >> src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++----- >> 1 files changed, 125 insertions(+), 13 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index af897ad..a4fb1cd 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -4144,16 +4144,125 @@ 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")); >> +/* >> + * 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, >> + 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 is specified at the same, >> + * time return error for now. We should support this later. > > s/at the same, time/at the same time,/ > will fix. >> + */ >> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { >> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", >> + _("Now, cannot modify alive domain and its definition " > > You use 'alive domain' here, but you use 'active domain' below. > The message should be consistent. > will fix. >> + "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", >> + _("unsupported to update active domain's definition.")); >> + goto endjob; >> + } >> + >> + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); >> >> - return qemudDomainAttachDevice(dom, xml); >> + 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); > > vmdef may be modified even if ret is not 0, but you do not update > the xml file. I do not find any problem about this, but it may be > better to update the xml file when ret is not 0. > Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to never update vmdef when return !0. Is it ok ?

>> + >> + 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 not supported"), flags); > > If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not > report error here. > > You can use the macro virCheckFlags to check it. > Hm, ok. will see it. Thanks, -Kame