From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:43223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJNXQ-0002II-72 for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:50:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RJNXJ-0006Pn-Ft for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:50:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3031) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJNXJ-0006Pa-5O for qemu-devel@nongnu.org; Thu, 27 Oct 2011 06:50:13 -0400 Message-ID: <4EA9381C.8050901@redhat.com> Date: Thu, 27 Oct 2011 12:53: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> <4EA92FE8.6080003@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 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: Stefan Hajnoczi , qemu-devel@nongnu.org Am 27.10.2011 12:45, schrieb Stefan Hajnoczi: > On Thu, Oct 27, 2011 at 11:18 AM, Kevin Wolf wrote: >> 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. > > Perhaps this is an attempt to support -drive > file=pristine.img,readonly=on,snapshot=on. The idea being that the > user absolutely wants to keep pristine.img unmodified. But the nature > of backing files means we should automatically get this. I would have said that it breaks this command line. It all depends on what your expectation of the semantics of these options is. Mine would be that the disk is presented read-only to the guest (and the snapshot is done but useless). Kevin