[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-bl
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev |
Date: |
Wed, 26 Nov 2014 09:46:12 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature