[libvirt] [RFC PATCH 4/5] util: storagefile: Add deep copy for struct virStorageSource

[Eric Blake]

On 06/12/2014 09:02 AM, Peter Krempa wrote:
> Now that we have pointers to store disk source information and thus can
> easily exchange the structs behind we need a function to copy all the
> data.
> ---
>  src/libvirt_private.syms  |   1 +
>  src/util/virstoragefile.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstoragefile.h |   2 +
>  3 files changed, 153 insertions(+)
> 

> +virStorageSourcePtr
> +virStorageSourceCopy(const virStorageSource *src,
> +                     bool backingChain)
> +{
> +    size_t i;
> +    virStorageSourcePtr ret = NULL;
> +
> +    if (VIR_ALLOC(ret) < 0)
> +        return NULL;
> +
> +    ret->type = src->type;
> +    ret->protocol = src->protocol;
> +    ret->format = src->format;
> +    ret->allocation = src->allocation;
> +    ret->capacity = src->capacity;
> +    ret->backingRelative = src->backingRelative;

If I'm not mistaken, backingRelative disappears in your other series;
I'm guessing you plan on rebasing that on top of this to pick up on the
easier semantics.

> +
> +    /* storage driver metadata are not copied */
> +    ret->drv = NULL;

Seems okay, particularly since doing a deep copy of that would require
callback functions in the storage drivers.

> +
> +    if (VIR_STRDUP(ret->path, src->path) < 0 ||
> +        VIR_STRDUP(ret->volume, src->volume) < 0 ||
> +        VIR_STRDUP(ret->driverName, src->driverName) < 0 ||
> +        VIR_STRDUP(ret->relPath, src->relPath) < 0 ||
> +        VIR_STRDUP(ret->relDir, src->relDir) < 0 ||
> +        VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 ||
> +        VIR_STRDUP(ret->compat, src->compat) < 0 ||
> +        VIR_STRDUP(ret->auth.username, src->auth.username) < 0)
> +        goto error;
...
> +
> +    ret->auth.secretType = src->auth.secretType;
> +    switch ((virStorageSecretType) src->auth.secretType) {
> +        case VIR_STORAGE_SECRET_TYPE_NONE:
> +        case VIR_STORAGE_SECRET_TYPE_LAST:
> +            break;

It might be worth adding an intermediate patch to break out the embedded
auth struct into a formal top-level struct, then add a function that
copies an auth struct, rather than having to inline all the nested
struct work here.  But for now I can live with it.

> +    if (backingChain && src->backingStore) {
> +        if (!(ret->backingStore = virStorageSourceCopy(src->backingStore,
> +                                                       true)))
> +            goto error;
> +    }

Nice - we have the choice of shallow or deep chain copy.

Random thought - I wonder if it would be better to make
virStorageSourcePtr inherit from virObject, and make it
reference-counted, so that we can share a single object among multiple
users rather than copying users right and left.  But even if it is a
good idea, it's enough refactoring that I don't think we need to tackle
it right away.

> +
> +    ret->nseclabels = src->nseclabels;
> +    if (VIR_ALLOC_N(ret->seclabels, ret->nseclabels) < 0)
> +        goto error;

Maybe consider swapping those two lines.  Unlike my comment in patch
1/5, in _this_ case, we were careful enough that virStorageSourceClear
checks seclabels for non-NULL before paying attention to nseclabels.
But as we are not consistently careful in the clear/free functions, it's
more robust to just get in the habit of setting the count after the
allocation is successful without having to check a particular free
function.  Hmm - do I dare suggest simplifying virStorageSourceClear to
unconditionally iterate over nseclabels instead of hiding it in an 'if
(def->seclabels)' block?  Or is it better form to suggest that all
cleanup functions be audited to be robust to non-zero count but NULL arrays?

> +++ b/src/util/virstoragefile.h
> @@ -324,6 +324,8 @@ int virStorageSourceGetActualType(virStorageSourcePtr def);
>  void virStorageSourceFree(virStorageSourcePtr def);
>  void virStorageSourceClearBackingStore(virStorageSourcePtr def);
>  virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent);
> +virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src,
> +                                         bool backingChain);

ATTRIBUTE_NONNULL(1)

ACK

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140612/8e3e105b/attachment-0001.sig>