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
