On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa <pkrempa(a)redhat.com> wrote:
> @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
> if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
> goto cleanup;
>
> - target->backingStore->format = backingStoreFormat;
> + backingStore = virStorageSourceGetBackingStore(target, 0);
> + backingStore->format = backingStoreFormat;
>
> /* XXX: Remote storage doesn't play nicely with volumes backed by
> * remote storage. To avoid trouble, just fake the backing store is RAW
> * and put the string from the metadata as the path of the target. */
> - if (!virStorageSourceIsLocalStorage(target->backingStore)) {
> - virStorageSourceFree(target->backingStore);
> + if (!virStorageSourceIsLocalStorage(backingStore)) {
> + virStorageSourceFree(backingStore);
So this frees the new backingStore variable, which also corresponds to
target->backingStore at this point, but ...
>
> - if (VIR_ALLOC(target->backingStore) < 0)
> + if (VIR_ALLOC(backingStore) < 0)
... here only the local copy is allocated, so target->backingStore
contains an invalid pointer ...
> goto cleanup;
>
> - target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> - target->backingStore->path = meta->backingStoreRaw;
> + target->backingStore = backingStore;
> + backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> + backingStore->path = meta->backingStoreRaw;
> meta->backingStoreRaw = NULL;
> - target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + backingStore->format = VIR_STORAGE_FILE_RAW;
> }
>
> - if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) {
> - if ((rc = virStorageFileProbeFormat(target->backingStore->path,
> + if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> + if ((rc = virStorageFileProbeFormat(backingStore->path,
> -1, -1)) < 0) {
> /* If the backing file is currently unavailable or is
> * accessed via remote protocol only log an error, fake the
> * format as RAW and continue. Returning -1 here would
> * disable the whole storage pool, making it unavailable for
> * even maintenance. */
> - target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + backingStore->format = VIR_STORAGE_FILE_RAW;
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot probe backing volume format:
%s"),
> - target->backingStore->path);
> + backingStore->path);
> } else {
> - target->backingStore->format = rc;
> + backingStore->format = rc;
... and I couldn't find a place where you update it back.
> - target->backingStore->type =
VIR_STORAGE_TYPE_NETWORK;
> - target->backingStore->path = meta->backingStoreRaw;
> + target->backingStore = backingStore;
I do it here.
Still, you have a good point about temp variables, and long line,
I'm working on this now.
Thank you for the review.