[libvirt] [PATCH 05/10] util: storagefile: Add deep copy for struct virStorageSource

[Eric Blake]

On 06/19/2014 07:46 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 | 143 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstoragefile.h |   3 +
>  3 files changed, 147 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index acb2ea3..1c84777 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1898,6 +1898,7 @@ virStorageNetProtocolTypeToString;
>  virStorageSourceAuthClear;
>  virStorageSourceClear;
>  virStorageSourceClearBackingStore;
> +virStorageSourceCopy;
>  virStorageSourceCopySeclabels;

Ah, back to my naming choices in 4/10 - observe how we have noun-verb in
virStorageSourceAuthClear (clear the StorageSourceAuth), but
object-verb-noun on virStorageSourceClearBackingStore (take the Storage
Source, and clear its backingStore).  It looks weird to have two styles,
but at this point, it's a global search-and-replace pre-req patch if we
want to make those two names consistent (if you go with
virStorageSourceSeclabelsCopy, then it is the backingStore variant that
needs the rename).

> 
> +static virStorageTimestampsPtr
> +virStorageTimestampsCopy(const virStorageTimestamps *src)

And here you did noun-verb, not object-verb-noun :)

> +
> +static virStorageSourcePoolDefPtr
> +virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)

And again here.  Looks like we have our majority-rules style.

> +virStorageSourcePtr
> +virStorageSourceCopy(const virStorageSource *src,
> +                     bool backingChain)
> +{
> +    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;
> +
> +    /* storage driver metadata are not copied */
> +    ret->drv = NULL;

Dead assignment given the VIR_ALLOC above; maybe you could just fold
that into the /* comment */

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

Dan has at times persuaded me that breaking this into one 'if' per
VIR_STRDUP makes it easier to tell _which_ strdup failed when stepping
through gdb execution; on the other hand, OOM is so unlikely to be the
cause of a debug session, and you aren't mixing in any complex actions
alongside the VIR_STRDUP, so I can live with this compressed form.  I
guess it's a judgment call of how complex each individual step is on
whether it makes sense to make it easier to put breakpoints on a step
and check intermediate state.


> +    if (virStorageSourceCopySeclabels(ret, src) < 0)
> +        goto error;

Huh, I wonder if we should make the seclabels contents a standalone
struct, where we can manipulate it in isolation instead of always having
to be part of a virStorageSourcePtr.

What you have works; you may want to fold in my nits above, or may have
to rebase this on top of changes in 4/10, but for now I'm okay with:

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/20140619/6e6f0c4b/attachment-0001.sig>