[libvirt] [PATCH v6 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store

[Matthias Gatto]

On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa <pkrempa at 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.