On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa <pkrempa@xxxxxxxxxx> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list