From patchwork Mon Dec 14 13:35:07 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naphtali Sprei X-Patchwork-Id: 41106 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A7EFEB7B3E for ; Tue, 15 Dec 2009 00:35:59 +1100 (EST) Received: from localhost ([127.0.0.1]:37466 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NKB5f-0000LM-OP for incoming@patchwork.ozlabs.org; Mon, 14 Dec 2009 08:35:55 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NKB52-0000Kk-El for qemu-devel@nongnu.org; Mon, 14 Dec 2009 08:35:16 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NKB4y-0000IG-NO for qemu-devel@nongnu.org; Mon, 14 Dec 2009 08:35:16 -0500 Received: from [199.232.76.173] (port=57363 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NKB4y-0000IA-IM for qemu-devel@nongnu.org; Mon, 14 Dec 2009 08:35:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53980) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NKB4x-0004Gy-Vr for qemu-devel@nongnu.org; Mon, 14 Dec 2009 08:35:12 -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 nBEDZADL030470 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 14 Dec 2009 08:35:10 -0500 Received: from [10.35.0.60] (dhcp-0-60.tlv.redhat.com [10.35.0.60]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id nBEDZ7eh028019 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 14 Dec 2009 08:35:09 -0500 Message-ID: <4B263F0B.90408@redhat.com> Date: Mon, 14 Dec 2009 15:35:07 +0200 From: Naphtali Sprei User-Agent: Thunderbird 2.0.0.23 (X11/20090817) MIME-Version: 1.0 To: qemu-devel@nongnu.org X-Scanned-By: MIMEDefang 2.67 on 10.5.11.18 X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. Subject: [Qemu-devel] [PATCH] A different way to ask for readonly drive X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi, After feedback from Red Hat guys, I've decided to slightly modify the approach to drive's readonly. The new approach also addresses the silent fall-back to open the drives' file as read-only when read-write fails (permission denied) that causes unexpected behavior. Instead of the 'readonly' boolean flag, another flag introduced (a replacement), 'read_write' with three values [on|off|try]: read_write=on : open with read and write permission, no fall-back to read-only read_write=off: open with read-only permission read_write=try: open with read and write permission and if fails, fall-back to read-only (the default if nothing specified) Suggestions for better naming for flag/values welcomed. I've tried to explicitly pass the required flags for the bdrv_open function in callers, but probably missed some. Naphtali Signed-off-by: Naphtali Sprei --- block.c | 29 +++++++++++++++++------------ block.h | 7 +++++-- hw/xen_disk.c | 3 ++- monitor.c | 2 +- qemu-config.c | 4 ++-- qemu-img.c | 14 ++++++++------ qemu-nbd.c | 2 +- vl.c | 42 +++++++++++++++++++++++++++++++++--------- 8 files changed, 69 insertions(+), 34 deletions(-) diff --git a/block.c b/block.c index 3f3496e..75788ca 100644 --- a/block.c +++ b/block.c @@ -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]; @@ -378,7 +378,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, /* if there is a backing file, use it */ bs1 = bdrv_new(""); - ret = bdrv_open2(bs1, filename, 0, drv); + ret = bdrv_open2(bs1, filename, BDRV_O_RDONLY, drv); if (ret < 0) { bdrv_delete(bs1); return ret; @@ -445,19 +445,22 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, bs->enable_write_cache = 1; /* 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 + RDONLY as fallback (if flag BDRV_O_RO_FBCK is on) */ + bs->read_only = BDRV_FLAGS_IS_RO(flags); + if (!(flags & BDRV_O_FILE)) { + open_flags = (flags & (BDRV_O_ACCESS | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); + if (bs->is_temporary) { /* snapshot */ + open_flags |= BDRV_O_RDWR; + } + } else open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT); if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) ret = -ENOTSUP; 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); + if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE) && + (flags & BDRV_O_RO_FBCK)) { + ret = drv->bdrv_open(bs, filename, (open_flags & ~BDRV_O_RDWR) | BDRV_O_RDONLY); bs->read_only = 1; } if (ret < 0) { @@ -481,12 +484,14 @@ 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); + /* pass on ro flags to the backing_hd */ + bs->backing_hd->read_only = BDRV_FLAGS_IS_RO(flags); + open_flags &= ~BDRV_O_ACCESS; + open_flags |= (flags & BDRV_O_ACCESS); ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags, back_drv); if (ret < 0) { diff --git a/block.h b/block.h index fa51ddf..b32c6a4 100644 --- a/block.h +++ b/block.h @@ -28,8 +28,9 @@ typedef struct QEMUSnapshotInfo { } QEMUSnapshotInfo; #define BDRV_O_RDONLY 0x0000 -#define BDRV_O_RDWR 0x0002 -#define BDRV_O_ACCESS 0x0003 +#define BDRV_O_RDWR 0x0001 +#define BDRV_O_ACCESS 0x0001 +#define BDRV_O_RO_FBCK 0x0002 #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 @@ -41,6 +42,8 @@ typedef struct QEMUSnapshotInfo { #define BDRV_O_NATIVE_AIO 0x0080 /* use native AIO instead of the thread pool */ #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB) +#define BDRV_O_DFLT_OPEN (BDRV_O_RDWR | BDRV_O_RO_FBCK) +#define BDRV_FLAGS_IS_RO(flags) ((flags & BDRV_O_ACCESS) == BDRV_O_RDONLY) #define BDRV_SECTOR_BITS 9 #define BDRV_SECTOR_SIZE (1 << BDRV_SECTOR_BITS) diff --git a/hw/xen_disk.c b/hw/xen_disk.c index 5c55251..13688db 100644 --- a/hw/xen_disk.c +++ b/hw/xen_disk.c @@ -610,7 +610,8 @@ static int blk_init(struct XenDevice *xendev) /* read-only ? */ if (strcmp(blkdev->mode, "w") == 0) { mode = O_RDWR; - qflags = BDRV_O_RDWR; + /* for compatibility, also allow readonly fallback, for now */ + qflags = BDRV_O_RDWR | BDRV_O_RO_FBCK; } else { mode = O_RDONLY; qflags = BDRV_O_RDONLY; diff --git a/monitor.c b/monitor.c index d97d529..440e17e 100644 --- a/monitor.c +++ b/monitor.c @@ -910,7 +910,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_DFLT_OPEN, drv); monitor_read_bdrv_key_start(mon, bs, NULL, NULL); } diff --git a/qemu-config.c b/qemu-config.c index c3203c8..b559459 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -76,8 +76,8 @@ QemuOptsList qemu_drive_opts = { .type = QEMU_OPT_STRING, .help = "pci address (virtio only)", },{ - .name = "readonly", - .type = QEMU_OPT_BOOL, + .name = "read_write", + .type = QEMU_OPT_STRING, }, { /* end if list */ } }, diff --git a/qemu-img.c b/qemu-img.c index 1d97f2e..dee3e60 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -201,7 +201,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_DFLT_OPEN, drv) < 0) { error("Could not open '%s'", filename); } if (bdrv_is_encrypted(bs)) { @@ -406,7 +406,7 @@ static int img_check(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_RDONLY, drv) < 0) { error("Could not open '%s'", filename); } ret = bdrv_check(bs); @@ -465,7 +465,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); @@ -884,7 +884,7 @@ static int img_info(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_RDONLY, drv) < 0) { error("Could not open '%s'", filename); } bdrv_get_format(bs, fmt_name, sizeof(fmt_name)); @@ -932,10 +932,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; /* but no read-only fallback */ /* Parse commandline parameters */ for(;;) { c = getopt(argc, argv, "la:c:d:h"); @@ -951,6 +952,7 @@ static int img_snapshot(int argc, char **argv) return 0; } action = SNAPSHOT_LIST; + bdrv_oflags = BDRV_O_RDONLY; /* no need for RW */ break; case 'a': if (action) { @@ -988,7 +990,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-nbd.c b/qemu-nbd.c index 6cdb834..64f4c68 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -212,7 +212,7 @@ int main(int argc, char **argv) int opt_ind = 0; int li; char *end; - int flags = 0; + int flags = BDRV_O_DFLT_OPEN; int partition = -1; int ret; int shared = 1; diff --git a/vl.c b/vl.c index c0d98f5..cef2d67 100644 --- a/vl.c +++ b/vl.c @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int *fatal_error) { const char *buf; + const char *rw_buf = 0; const char *file = NULL; char devname[128]; const char *serial; @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int index; int cache; int aio = 0; - int ro = 0; int bdrv_flags; int on_read_error, on_write_error; const char *devaddr; DriveInfo *dinfo; int snapshot = 0; + int read_write, ro_fallback; *fatal_error = 1; @@ -2137,7 +2138,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, secs = qemu_opt_get_number(opts, "secs", 0); snapshot = qemu_opt_get_bool(opts, "snapshot", 0); - ro = qemu_opt_get_bool(opts, "readonly", 0); + rw_buf = qemu_opt_get(opts, "read_write"); file = qemu_opt_get(opts, "file"); serial = qemu_opt_get(opts, "serial"); @@ -2420,6 +2421,29 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, *fatal_error = 0; return NULL; } + + read_write = 1; + ro_fallback = 1; + if (rw_buf) { + if (!strcmp(rw_buf, "off")) { + read_write = 0; + ro_fallback = 0; + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) { + fprintf(stderr, "qemu: read_write=off flag not supported for drive of this interface\n"); + return NULL; + } + } else if (!strcmp(rw_buf, "on")) { + read_write = 1; + ro_fallback = 0; + } else if (!strcmp(rw_buf, "try")) { /* default, but keep it explicit */ + read_write = 1; + ro_fallback = 1; + } else { + fprintf(stderr, "qemu: '%s' invalid read_write option (valid values: [on|off|try] )\n", buf); + return NULL; + } + } + bdrv_flags = 0; if (snapshot) { bdrv_flags |= BDRV_O_SNAPSHOT; @@ -2436,16 +2460,16 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, bdrv_flags &= ~BDRV_O_NATIVE_AIO; } - if (ro == 1) { - if (type == IF_IDE) { - fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n"); - return NULL; - } - (void)bdrv_set_read_only(dinfo->bdrv, 1); + if (read_write) { + bdrv_flags |= BDRV_O_RDWR; + } + if (ro_fallback) { + bdrv_flags |= BDRV_O_RO_FBCK; } + if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) { - fprintf(stderr, "qemu: could not open disk image %s: %s\n", + fprintf(stderr, "qemu: could not open disk image %s with requested permission: %s\n", file, strerror(errno)); return NULL; }