At 04/01/2011 12:17 PM, KAMEZAWA Hiroyuki Write: This comment can be removed in patch 2 as we use a copy of XML definition.

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; 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(a)jp.fujitsu.com&gt; --- 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

On Fri, 01 Apr 2011 14:33:23 +0800 Wen Congyang <wency(a)cn.fujitsu.com&gt; wrote: Okay, here is a replacement. it seems to work fine. Patch 3 and 4 can be applied without hunk. == From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; 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(a)jp.fujitsu.com&gt; --- 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: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; 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(a)jp.fujitsu.com&gt; 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