[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bit
From: |
Naphtali Sprei |
Subject: |
[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. |
Date: |
Mon, 18 Jan 2010 13:32:05 +0200 |
User-agent: |
Thunderbird 2.0.0.23 (X11/20090817) |
Michael S. Tsirkin wrote:
> 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 <address@hidden>
>
> 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.
Thanks for the review.
Unfortunately, some of my comments went to the subject and are not part of the
changelog.
So here's the (intended) changelog. This will clear-up some of your comments.
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.
>
> 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.
See Markus reply (thanks Markus).
>
>> ---
>> block.c | 31 +++++++++++++++++--------------
>> block.h | 2 --
>> block/raw-posix.c | 2 +-
>> block/raw-win32.c | 4 ++--
>> block/vmdk.c | 9 +++++----
>> hw/xen_disk.c | 2 +-
>> monitor.c | 2 +-
>> qemu-img.c | 10 ++++++----
>> qemu-io.c | 14 ++++++--------
>> qemu-nbd.c | 2 +-
>> vl.c | 8 ++++----
>> 11 files changed, 44 insertions(+), 42 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 115e591..8def3c4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char
>> *filename)
>> if (drv && strcmp(drv->format_name, "vvfat") == 0)
>> return drv;
>>
>> - ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
>> + ret = bdrv_file_open(&bs, filename, 0);
>> if (ret < 0)
>> return NULL;
>> ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>> @@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char
>> *filename, int flags)
>> int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>> BlockDriver *drv)
>> {
>> - int ret, open_flags, try_rw;
>> + int ret, open_flags;
>> char tmp_filename[PATH_MAX];
>> char backing_filename[PATH_MAX];
>>
>> @@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char
>> *filename, int flags,
>>
>> /* Note: for compatibility, we open disk image files as RDWR, and
>> RDONLY as fallback */
>> - try_rw = !bs->read_only || bs->is_temporary;
>> - if (!(flags & BDRV_O_FILE))
>> - open_flags = (try_rw ? BDRV_O_RDWR : 0) |
>> - (flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
>> - else
>> + bs->read_only = (flags & BDRV_O_RDWR) == 0;
>
> !(flags & BDRV_O_RDWR) is a standard idiom, and it's more readable IMO.
>
>> + if (!(flags & BDRV_O_FILE)) {
>> + open_flags = (flags & (BDRV_O_RDWR |
>> BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
>> + if (bs->is_temporary) { /* snapshot should be writeable */
>> + open_flags |= BDRV_O_RDWR;
>> + }
>> + } else {
>> open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
>> - if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
>> + }
>> + if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
>> ret = -ENOTSUP;
>> - else
>> + } else {
>> ret = drv->bdrv_open(bs, filename, open_flags);
>> - if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
>> - ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
>> - bs->read_only = 1;
>> + if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
>> + ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
>> + bs->read_only = 1;
>> + }
>> }
>> if (ret < 0) {
>> qemu_free(bs->opaque);
>> @@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char
>> *filename, int flags,
>> /* if there is a backing file, use it */
>> BlockDriver *back_drv = NULL;
>> bs->backing_hd = bdrv_new("");
>> - /* pass on read_only property to the backing_hd */
>> - bs->backing_hd->read_only = bs->read_only;
>> path_combine(backing_filename, sizeof(backing_filename),
>> filename, bs->backing_file);
>> if (bs->backing_format[0] != '\0')
>> back_drv = bdrv_find_format(bs->backing_format);
>> ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
>> back_drv);
>> + bs->backing_hd->read_only = (open_flags & BDRV_O_RDWR) == 0;
>
> !(open_flags & BDRV_O_RDWR) is a standard idiom and it's more readable
> IMO.
Sorry, I prefer the more verbose style. The extra bytes on me. Seems more
readable for me.
> Further, pls don't put two spaces after ==.
Sure, will correct.
>
>> if (ret < 0) {
>> bdrv_close(bs);
>> return ret;
>> diff --git a/block.h b/block.h
>> index bee9ec5..fd4e8dd 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
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 5a6a22b..b427211 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -138,7 +138,7 @@ static int raw_open_common(BlockDriverState *bs, const
>> char *filename,
>>
>> s->open_flags = open_flags | O_BINARY;
>> s->open_flags &= ~O_ACCMODE;
>> - if ((bdrv_flags & BDRV_O_ACCESS) == BDRV_O_RDWR) {
>> + if (bdrv_flags & BDRV_O_RDWR) {
>> s->open_flags |= O_RDWR;
>> } else {
>> s->open_flags |= O_RDONLY;
>> diff --git a/block/raw-win32.c b/block/raw-win32.c
>> index 72acad5..01a6d25 100644
>> --- a/block/raw-win32.c
>> +++ b/block/raw-win32.c
>> @@ -81,7 +81,7 @@ static int raw_open(BlockDriverState *bs, const char
>> *filename, int flags)
>>
>> s->type = FTYPE_FILE;
>>
>> - if ((flags & BDRV_O_ACCESS) == O_RDWR) {
>> + if (flags & BDRV_O_RDWR) {
>> access_flags = GENERIC_READ | GENERIC_WRITE;
>> } else {
>> access_flags = GENERIC_READ;
>> @@ -337,7 +337,7 @@ static int hdev_open(BlockDriverState *bs, const char
>> *filename, int flags)
>> }
>> s->type = find_device_type(bs, filename);
>>
>> - if ((flags & BDRV_O_ACCESS) == O_RDWR) {
>> + if (flags & BDRV_O_RDWR) {
>> access_flags = GENERIC_READ | GENERIC_WRITE;
>> } else {
>> access_flags = GENERIC_READ;
>
> The above changes seem nothing to do with passing in parameter to the
> function. If the are intentional, pls mention them in changelog or split
> to a separate patch.
Correct, see my preface.
>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 4e48622..ddc2fcb 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, no RDWR. */
>> + flags = 0;
>> + }
>>
>> 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..5bb8872 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_RDWR, drv);
>
> This and below seems to change file from rdwr to readonly.
> Intentional?
Yes, intentioanl. The default used to be read-write, even when passed nothing,
see old code citation from bdrv_open2:
======BEGIN======
/* Note: for compatibility, we open disk image files as RDWR, and
RDONLY as fallback */
if (!(flags & BDRV_O_FILE))
open_flags = BDRV_O_RDWR | (flags & BDRV_O_CACHE_MASK);
======END======
Now you need to explicitly ask what you want.
>
>> monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>> }
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 48b9315..3cea8ce 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_RDWR, 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) {
>> error("Could not open '%s'", filename);
>> }
>> ret = bdrv_commit(bs);
>> @@ -966,10 +966,11 @@ static int img_snapshot(int argc, char **argv)
>> BlockDriverState *bs;
>> QEMUSnapshotInfo sn;
>> char *filename, *snapshot_name = NULL;
>> - int c, ret;
>> + int c, ret, bdrv_oflags;
>> int action = 0;
>> qemu_timeval tv;
>>
>> + bdrv_oflags = BDRV_O_RDWR;
>> /* Parse commandline parameters */
>> for(;;) {
>> c = getopt(argc, argv, "la:c:d:h");
>> @@ -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?
I had some thoughts about that. I added just few lines above the assignment:
'bdrv_oflags = BDRV_O_RDWR;'
so I could just overwrite it with:
'bdrv_oflags = 0; ' /* no BDRV_O_RDONLY anymore */
but thought that's clearer.
>
>> break;
>> case 'a':
>> if (action) {
>> @@ -1022,7 +1024,7 @@ static int img_snapshot(int argc, char **argv)
>> if (!bs)
>> error("Not enough memory");
>>
>> - if (bdrv_open2(bs, filename, 0, NULL) < 0) {
>> + if (bdrv_open2(bs, filename, bdrv_oflags, NULL) < 0) {
>> error("Could not open '%s'", filename);
>> }
>>
>> 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;
>> + }
>>
>> 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;
>> + }
>
> alignment seems broken in 2 places above
Sure, will fix (both).
>
>>
>> if ((argc - optind) == 1)
>> openfile(argv[optind], flags, growable);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 6707ea5..4463679 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_RDWR;
>> int partition = -1;
>> int ret;
>> int shared = 1;
>> diff --git a/vl.c b/vl.c
>> index 76ef8ca..eee59dd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2227,19 +2227,19 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>> }
>>
>> if (ro == 1) {
>> - if (type == IF_IDE) {
>> - fprintf(stderr, "qemu: readonly flag not supported for drive
>> with ide interface\n");
>> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) {
>
> So each new user will have to be added here? Any idea how todo this
> better? Can't relevant drive emulation check read_only flag and report
> an error? Or set a flag in drive info declaring readonly support.
Right. Second options seems better to me. Will do.
>
>> + fprintf(stderr, "qemu: readonly flag not supported for drive
>> with this interface\n");
>> return NULL;
>> }
>> - (void)bdrv_set_read_only(dinfo->bdrv, 1);
>> }
>> /*
>> * cdrom is read-only. Set it now, after above interface checking
>> * since readonly attribute not explicitly required, so no error.
>> */
>> if (media == MEDIA_CDROM) {
>> - (void)bdrv_set_read_only(dinfo->bdrv, 1);
>> + ro = 1;
>> }
>> + bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>
>> if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
>> fprintf(stderr, "qemu: could not open disk image %s: %s\n",
>> --
>> 1.6.3.3
>>
>>
#
- 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., (continued)
- 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., Markus Armbruster, 2010/01/18
- 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., Michael S. Tsirkin, 2010/01/18
- 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., Markus Armbruster, 2010/01/18
- 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., Jamie Lokier, 2010/01/19
- 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., Markus Armbruster, 2010/01/20
- 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., Michael S. Tsirkin, 2010/01/20
- 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., Markus Armbruster, 2010/01/20
- 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., Michael S. Tsirkin, 2010/01/20
- 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., Markus Armbruster, 2010/01/20
- 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., Jamie Lokier, 2010/01/20
- [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.,
Naphtali Sprei <=
Re: [Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive, Jamie Lokier, 2010/01/19
Re: [Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive, Anthony Liguori, 2010/01/20
Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute, Christoph Hellwig, 2010/01/20