From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfjX-0005JG-2z for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:46:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtfjR-0002Qa-OS for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:46:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39356) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfjR-0002QM-Hc for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:46:21 -0500 Message-ID: <547603D4.7040201@redhat.com> Date: Wed, 26 Nov 2014 09:46:12 -0700 From: Eric Blake MIME-Version: 1.0 References: <1416487488-8423-1-git-send-email-mreitz@redhat.com> <1416487488-8423-2-git-send-email-mreitz@redhat.com> <5475FEC0.3090306@redhat.com> <54760175.8090100@redhat.com> In-Reply-To: <54760175.8090100@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="nerqNQgdt7XA9XkVCkUt576N6lDHf53uW" Subject: Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Markus Armbruster , Stefan Hajnoczi , Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --nerqNQgdt7XA9XkVCkUt576N6lDHf53uW Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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, &er= r); >>> + >>> + if (err) { >>> + if (read_only =3D=3D BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) { >>> + error_free(err); >>> + err =3D NULL; >>> + >>> + /* RDWR did not work, try RO now */ >>> + bdrv_flags &=3D ~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?= >=20 > 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 stra= nge. 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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --nerqNQgdt7XA9XkVCkUt576N6lDHf53uW Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUdgPUAAoJEKeha0olJ0Nqa7kH/2qcya3A75cmj2AVmuKXzx9d ZCm4rpr705sTTaL9gEtRMu3Oo+9ECXRJwQjlE2tOUvdOdIUA0ffbLmplwz5QN0QR OhWt6VmwDlielz3RFnVeVSTvpIha6L9q5Drnt+90e1kzJ33pmlT91l3T4JEoGGJ+ mHwKgqVUJwxS0jO3bijNixFi2cV3pGKS7tUXQbeHQ3Cqu5jp28vhbSLu5pV97GQq hP4LeoLbSuTydiPMa0lsaJgwKL59quJLBjlqAXTfa0LKRtMel0HJgWj7XHvRTrid BRztwi2cFqxWaUkwbqFiYm1joxuj2YN+0pfqNPH+jurEgldA9D6wDee5gCKoJMo= =ADvr -----END PGP SIGNATURE----- --nerqNQgdt7XA9XkVCkUt576N6lDHf53uW--