From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfZk-0008GB-U1 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:36:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtfZg-0007MS-Jh for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:36:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfZg-0007MF-5D for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:36:16 -0500 Message-ID: <54760175.8090100@redhat.com> Date: Wed, 26 Nov 2014 17:36:05 +0100 From: Max Reitz 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> In-Reply-To: <5475FEC0.3090306@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Markus Armbruster , Stefan Hajnoczi , Luiz Capitulino On 2014-11-26 at 17:24, Eric Blake wrote: > On 11/20/2014 05:44 AM, Max Reitz wrote: >> Add an option to qmp_change_blockdev() which allows changing the >> read-only status of the block device to be changed. >> >> Some drives do not have a inherently fixed read-only status; for >> instance, floppy disks can be set read-only or writable independently of >> the drive. Some users may find it useful to be able to therefore change >> the read-only status of a block device when changing the medium. >> >> Signed-off-by: Max Reitz >> --- >> blockdev.c | 41 ++++++++++++++++++++++++++++++++++++++--- >> include/sysemu/blockdev.h | 3 ++- >> qapi-schema.json | 20 ++++++++++++++++++++ >> qmp.c | 3 ++- >> 4 files changed, 62 insertions(+), 5 deletions(-) >> >> >> - 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. The tree I'm looking at is master, commit 3ef4ebcc5c0360629bb05f49d9b961774247d6ca. In my block-next tree, which this patch is actually based against, commit 656ddfaafd67af2b9234567e51cd3418588b2ddd. > Particularly since the second open_encrypted call still relies on > fallthrough for propagating the error? I think this should be > simplified to: > > if (err && 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(...); > } But then you're not propagating the error anymore (in case of !err || read_only != BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO). As I said, at least in my tree, there is no error_propagate(errp, err); after this block. I may add it, though. (to be completely sure: http://git.qemu.org/?p=qemu.git;a=blob;f=blockdev.c;h=57910b82c7adc3ce59173afeeebcd37ff2a3dfd0;hb=3ef4ebcc5c0360629bb05f49d9b961774247d6ca#l1727 and https://github.com/XanClic/qemu/blob/block-next/blockdev.c#L1760) Max > Otherwise, looks okay to me.