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