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 <ebl...@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
