From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NVRf0-0007DI-MG for qemu-devel@nongnu.org; Thu, 14 Jan 2010 10:30:58 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NVRew-00078R-Nt for qemu-devel@nongnu.org; Thu, 14 Jan 2010 10:30:58 -0500 Received: from [199.232.76.173] (port=41898 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NVRew-00078G-Ei for qemu-devel@nongnu.org; Thu, 14 Jan 2010 10:30:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51709) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NVRev-0000In-RH for qemu-devel@nongnu.org; Thu, 14 Jan 2010 10:30:54 -0500 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0EFUqbT016937 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 14 Jan 2010 10:30:52 -0500 Message-ID: <4B4F3871.5080205@redhat.com> Date: Thu, 14 Jan 2010 16:29:53 +0100 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/3] Switch to bit-flags based read-only drive option implementation References: <4B4F2B32.6030704@redhat.com> In-Reply-To: <4B4F2B32.6030704@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Naphtali Sprei Cc: qemu-devel@nongnu.org Am 14.01.2010 15:33, schrieb Naphtali Sprei: > > 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. > > 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 I think this is a good change in general. Note that the block.c part is going to conflict with Christoph's cleanup patch from yesterday. You'll probably want to rebase on it. > diff --git a/block.h b/block.h > index bee9ec5..a37a605 100644 > --- a/block.h > +++ b/block.h > @@ -27,9 +27,7 @@ typedef struct QEMUSnapshotInfo { > uint64_t vm_clock_nsec; /* VM clock relative to boot */ > } QEMUSnapshotInfo; > > -#define BDRV_O_RDONLY 0x0000 > #define BDRV_O_RDWR 0x0002 > -#define BDRV_O_ACCESS 0x0003 > #define BDRV_O_CREAT 0x0004 /* create an empty file */ > #define BDRV_O_SNAPSHOT 0x0008 /* open the file read only and save writes in a snapshot */ > #define BDRV_O_FILE 0x0010 /* open as a raw file (do not try to > @@ -42,6 +40,8 @@ typedef struct QEMUSnapshotInfo { > #define BDRV_O_NO_BACKING 0x0100 /* don't open the backing file */ > > #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB) > +#define BDRV_O_DEFAULT_OPEN (BDRV_O_RDWR) > +#define BDRV_FLAGS_IS_RO(flags) ((flags & BDRV_O_RDWR) == 0) I think this introduces an inconsistency: All other flags are checked without such a macro. I think we should have it for all of them or for none (I think I'd prefer none). > diff --git a/block/vmdk.c b/block/vmdk.c > index 4e48622..7f866f9 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -357,7 +357,7 @@ static int vmdk_parent_open(BlockDriverState *bs, const char * filename) > return -1; > } > parent_open = 1; > - if (bdrv_open(bs->backing_hd, parent_img_name, BDRV_O_RDONLY) < 0) > + if (bdrv_open(bs->backing_hd, parent_img_name, 0) < 0) > goto failure; > parent_open = 0; > } > @@ -371,9 +371,10 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags) > uint32_t magic; > int l1_size, i, ret; > > - if (parent_open) > - // Parent must be opened as RO. > - flags = BDRV_O_RDONLY; > + if (parent_open) { > + /* Parent must be opened as RO, turn off RDWR bit. */ > + flags &= ~BDRV_O_RDWR; > + } I'm not familiar with the VMDK code, but to retain current behaviour you'd need to use flags = 0. Now I'm not sure if you're introducing or fixing a bug with your change (or maybe both at the same time). > ret = bdrv_file_open(&s->hd, filename, flags); > if (ret < 0) > diff --git a/hw/xen_disk.c b/hw/xen_disk.c > index 5c55251..beadf90 100644 > --- a/hw/xen_disk.c > +++ b/hw/xen_disk.c > @@ -613,7 +613,7 @@ static int blk_init(struct XenDevice *xendev) > qflags = BDRV_O_RDWR; > } else { > mode = O_RDONLY; > - qflags = BDRV_O_RDONLY; > + qflags = 0; > info |= VDISK_READONLY; > } > > diff --git a/monitor.c b/monitor.c > index b824e7c..0f2bdca 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -916,7 +916,7 @@ static void do_change_block(Monitor *mon, const char *device, > } > if (eject_device(mon, bs, 0) < 0) > return; > - bdrv_open2(bs, filename, 0, drv); > + bdrv_open2(bs, filename, BDRV_O_DEFAULT_OPEN, drv); > monitor_read_bdrv_key_start(mon, bs, NULL, NULL); > } > > diff --git a/qemu-img.c b/qemu-img.c > index 48b9315..ccfdc30 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -204,7 +204,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, > } else { > drv = NULL; > } > - if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) { > + if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_DEFAULT_OPEN, drv) < 0) { > error("Could not open '%s'", filename); > } > if (bdrv_is_encrypted(bs)) { > @@ -468,7 +468,7 @@ static int img_commit(int argc, char **argv) > } else { > drv = NULL; > } > - if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) { > + if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv) < 0) { Is there a reason why you chose to add DEFAULT_OPEN in one place and RDWR in the other one? > diff --git a/qemu-io.c b/qemu-io.c > index 1c19b92..b159bc9 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -1359,10 +1359,9 @@ open_f(int argc, char **argv) > } > } > > - if (readonly) > - flags |= BDRV_O_RDONLY; > - else > - flags |= BDRV_O_RDWR; > + if (!readonly) { > + flags |= BDRV_O_RDWR; > + } Indentation looks wrong. > > if (optind != argc - 1) > return command_usage(&open_cmd); > @@ -1506,10 +1505,9 @@ int main(int argc, char **argv) > add_check_command(init_check_command); > > /* open the device */ > - if (readonly) > - flags |= BDRV_O_RDONLY; > - else > - flags |= BDRV_O_RDWR; > + if (!readonly) { > + flags |= BDRV_O_RDWR; > + } Same here. > > if ((argc - optind) == 1) > openfile(argv[optind], flags, growable); > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 6707ea5..0f0e2fc 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -213,7 +213,7 @@ int main(int argc, char **argv) > int opt_ind = 0; > int li; > char *end; > - int flags = 0; > + int flags = BDRV_O_DEFAULT_OPEN; > int partition = -1; > int ret; > int shared = 1; > diff --git a/qemu-options.hx b/qemu-options.hx > index e2edd71..4617867 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -103,7 +103,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, > "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n" > " [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n" > " [,cache=writethrough|writeback|none][,format=f][,serial=s]\n" > - " [,addr=A][,id=name][,aio=threads|native]\n" > + " [,addr=A][,id=name][,aio=threads|native][,readonly=on|off]\n" > " use 'file' as a drive image\n") > DEF("set", HAS_ARG, QEMU_OPTION_set, > "-set group.id.arg=value\n" > diff --git a/vl.c b/vl.c > index 4f19505..6573bb9 100644 > --- a/vl.c > +++ b/vl.c > @@ -2210,6 +2210,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > *fatal_error = 0; > return NULL; > } > + > bdrv_flags = 0; > if (snapshot) { > bdrv_flags |= BDRV_O_SNAPSHOT; This hunk doesn't belong in the patch. Kevin