Re: [Qemu-block] [PATCH v3 10/13] block: Simplify bdrv_append_temp_snapshot() logic

[Eric Blake]

On 04/06/2017 08:27 AM, Kevin Wolf wrote:

>>>> @@ -2209,13 +2209,10 @@ static BlockDriverState 
>>>> *bdrv_append_temp_snapshot(BlockDriverState *bs,

More context:

>     bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp);
>     snapshot_options = NULL;
>     if (!bs_snapshot) {
>         ret = -EINVAL;
>         goto out;
>     }

Up to here, bs_snapshot is NULL if we return.  What is the deal with
ret? Setting it to -EINVAL has no purpose, as we don't reference ret in
the out label.

> 
>     /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will
>      * call bdrv_unref() on it), so in order to be able to return one, we have
>      * to increase bs_snapshot's refcount here */
>     bdrv_ref(bs_snapshot);
>     bdrv_append(bs_snapshot, bs, &local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         ret = -EINVAL;
>         goto out;
>     }

Again, what's with setting ret? But you are right that this is a place
where bs_snapshot would be non-null when we want to return an error.
The goto out looks a bit funny with no further code (although it's okay
to leave it as a defensive measure for code added later, as that's what
bdrv_append() does).

>>>>
>>>> +out:
>>>> +    QDECREF(snapshot_options);
>>>>      g_free(tmp_filename);
>>>>      return bs_snapshot;
>>>
>>> bs_snapshot is uninitialised or at least the wrong return value in error
>>> cases. (Hm... Shouldn't the compiler catch the uninitialised part?)
>>
>> Odd, and I agree that I recall the compiler generally able to catch that
>> (maybe it's a matter of compiling with -g vs. -O2).  I'm surprised the
>> autobuilder didn't flag it.  (I think I missed it due to rebase churn on
>> my end).  The obvious fix is to:
>>
>> -    BlockDriverState *bs_snapshot;
>> +    BlockDriverState *bs_snapshot = NULL;
> 
> That's just half of the fix. It doesn't fix the cases where bs_snapshot
> is already set. We still want to return NULL on errors there.

Thanks for calling it to my attention.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature