[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 10/13] block: Simplify bdrv_append_temp_snaps

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v3 10/13] block: Simplify bdrv_append_temp_snapshot() logic
Date: Thu, 6 Apr 2017 08:41:20 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]