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