From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:45484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJMzS-00044F-06 for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:15:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RJMzQ-0008IM-R1 for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:15:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15879) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJMzQ-0008IB-Ip for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:15:12 -0400 Message-ID: <4EA92FE8.6080003@redhat.com> Date: Thu, 27 Oct 2011 12:18:16 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1319709268-7435-1-git-send-email-stefanha@linux.vnet.ibm.com> <1319709268-7435-2-git-send-email-stefanha@linux.vnet.ibm.com> In-Reply-To: <1319709268-7435-2-git-send-email-stefanha@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org Am 27.10.2011 11:54, schrieb Stefan Hajnoczi: > Several block drivers set bs->read_only in .bdrv_open() but > block.c:bdrv_open_common() clobbers its value. Additionally, QED uses > bdrv_is_read_only() in .bdrv_open() to decide whether to perform > consistency checks. > > The correct ordering is to initialize bs->read_only from the open flags > before calling .bdrv_open(). This way block drivers can override it if > necessary and can use bdrv_is_read_only() in .bdrv_open(). > > Signed-off-by: Stefan Hajnoczi > --- > block.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 70aab63..3207e99 100644 > --- a/block.c > +++ b/block.c > @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > open_flags |= BDRV_O_RDWR; > } Not directly related, but the context made me wonder when we're making a BlockkDriverState writeable unconditionally. This is the full context: /* * Snapshots should be writable. */ if (bs->is_temporary) { open_flags |= BDRV_O_RDWR; } Does anyone understand what the point of this is? If the user requested read-only, he certainly wants to have read-only, even if he specified -snapshot as well. > + bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); > + > /* Open the image, either directly or using a protocol */ > if (drv->bdrv_file_open) { > ret = drv->bdrv_file_open(bs, filename, open_flags); > @@ -514,8 +516,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > goto free_and_fail; > } > > - bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); > - The assignment was already at the new place before 4dca4b6. Not sure if there was any real reason for moving it, though. Kevin