Re: [libvirt] [RFC PATCH 4/5] util: storagefile: Add deep copy for struct virStorageSource
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)