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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list