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