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