From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NWpCi-00056k-0v for qemu-devel@nongnu.org; Mon, 18 Jan 2010 05:51:28 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NWpCe-00051y-6N for qemu-devel@nongnu.org; Mon, 18 Jan 2010 05:51:27 -0500 Received: from [199.232.76.173] (port=34834 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NWpCd-00051Z-KP for qemu-devel@nongnu.org; Mon, 18 Jan 2010 05:51:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7751) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NWpCd-0001Rx-5K for qemu-devel@nongnu.org; Mon, 18 Jan 2010 05:51:23 -0500 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0IApMrd008182 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 18 Jan 2010 05:51:22 -0500 Date: Mon, 18 Jan 2010 12:48:17 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE. Message-ID: <20100118104816.GC5874@redhat.com> References: <1263739695-13043-1-git-send-email-nsprei@redhat.com> <1263739695-13043-2-git-send-email-nsprei@redhat.com> <1263739695-13043-3-git-send-email-nsprei@redhat.com> <20100117153202.GC3420@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Naphtali Sprei , qemu-devel@nongnu.org On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote: > >> Instead of using the field 'readonly' of the BlockDriverState struct for passing the request, > >> pass the request in the flags parameter to the function. > >> > >> Signed-off-by: Naphtali Sprei > > > > Many changes seem to be about passing 0 to bdrv_open. This is not what > > the changelog says the patch does. Better split unrelated changes to a > > separate patch. > > > > One of the things you seem to do is get rid of BDRV_O_RDONLY. Why is > > this an improvement? Symbolic name like BDRV_O_RDONLY seems better than > > 0. > > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT. We don't have > BDRV_DONT_SNAPSHOT, either. Well, this just mirros the file access macros: we have RDONLY, WRONLY and RDRW. I assume this similarity is just historical? > The old code can't quite devide whether BDRV_O_RDWR is a flag, or > whether to use bits BDRV_O_ACCESS for an access mode, with possible > values BDRV_O_RDONLY and BDRV_O_RDWR. I asked Naphtali to clean this > up, and recommended to go with flag rather than access mode: > > In my opinion, any benefit in readability you might hope gain by > having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you > need to keep knowledge of its encoding out of its users. > > http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html > > [...] > >> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv) > >> return 0; > >> } > >> action = SNAPSHOT_LIST; > >> + bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */ > > > > bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need > > for comment then? > > BDRV_O_RDWR is a flag, and this is the clean way to clear it. > > "bdrv_oflags = BDRV_O_RDONLY" assumes that everything but the access > mode in bdrv_oflags is clear. Tolerable, because the correctness > argument is fairly local, but the clean way to do it would be > > bdrv_oflags = (bdrv_oflags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY; > > That's what I meant by "tortuous bit twiddling". > > [...] Thinking about it, /* no need for RW */ comment can just go. But other places in code just do flags = 0 maybe they should all do &= ~BDRV_O_RDWR? I don't really have an opinion here but I do think this patch needs a better commit log (all it says "pass the request in the flags parameter to the function") and be split up: patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS patch 2 - pass the request in the flags parameter to the function patch 3 - any other fixups As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as well, and it's hard to see why. -- MST