[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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);
>  


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]