[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] Switch to bit-flags based read-only drive o
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] Switch to bit-flags based read-only drive option implementation |
Date: |
Thu, 14 Jan 2010 16:29:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091209 Fedora/3.0-3.fc11 Thunderbird/3.0 |
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 <address@hidden>
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