Re: [libvirt] [PATCHv8 2/4] libvirt/qemu - synchronous update of device definition

>From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001 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);