At 04/01/2011 12:19 PM, KAMEZAWA Hiroyuki Write:
>>From 229cfc90781bfd7024f79db1aed8bea5963757e3 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> ---
> 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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list