[libvirt] [PATCHv8 0/4] libvirt/qemu - persistent modification of inactive domans.
Hi, this is v8. This one implements a new method for rollback in domain changes in patch 2/4. And I dropped "verify new device is.." patch which was 4/4 in v7. Then, totarl numbers of patches are not changed. I'll add NIC support when this patch goes. 1/4 ..... a base patch for all devices. (not changed much) 2/4 ..... copy-sync-undo for synchronized update of domain definition. 3/4 ..... allow persistent addtion of disks to invalid domain. 4/4 ..... address conflic check. Thanks, -Kame
From 1ffbe73b1663719414367abbdebc8f31b9592331 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 16:20:05 +0900 Subject: [PATCHv8 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@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 150 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 136 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..b89bc8f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3873,16 +3873,133 @@ 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")); +/* + * At(de)tach a device given by XML, the change will be persistent + * and domain XML definition file is updated when these function + * returns 0(success). IOW, at failure, XML definition is never updated. + * So, this function must guarantee consistency between vmdef and its XML + * definition in the file. + */ + +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)) { + /* + * 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", + _("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); + /* + * At(De)tachDevicePersistent() must guarantee that vmdef is consistent + * with XML definition when they returns a failure, ret != 0. + */ + 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; } @@ -4096,14 +4213,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
At 04/01/2011 12:17 PM, KAMEZAWA Hiroyuki Write:
From 1ffbe73b1663719414367abbdebc8f31b9592331 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 16:20:05 +0900 Subject: [PATCHv8 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@jp.fujitsu.com>
--- src/qemu/qemu_driver.c | 150 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 136 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..b89bc8f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3873,16 +3873,133 @@ 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")); +/* + * At(de)tach a device given by XML, the change will be persistent + * and domain XML definition file is updated when these function + * returns 0(success). IOW, at failure, XML definition is never updated. + * So, this function must guarantee consistency between vmdef and its XML + * definition in the file. + */ + +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)) { + /* + * 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", + _("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); + /* + * At(De)tachDevicePersistent() must guarantee that vmdef is consistent + * with XML definition when they returns a failure, ret != 0. + */
This comment can be removed in patch 2 as we use a copy of XML definition.
+ 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; }
@@ -4096,14 +4213,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,
On Fri, 01 Apr 2011 15:30:13 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 04/01/2011 12:17 PM, KAMEZAWA Hiroyuki Write:
From 1ffbe73b1663719414367abbdebc8f31b9592331 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 16:20:05 +0900 Subject: [PATCHv8 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@jp.fujitsu.com>
--- src/qemu/qemu_driver.c | 150 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 136 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..b89bc8f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3873,16 +3873,133 @@ 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")); +/* + * At(de)tach a device given by XML, the change will be persistent + * and domain XML definition file is updated when these function + * returns 0(success). IOW, at failure, XML definition is never updated. + * So, this function must guarantee consistency between vmdef and its XML + * definition in the file. + */ + +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)) { + /* + * 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", + _("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); + /* + * At(De)tachDevicePersistent() must guarantee that vmdef is consistent + * with XML definition when they returns a failure, ret != 0. + */
This comment can be removed in patch 2 as we use a copy of XML definition.
Hmm, will do when I'll be back. I'll stop update for a week. Thanks, -Kame
From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 18:52:24 +0900 Subject: [PATCHv8 2/4] libvirt/qemu - synchronous update of device definition.
At persistent modification of inactive domains, we go several steps as - insert disk to vmdef - assign pci address if necessary - assign controller if necessary - save it to file If failure happens in above sequence, that implies there is inconsistency between XML definition in file and vmdef cached in memory. So, it's better to have some rollback method. Implementing undo in each stage doesn't seem very neat, this patch implements a generic one. This patch adds 3 calls. virDomainTemporalCopyAlloc(vmdef) virDomainTemporalCopySync(orig, copy) virDomainTemporalCopyUndo(orig, copy) CopyAlloc() will create a copy of vmdef, CopySync() is for synchronizing copy and orginal, updating origina., CopyUndo() is for discarding for discarding unnecessary things in copy at failure. With these, vmdef modification can be done in following manner. copy = virDomainTemporalCopyAlloc(orig) ....do update on copy ....save updated data to its file if (error) virDomainTemporalCopyUndo(orig, copy); else virDomainTemporalCopySync(orig, copy) # this never fails. At failure of attach or success of detach, all stale devices in copy or orig will be freed automatically. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 + src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 18 +++- 4 files changed, 230 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6aaf33..098face 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9368,3 +9368,211 @@ cleanup: return ret; } + +/* + * This function creates a copy of vmdef. The copied vmdef and + * original vmdef share their pointers other than arrays. + * The caller can make any changes to Copy, and later + * sync copy and original one, later. + * + * The caller must guarantee + * - vmdef is under a lock and no other one touches it. + * - never free pointers pointed by vmdef or copy before calling + * virDomainTemporalCopySync() or virDomainTemporalCopyUndo(). + * + * This function just handles 'generic' structure of virDomainDef. + * So, if some driver specific handling is required, you need another + * calls. See qemuDomainModifyDevicePersistent() for example. + */ + +static void virDomainTemporalCopyFree(virDomainDefPtr copy) +{ + VIR_FREE(copy->graphics); + VIR_FREE(copy->inputs); + VIR_FREE(copy->disks); + VIR_FREE(copy->controllers); + VIR_FREE(copy->fss); + VIR_FREE(copy->nets); + VIR_FREE(copy->smartcards); + VIR_FREE(copy->serials); + VIR_FREE(copy->parallels); + VIR_FREE(copy->channels); + VIR_FREE(copy->sounds); + VIR_FREE(copy->videos); + VIR_FREE(copy->hostdevs); + VIR_FREE(copy); +} + +#define DupArray(ptr, count, orig) do {\ + if (VIR_ALLOC_N((ptr), (count)) < 0) {\ + virReportOOMError();\ + goto oom_out;\ + }\ + memcpy((ptr), (orig), sizeof(*ptr) * (count));\ +} while (0) + +virDomainDefPtr virDomainTemporalCopyAlloc(virDomainDefPtr vmdef) +{ + virDomainDefPtr copy; + + if (VIR_ALLOC(copy) < 0) { + virReportOOMError(); + return NULL; + } + + memcpy(copy, vmdef, sizeof(*copy)); + + /* + * For now, this is used for modify devices. Further updates + * may be necessary for handle other components. + * At handling copy, array of disks,networks....can be + * replaced by realloc(). We need to allocate another array + * to avoid confusions. + */ + copy->graphics = NULL; + copy->inputs = NULL; + copy->disks = NULL; + copy->controllers = NULL; + copy->fss = NULL; + copy->nets = NULL; + copy->smartcards = NULL; + copy->serials = NULL; + copy->parallels = NULL; + copy->channels = NULL; + copy->sounds = NULL; + copy->videos = NULL; + copy->hostdevs = NULL; + + DupArray(copy->graphics, vmdef->ngraphics, vmdef->graphics); + DupArray(copy->inputs, vmdef->ninputs, vmdef->inputs); + DupArray(copy->disks, vmdef->ndisks, vmdef->disks); + DupArray(copy->controllers, vmdef->ncontrollers, vmdef->controllers); + DupArray(copy->fss, vmdef->nfss, vmdef->fss); + DupArray(copy->nets, vmdef->nnets, vmdef->nets); + DupArray(copy->smartcards, vmdef->nsmartcards, vmdef->smartcards); + DupArray(copy->serials, vmdef->nserials, vmdef->serials); + DupArray(copy->parallels, vmdef->nparallels, vmdef->parallels); + DupArray(copy->channels, vmdef->nchannels, vmdef->channels); + DupArray(copy->sounds, vmdef->nsounds, vmdef->sounds); + DupArray(copy->videos, vmdef->nvideos, vmdef->videos); + DupArray(copy->hostdevs, vmdef->nhostdevs, vmdef->hostdevs); + + /* + * After this, we can modify copy with usual routines. + */ + return copy; +oom_out: + virDomainTemporalCopyFree(copy); + return NULL; +} +#undef DupArray + + +/* + * Original anc copy has different arrays, but objects potinted by + * arrays are shared. At syncing, if an object is in orig but not in copy, + * free it. If an object is not in orig but in copy, copy it. + * At rollback, if an object is in copy but not in orig, free it. + */ + +/* use macro for avoiding warnings */ +#define SyncArray(orig, copy, member, num, func) do {\ + int i, j;\ + for (i = 0; i < orig->num; i++) {\ + for (j = 0; j < copy->num; j++) \ + if (orig->member[i] == copy->member[j])\ + break;\ + if (j == copy->num)\ + func(orig->member[i]);\ + }\ +} while (0); + +#define ReplaceArray(orig, copy, member, num) do {\ + VIR_FREE(orig->member);\ + orig->member = copy->member;\ + copy->member = NULL;\ + orig->num = copy->num;\ +} while (0); + +void virDomainTemporalCopySync(virDomainDefPtr orig, + virDomainDefPtr copy) +{ + SyncArray(orig, copy, graphics, ngraphics, virDomainGraphicsDefFree); + ReplaceArray(orig, copy, graphics, ngraphics); + + SyncArray(orig, copy, inputs, ninputs, virDomainInputDefFree); + ReplaceArray(orig, copy, inputs, ninputs); + + SyncArray(orig, copy, disks, ndisks, virDomainDiskDefFree); + ReplaceArray(orig, copy, disks, ndisks); + + SyncArray(orig, copy, controllers, + ncontrollers, virDomainControllerDefFree); + ReplaceArray(orig, copy, controllers, ncontrollers); + + SyncArray(orig, copy, fss, nfss, virDomainFSDefFree); + ReplaceArray(orig, copy, fss, nfss); + + SyncArray(orig, copy, nets, nnets, virDomainNetDefFree); + ReplaceArray(orig, copy, nets, nnets); + + SyncArray(orig, copy, smartcards, nsmartcards, virDomainSmartcardDefFree); + ReplaceArray(orig, copy, smartcards, nsmartcards); + + SyncArray(orig, copy, serials, nserials, virDomainChrDefFree); + ReplaceArray(orig, copy, serials, nserials); + + SyncArray(orig, copy, parallels, nparallels, virDomainChrDefFree); + ReplaceArray(orig, copy, parallels, nparallels); + + SyncArray(orig, copy, channels, nchannels, virDomainChrDefFree); + ReplaceArray(orig, copy, channels, nchannels); + + SyncArray(orig, copy, sounds, nsounds, virDomainSoundDefFree); + ReplaceArray(orig, copy, sounds, nsounds); + + SyncArray(orig, copy, videos, nvideos, virDomainVideoDefFree); + ReplaceArray(orig, copy, videos, nvideos); + + SyncArray(orig, copy, hostdevs, nhostdevs, virDomainHostdevDefFree); + ReplaceArray(orig, copy, hostdevs, nhostdevs); + + VIR_FREE(copy); +} +/* + * Free all devices exists only in the copy. + */ +void virDomainTemporalCopyUndo(virDomainDefPtr orig, + virDomainDefPtr copy) +{ + /* Free redundunt objects in the copied array */ + SyncArray(copy, orig, graphics, ngraphics, virDomainGraphicsDefFree); + + SyncArray(copy, orig, inputs, ninputs, virDomainInputDefFree); + + SyncArray(copy, orig, disks, ndisks, virDomainDiskDefFree); + + SyncArray(copy, orig, + controllers, ncontrollers, virDomainControllerDefFree); + SyncArray(copy, orig, fss, nfss, virDomainFSDefFree); + + SyncArray(copy, orig, nets, nnets, virDomainNetDefFree); + + SyncArray(copy, orig, smartcards, nsmartcards, virDomainSmartcardDefFree); + + SyncArray(copy, orig, serials, nserials, virDomainChrDefFree); + + SyncArray(copy, orig, parallels, nparallels, virDomainChrDefFree); + + SyncArray(copy, orig, channels, nchannels, virDomainChrDefFree); + + SyncArray(copy, orig, sounds, nsounds, virDomainSoundDefFree); + + SyncArray(copy, orig, videos, nvideos, virDomainVideoDefFree); + + SyncArray(copy, orig, hostdevs, nhostdevs, virDomainHostdevDefFree); + + virDomainTemporalCopyFree(copy); +} +#undef SyncArray +#undef ReplaceArray diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 10e73cb..28b3c0e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1377,6 +1377,11 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainDiskDefPathIterator iter, void *opaque); +virDomainDefPtr virDomainTemporalCopyAlloc(virDomainDefPtr vmdef); +void virDomainTemporalCopySync(virDomainDefPtr orig, + virDomainDefPtr copy); +void virDomainTemporalCopyUndo(virDomainDefPtr orig, virDomainDefPtr copy); + 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 65a86d3..62643d4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -312,6 +312,9 @@ virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; virDomainStateTypeFromString; virDomainStateTypeToString; +virDomainTemporalCopyAlloc; +virDomainTemporalCopySync; +virDomainTemporalCopyUndo; virDomainTimerModeTypeFromString; virDomainTimerModeTypeToString; virDomainTimerNameTypeFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b89bc8f..08055ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3914,7 +3914,7 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, { struct qemud_driver *driver; virDomainDeviceDefPtr device; - virDomainDefPtr vmdef; + virDomainDefPtr vmdef, copied; virDomainObjPtr vm; int ret = -1; @@ -3962,16 +3962,26 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, if (!device) goto endjob; + copied = virDomainTemporalCopyAlloc(vmdef); + if (!copied) + goto endjob; + if (attach) - ret = qemuDomainAttachDevicePersistent(vmdef, device); + ret = qemuDomainAttachDevicePersistent(copied, device); else - ret = qemuDomainDetachDevicePersistent(vmdef, device); + ret = qemuDomainDetachDevicePersistent(copied, device); /* * At(De)tachDevicePersistent() must guarantee that vmdef is consistent * with XML definition when they returns a failure, ret != 0. */ if (!ret) - ret = virDomainSaveConfig(driver->configDir, vmdef); + ret = virDomainSaveConfig(driver->configDir, copied); + + /* If successfully saved the new vmdef, sync it. this never fails */ + if (!ret) + virDomainTemporalCopySync(vmdef, copied); + else /* discard */ + virDomainTemporalCopyUndo(vmdef, copied); virDomainDeviceDefFree(device); -- 1.7.4.1
At 04/01/2011 12:19 PM, KAMEZAWA Hiroyuki Write:
From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 18:52:24 +0900 Subject: [PATCHv8 2/4] libvirt/qemu - synchronous update of device definition.
At persistent modification of inactive domains, we go several steps as - insert disk to vmdef - assign pci address if necessary - assign controller if necessary - save it to file
Step 2 should be after step 3.
If failure happens in above sequence, that implies there is inconsistency between XML definition in file and vmdef cached in memory. So, it's better to have some rollback method. Implementing undo in each stage doesn't seem very neat, this patch implements a generic one.
This patch adds 3 calls. virDomainTemporalCopyAlloc(vmdef) virDomainTemporalCopySync(orig, copy) virDomainTemporalCopyUndo(orig, copy)
CopyAlloc() will create a copy of vmdef, CopySync() is for synchronizing copy and orginal, updating origina., CopyUndo() is for discarding for discarding unnecessary things in copy at failure. With these, vmdef modification can be done in following manner.
copy = virDomainTemporalCopyAlloc(orig) ....do update on copy ....save updated data to its file if (error) virDomainTemporalCopyUndo(orig, copy); else virDomainTemporalCopySync(orig, copy) # this never fails.
You only copy arrays in orig. How about copy all from orig to copy? And we can introduce a new API virDomainObjAssignPersistentDef() that is like the API virDomainObjAssignDef(). So vmdef modification can be done like this: copy = virDomainTemporalCopyAlloc(orig) ....do update on copy if (error) { virDomainDefFree(copy); } else { virDomainObjAssignPersistentDef(vm, copy) # this never fails. ....save updated data to its file } We can copy vmdef easily: 1. xml = virDomainDefFormat(def, VIR_DOMAIN_XML_WRITE_FLAGS) 2. newdef = virDomainDefParseString(caps, xml, VIR_DOMAIN_XML_READ_FLAGS)
At failure of attach or success of detach, all stale devices in copy or orig will be freed automatically.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 + src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 18 +++- 4 files changed, 230 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6aaf33..098face 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9368,3 +9368,211 @@ cleanup:
return ret; } + +/* + * This function creates a copy of vmdef. The copied vmdef and + * original vmdef share their pointers other than arrays. + * The caller can make any changes to Copy, and later + * sync copy and original one, later. + * + * The caller must guarantee + * - vmdef is under a lock and no other one touches it. + * - never free pointers pointed by vmdef or copy before calling + * virDomainTemporalCopySync() or virDomainTemporalCopyUndo(). + * + * This function just handles 'generic' structure of virDomainDef. + * So, if some driver specific handling is required, you need another + * calls. See qemuDomainModifyDevicePersistent() for example. + */ + +static void virDomainTemporalCopyFree(virDomainDefPtr copy) +{ + VIR_FREE(copy->graphics); + VIR_FREE(copy->inputs); + VIR_FREE(copy->disks); + VIR_FREE(copy->controllers); + VIR_FREE(copy->fss); + VIR_FREE(copy->nets); + VIR_FREE(copy->smartcards); + VIR_FREE(copy->serials); + VIR_FREE(copy->parallels); + VIR_FREE(copy->channels); + VIR_FREE(copy->sounds); + VIR_FREE(copy->videos); + VIR_FREE(copy->hostdevs); + VIR_FREE(copy); +} + +#define DupArray(ptr, count, orig) do {\ + if (VIR_ALLOC_N((ptr), (count)) < 0) {\ + virReportOOMError();\ + goto oom_out;\ + }\ + memcpy((ptr), (orig), sizeof(*ptr) * (count));\ +} while (0) + +virDomainDefPtr virDomainTemporalCopyAlloc(virDomainDefPtr vmdef) +{ + virDomainDefPtr copy; + + if (VIR_ALLOC(copy) < 0) { + virReportOOMError(); + return NULL; + } + + memcpy(copy, vmdef, sizeof(*copy)); + + /* + * For now, this is used for modify devices. Further updates + * may be necessary for handle other components. + * At handling copy, array of disks,networks....can be + * replaced by realloc(). We need to allocate another array + * to avoid confusions. + */ + copy->graphics = NULL; + copy->inputs = NULL; + copy->disks = NULL; + copy->controllers = NULL; + copy->fss = NULL; + copy->nets = NULL; + copy->smartcards = NULL; + copy->serials = NULL; + copy->parallels = NULL; + copy->channels = NULL; + copy->sounds = NULL; + copy->videos = NULL; + copy->hostdevs = NULL; + + DupArray(copy->graphics, vmdef->ngraphics, vmdef->graphics); + DupArray(copy->inputs, vmdef->ninputs, vmdef->inputs); + DupArray(copy->disks, vmdef->ndisks, vmdef->disks); + DupArray(copy->controllers, vmdef->ncontrollers, vmdef->controllers); + DupArray(copy->fss, vmdef->nfss, vmdef->fss); + DupArray(copy->nets, vmdef->nnets, vmdef->nets); + DupArray(copy->smartcards, vmdef->nsmartcards, vmdef->smartcards); + DupArray(copy->serials, vmdef->nserials, vmdef->serials); + DupArray(copy->parallels, vmdef->nparallels, vmdef->parallels); + DupArray(copy->channels, vmdef->nchannels, vmdef->channels); + DupArray(copy->sounds, vmdef->nsounds, vmdef->sounds); + DupArray(copy->videos, vmdef->nvideos, vmdef->videos); + DupArray(copy->hostdevs, vmdef->nhostdevs, vmdef->hostdevs); + + /* + * After this, we can modify copy with usual routines. + */ + return copy; +oom_out: + virDomainTemporalCopyFree(copy); + return NULL; +} +#undef DupArray + + +/* + * Original anc copy has different arrays, but objects potinted by + * arrays are shared. At syncing, if an object is in orig but not in copy, + * free it. If an object is not in orig but in copy, copy it. + * At rollback, if an object is in copy but not in orig, free it. + */ + +/* use macro for avoiding warnings */ +#define SyncArray(orig, copy, member, num, func) do {\ + int i, j;\ + for (i = 0; i < orig->num; i++) {\ + for (j = 0; j < copy->num; j++) \ + if (orig->member[i] == copy->member[j])\ + break;\ + if (j == copy->num)\ + func(orig->member[i]);\ + }\ +} while (0); + +#define ReplaceArray(orig, copy, member, num) do {\ + VIR_FREE(orig->member);\ + orig->member = copy->member;\ + copy->member = NULL;\ + orig->num = copy->num;\ +} while (0); + +void virDomainTemporalCopySync(virDomainDefPtr orig, + virDomainDefPtr copy) +{ + SyncArray(orig, copy, graphics, ngraphics, virDomainGraphicsDefFree); + ReplaceArray(orig, copy, graphics, ngraphics); + + SyncArray(orig, copy, inputs, ninputs, virDomainInputDefFree); + ReplaceArray(orig, copy, inputs, ninputs); + + SyncArray(orig, copy, disks, ndisks, virDomainDiskDefFree); + ReplaceArray(orig, copy, disks, ndisks); + + SyncArray(orig, copy, controllers, + ncontrollers, virDomainControllerDefFree); + ReplaceArray(orig, copy, controllers, ncontrollers); + + SyncArray(orig, copy, fss, nfss, virDomainFSDefFree); + ReplaceArray(orig, copy, fss, nfss); + + SyncArray(orig, copy, nets, nnets, virDomainNetDefFree); + ReplaceArray(orig, copy, nets, nnets); + + SyncArray(orig, copy, smartcards, nsmartcards, virDomainSmartcardDefFree); + ReplaceArray(orig, copy, smartcards, nsmartcards); + + SyncArray(orig, copy, serials, nserials, virDomainChrDefFree); + ReplaceArray(orig, copy, serials, nserials); + + SyncArray(orig, copy, parallels, nparallels, virDomainChrDefFree); + ReplaceArray(orig, copy, parallels, nparallels); + + SyncArray(orig, copy, channels, nchannels, virDomainChrDefFree); + ReplaceArray(orig, copy, channels, nchannels); + + SyncArray(orig, copy, sounds, nsounds, virDomainSoundDefFree); + ReplaceArray(orig, copy, sounds, nsounds); + + SyncArray(orig, copy, videos, nvideos, virDomainVideoDefFree); + ReplaceArray(orig, copy, videos, nvideos); + + SyncArray(orig, copy, hostdevs, nhostdevs, virDomainHostdevDefFree); + ReplaceArray(orig, copy, hostdevs, nhostdevs); + + VIR_FREE(copy); +} +/* + * Free all devices exists only in the copy. + */ +void virDomainTemporalCopyUndo(virDomainDefPtr orig, + virDomainDefPtr copy) +{ + /* Free redundunt objects in the copied array */ + SyncArray(copy, orig, graphics, ngraphics, virDomainGraphicsDefFree); + + SyncArray(copy, orig, inputs, ninputs, virDomainInputDefFree); + + SyncArray(copy, orig, disks, ndisks, virDomainDiskDefFree); + + SyncArray(copy, orig, + controllers, ncontrollers, virDomainControllerDefFree); + SyncArray(copy, orig, fss, nfss, virDomainFSDefFree); + + SyncArray(copy, orig, nets, nnets, virDomainNetDefFree); + + SyncArray(copy, orig, smartcards, nsmartcards, virDomainSmartcardDefFree); + + SyncArray(copy, orig, serials, nserials, virDomainChrDefFree); + + SyncArray(copy, orig, parallels, nparallels, virDomainChrDefFree); + + SyncArray(copy, orig, channels, nchannels, virDomainChrDefFree); + + SyncArray(copy, orig, sounds, nsounds, virDomainSoundDefFree); + + SyncArray(copy, orig, videos, nvideos, virDomainVideoDefFree); + + SyncArray(copy, orig, hostdevs, nhostdevs, virDomainHostdevDefFree); + + virDomainTemporalCopyFree(copy); +} +#undef SyncArray +#undef ReplaceArray diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 10e73cb..28b3c0e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1377,6 +1377,11 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, virDomainDiskDefPathIterator iter, void *opaque);
+virDomainDefPtr virDomainTemporalCopyAlloc(virDomainDefPtr vmdef); +void virDomainTemporalCopySync(virDomainDefPtr orig, + virDomainDefPtr copy); +void virDomainTemporalCopyUndo(virDomainDefPtr orig, virDomainDefPtr copy); + 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 65a86d3..62643d4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -312,6 +312,9 @@ virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; virDomainStateTypeFromString; virDomainStateTypeToString; +virDomainTemporalCopyAlloc; +virDomainTemporalCopySync; +virDomainTemporalCopyUndo; virDomainTimerModeTypeFromString; virDomainTimerModeTypeToString; virDomainTimerNameTypeFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b89bc8f..08055ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3914,7 +3914,7 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, { struct qemud_driver *driver; virDomainDeviceDefPtr device; - virDomainDefPtr vmdef; + virDomainDefPtr vmdef, copied; virDomainObjPtr vm; int ret = -1;
@@ -3962,16 +3962,26 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, if (!device) goto endjob;
+ copied = virDomainTemporalCopyAlloc(vmdef); + if (!copied) + goto endjob; + if (attach) - ret = qemuDomainAttachDevicePersistent(vmdef, device); + ret = qemuDomainAttachDevicePersistent(copied, device); else - ret = qemuDomainDetachDevicePersistent(vmdef, device); + ret = qemuDomainDetachDevicePersistent(copied, device); /* * At(De)tachDevicePersistent() must guarantee that vmdef is consistent * with XML definition when they returns a failure, ret != 0. */ if (!ret) - ret = virDomainSaveConfig(driver->configDir, vmdef); + ret = virDomainSaveConfig(driver->configDir, copied); + + /* If successfully saved the new vmdef, sync it. this never fails */ + if (!ret) + virDomainTemporalCopySync(vmdef, copied); + else /* discard */ + virDomainTemporalCopyUndo(vmdef, copied);
virDomainDeviceDefFree(device);
On Fri, 01 Apr 2011 14:10:18 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 04/01/2011 12:19 PM, KAMEZAWA Hiroyuki Write:
From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 18:52:24 +0900 Subject: [PATCHv8 2/4] libvirt/qemu - synchronous update of device definition.
At persistent modification of inactive domains, we go several steps as - insert disk to vmdef - assign pci address if necessary - assign controller if necessary - save it to file
Step 2 should be after step 3.
yes ;)
If failure happens in above sequence, that implies there is inconsistency between XML definition in file and vmdef cached in memory. So, it's better to have some rollback method. Implementing undo in each stage doesn't seem very neat, this patch implements a generic one.
This patch adds 3 calls. virDomainTemporalCopyAlloc(vmdef) virDomainTemporalCopySync(orig, copy) virDomainTemporalCopyUndo(orig, copy)
CopyAlloc() will create a copy of vmdef, CopySync() is for synchronizing copy and orginal, updating origina., CopyUndo() is for discarding for discarding unnecessary things in copy at failure. With these, vmdef modification can be done in following manner.
copy = virDomainTemporalCopyAlloc(orig) ....do update on copy ....save updated data to its file if (error) virDomainTemporalCopyUndo(orig, copy); else virDomainTemporalCopySync(orig, copy) # this never fails.
You only copy arrays in orig. How about copy all from orig to copy? And we can introduce a new API virDomainObjAssignPersistentDef() that is like the API virDomainObjAssignDef().
So vmdef modification can be done like this: copy = virDomainTemporalCopyAlloc(orig) ....do update on copy if (error) { virDomainDefFree(copy); } else { virDomainObjAssignPersistentDef(vm, copy) # this never fails. ....save updated data to its file }
We can copy vmdef easily: 1. xml = virDomainDefFormat(def, VIR_DOMAIN_XML_WRITE_FLAGS) 2. newdef = virDomainDefParseString(caps, xml, VIR_DOMAIN_XML_READ_FLAGS)
Hmm, could you explain virDomainObjAssignPersistentDef() ? I think I should finally support moficication of active domains. Can I replace the whole vmdef of active domain ? Thanks, -Kame
At 04/01/2011 02:16 PM, KAMEZAWA Hiroyuki Write:
On Fri, 01 Apr 2011 14:10:18 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 04/01/2011 12:19 PM, KAMEZAWA Hiroyuki Write:
From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 31 Mar 2011 18:52:24 +0900 Subject: [PATCHv8 2/4] libvirt/qemu - synchronous update of device definition.
At persistent modification of inactive domains, we go several steps as - insert disk to vmdef - assign pci address if necessary - assign controller if necessary - save it to file
Step 2 should be after step 3.
yes ;)
If failure happens in above sequence, that implies there is inconsistency between XML definition in file and vmdef cached in memory. So, it's better to have some rollback method. Implementing undo in each stage doesn't seem very neat, this patch implements a generic one.
This patch adds 3 calls. virDomainTemporalCopyAlloc(vmdef) virDomainTemporalCopySync(orig, copy) virDomainTemporalCopyUndo(orig, copy)
CopyAlloc() will create a copy of vmdef, CopySync() is for synchronizing copy and orginal, updating origina., CopyUndo() is for discarding for discarding unnecessary things in copy at failure. With these, vmdef modification can be done in following manner.
copy = virDomainTemporalCopyAlloc(orig) ....do update on copy ....save updated data to its file if (error) virDomainTemporalCopyUndo(orig, copy); else virDomainTemporalCopySync(orig, copy) # this never fails.
You only copy arrays in orig. How about copy all from orig to copy? And we can introduce a new API virDomainObjAssignPersistentDef() that is like the API virDomainObjAssignDef().
So vmdef modification can be done like this: copy = virDomainTemporalCopyAlloc(orig) ....do update on copy if (error) { virDomainDefFree(copy); } else { virDomainObjAssignPersistentDef(vm, copy) # this never fails. ....save updated data to its file }
We can copy vmdef easily: 1. xml = virDomainDefFormat(def, VIR_DOMAIN_XML_WRITE_FLAGS) 2. newdef = virDomainDefParseString(caps, xml, VIR_DOMAIN_XML_READ_FLAGS)
Hmm, could you explain virDomainObjAssignPersistentDef() ?
I think I should finally support moficication of active domains. Can I replace the whole vmdef of active domain ?
I found that virDomainObjAssignDef() can work. We can call it like this: virDomainObjAssignDef(vm, copy, false). We modify vm->newdef if domain is active, and modify vm->def if domain is inactive.
Thanks, -Kame
On Fri, 01 Apr 2011 14:33:23 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
I found that virDomainObjAssignDef() can work. We can call it like this: virDomainObjAssignDef(vm, copy, false).
We modify vm->newdef if domain is active, and modify vm->def if domain is inactive.
Okay, here is a replacement. it seems to work fine. Patch 3 and 4 can be applied without hunk. ==
From b743fc6a0a449efa738638e165b139afab91439f Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 1 Apr 2011 16:02:31 +0900 Subject: [PATCH 2/2] libvirt/qemu - rollback for 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, that implies there is inconsistency between XML definition in file and vmdef cached in memory. So, it's better to have some rollback method. Implementing undo in each stage doesn't seem very neat, this patch implements a generic one. This patch adds virDomainObjCopyPersistentDef(). This will create a copy of persistent def. The caller 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 | 8 +++++++- 4 files changed, 29 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6aaf33..31641f1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9368,3 +9368,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 10e73cb..20bb4ed 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 65a86d3..ec69967 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 b89bc8f..96c4df2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3952,7 +3952,7 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, goto endjob; } - vmdef = virDomainObjGetPersistentDef(driver->caps, vm); + vmdef = virDomainObjCopyPersistentDef(driver->caps, vm); if (!vmdef) goto endjob; @@ -3973,6 +3973,12 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, if (!ret) ret = virDomainSaveConfig(driver->configDir, vmdef); + /* If successfully saved the new vmdef, update it. this never fails */ + if (!ret) + virDomainObjAssignDef(vm, vmdef, false); + else /* discard */ + virDomainDefFree(vmdef); + virDomainDeviceDefFree(device); endjob: -- 1.7.4.1
From 96e42331674c9de4640422dd6f9e044319d9f700 Mon Sep 17 00:00:00 2001 From: root <root@bluextal.(none)> Date: Fri, 1 Apr 2011 11:38:49 +0900 Subject: [PATCH 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@jp.fujitsu.com> Changelog: v7->v8 - adjusted to use Copy/Sync/Undo mechanism. --- 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 098face..8948ab0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4877,6 +4877,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) { @@ -4948,6 +4961,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 28b3c0e..fc46700 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 62643d4..e8c5dce 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 08055ef..eeaea0c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3884,8 +3884,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; + } + /* It's inserted. CopySync() or CopyUndo() 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")); @@ -3898,7 +3918,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
From a87f4804dcea2ede3a20bb3d647e291c4bd9a579 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 1 Apr 2011 11:59:32 +0900 Subject: [PATCH 4/4] libvirt/qemu - check address conflict at attach.
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. [Before Patch] error: Failed to attach device from /home/kamezawa/testc.xml error: An error occurred, but the cause is unknown [After Patch] error: Failed to attach device from /home/kamezawa/testc.xml error: invalid argument in device address conflict This error report is better. And this aslo checks devices other and PCI devides, which wasn't done. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Changelog v7->v8 - fixed error messages. - use STREQ. --- 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 8948ab0..032ad1e 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 fc46700..1124e73 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 e8c5dce..d539011 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 eeaea0c..4d4b7e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3886,6 +3886,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
participants (2)
-
KAMEZAWA Hiroyuki -
Wen Congyang