[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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 06/13] qobject: Add helper macros for common scalar insertions, (continued)
[Qemu-devel] [PATCH v3 07/13] block: Use simpler QDict/QList scalar insertion macros, Eric Blake, 2017/04/05
[Qemu-devel] [PATCH v3 13/13] test-qga: Actually test 0xff sync bytes, Eric Blake, 2017/04/05
[Qemu-devel] [PATCH v3 12/13] fdc-test: Avoid deprecated 'change' command, Eric Blake, 2017/04/05
[Qemu-devel] [PATCH v3 09/13] qobject: Use simpler QDict/QList scalar insertion macros, Eric Blake, 2017/04/05