Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

On 11/26/2014 09:36 AM, Max Reitz wrote:

>>>   -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL,
>>> errp);
>>> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
>>> +
>>> +    if (err) {
>>> +        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
>>> +            error_free(err);
>>> +            err = NULL;
>>> +
>>> +            /* RDWR did not work, try RO now */
>>> +            bdrv_flags &= ~BDRV_O_RDWR;
>>> +            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv,
>>> NULL, errp);
>>> +        } else {
>>> +            error_propagate(errp, err);
>>> +        }
>> Umm, why are you propagating the error here manually, when it was
>> previously propagated as part of the fall-through into the out: label?
> 
> Is it? I don't see any error_propagate() after that
> qmp_bdrv_open_encrypted_call()... And also, that call takes "errp" as a
> parameter, so having error_propagate() afterwards would be kind of strange.

Oh, I read too fast; I'm missing the difference between '&err' and
'errp'.  I think we generally use the name 'local_err' instead of plain
'err', so that it is more obvious when we are collecting into
'&local_err' with a need to propagate, vs. reusing the caller's 'errp'
directly because we aren't further checking things.

Okay, your code is correct after all.  The pre-existing confusion of
'err' instead of 'local_err' might be worth fixing if you have a reason
for a respin, but is not itself a reason for me to withhold approval.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]