From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43671 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOAvv-0004NQ-Tw for qemu-devel@nongnu.org; Mon, 14 Jun 2010 10:46:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOAvu-0000RD-1W for qemu-devel@nongnu.org; Mon, 14 Jun 2010 10:46:39 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:52453) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOAvt-0000Qr-UK for qemu-devel@nongnu.org; Mon, 14 Jun 2010 10:46:38 -0400 Received: by gyd5 with SMTP id 5so2569814gyd.4 for ; Mon, 14 Jun 2010 07:46:31 -0700 (PDT) Message-ID: <4C1640C1.7000401@codemonkey.ws> Date: Mon, 14 Jun 2010 09:46:25 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 13/13] blockdev: New -blockdev to define a host block device References: <1275497729-13120-1-git-send-email-armbru@redhat.com> <1275497729-13120-14-git-send-email-armbru@redhat.com> In-Reply-To: <1275497729-13120-14-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com On 06/02/2010 11:55 AM, Markus Armbruster wrote: > Existing -drive defines both host and guest part. To make it work > with -device, we created if=none. But all this does is peel off guest > device selection. The other guest properties such as geometry, > removable vs. fixed media, and serial number are still in the wrong > place. > > Instead of overloading -drive even further, create a new, clean option > to define a host block device. -drive stays around unchanged for > command line convenience and backwards compatibility. > > This is just a first step. Future work includes: > > * A set of monitor commands to go with it. > > * Let device model declare media removable. -drive has that in the > host part, as media=(cdrom|floppy), but it really belongs to the > guest part. > > * Turn geometry into device properties. This will also ensure proper > range checking. The existing range checking for -drive can't work > with if=none. > > * Make device models reject error actions they don't support. The > existing check for -drive can't work with if=none. > > * Like -drive, -blockdev ignores cache= silently when snapshot=on. Do > we really want that? > > Signed-off-by: Markus Armbruster > --- > blockdev.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++---- > blockdev.h | 2 + > qemu-config.c | 38 +++++++++++++++ > qemu-config.h | 1 + > qemu-options.hx | 49 +++++++++++++++++++ > vl.c | 29 ++++++++++- > 6 files changed, 246 insertions(+), 14 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index a1e6394..e03ecfc 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -98,6 +98,132 @@ static int parse_block_aio(const char *val) > } > } > > +static int blockdev_insert(BlockDriverState *bs, QemuOpts *opts) > +{ > + int snapshot = qemu_opt_get_bool(opts, "snapshot", 0); > + const char *file = qemu_opt_get(opts, "file"); > + const char *cache = qemu_opt_get(opts, "cache"); > + const char *aio = qemu_opt_get(opts, "aio"); > + const char *format = qemu_opt_get(opts, "format"); > + const char *rerror = qemu_opt_get(opts, "rerror"); > + const char *werror = qemu_opt_get(opts, "werror"); > + int readonly = qemu_opt_get_bool(opts, "readonly", 0); > + BlockDriver *drv; > + int res, flags; > + BlockErrorAction on_read_error, on_write_error; > + > + if (!file) { > + qerror_report(QERR_MISSING_PARAMETER, "file"); > + return -1; > + } > + > + drv = NULL; > + if (format) { > + drv = parse_block_format(format); > + if (!drv) { > + return -1; > + } > + } > + > + res = parse_block_error_action(rerror, 1); > + if (on_read_error< 0) { > + return -1; > + } > + on_read_error = res; > + res = parse_block_error_action(werror, 0); > + if (res< 0) { > + return -1; > + } > + on_write_error = res; > + > + flags = 0; > + res = parse_block_cache(cache); > + if (res< 0) { > + return -1; > + } > + flags |= res; > + if (snapshot) { > + /* always use write-back with snapshot */ > + /* FIXME ignores explicit cache= *silently*; really want that? */ > + flags&= ~BDRV_O_CACHE_MASK; > + flags |= (BDRV_O_SNAPSHOT | BDRV_O_CACHE_WB); > + flags |= BDRV_O_SNAPSHOT; > + } > + res = parse_block_aio(aio); > + if (res< 0) { > + return -1; > + } > + flags |= res; > + flags |= readonly ? 0 : BDRV_O_RDWR; > + > + bdrv_set_on_error(bs, on_read_error, on_write_error); > + res = bdrv_open(bs, file, flags, drv); > + if (res< 0) { > + qerror_report(QERR_OPEN_FILE_FAILED, file); > + bdrv_close(bs); > + return -1; > + } > + return 0; > +} > + > +BlockDriverState *blockdev_open(QemuOpts *opts) > +{ > + const char *id = qemu_opts_id(opts); > + const char *file = qemu_opt_get(opts, "file"); > + BlockDriverState *bs; > + QemuOpt *opt; > + const char *name; > + > + if (!id) { > + qerror_report(QERR_MISSING_PARAMETER, "id"); > + return NULL; > + } > + > + bs = bdrv_find(id); > + if (bs) { > + qerror_report(QERR_DUPLICATE_ID, id, "blockdev"); > + return NULL; > + } > + bs = bdrv_new(id); > + > + if (!file) { > + /* file is optional only if no other options are present; check */ > + opt = NULL; > + while ((opt = qemu_opt_next(opts, opt))) { > + name = qemu_opt_name(opt); > + if (strcmp(name, "file")) { > + qerror_report(QERR_MISSING_PARAMETER, "file"); > + return NULL; > + } > + } > + /* block device without media wanted */ > + return bs; > + } > + > + if (blockdev_insert(bs, opts)< 0) { > + return NULL; > + } > + return bs; > +} > + > +static void blockdev_format_help_iter(void *opaque, const char *name) > +{ > + error_printf(" %s", name); > +} > + > +int blockdev_format_help(QemuOpts *opts) > +{ > + const char *format = qemu_opt_get(opts, "format"); > + > + if (format&& !strcmp(format, "?")) { > + error_printf("Supported block device formats:"); > + bdrv_iterate_format(blockdev_format_help_iter, NULL); > + error_printf("\n"); > + return 1; > + } > + return 0; > +} > + > static int blockdev_del_dinfo(BlockDriverState *bs) > { > DriveInfo *dinfo, *next_dinfo; > @@ -190,11 +316,6 @@ const char *drive_get_serial(BlockDriverState *bdrv) > return "\0"; > } > > -static void bdrv_format_print(void *opaque, const char *name) > -{ > - fprintf(stderr, " %s", name); > -} > - > void drive_uninit(DriveInfo *dinfo) > { > qemu_opts_del(dinfo->opts); > @@ -227,6 +348,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error) > > *fatal_error = 1; > > + if (blockdev_format_help(opts)) { > + return NULL; > + } > + > translation = BIOS_ATA_TRANSLATION_AUTO; > > if (default_to_scsi) { > @@ -353,12 +478,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error) > bdrv_flags |= ret; > > if ((buf = qemu_opt_get(opts, "format")) != NULL) { > - if (strcmp(buf, "?") == 0) { > - fprintf(stderr, "qemu: Supported formats:"); > - bdrv_iterate_format(bdrv_format_print, NULL); > - fprintf(stderr, "\n"); > - return NULL; > - } > drv = parse_block_format(buf); > if (!drv) { > return NULL; > diff --git a/blockdev.h b/blockdev.h > index bb89bfa..564c64d 100644 > --- a/blockdev.h > +++ b/blockdev.h > @@ -17,6 +17,8 @@ > #include "block.h" > #include "qemu-queue.h" > > +int blockdev_format_help(QemuOpts *); > +BlockDriverState *blockdev_open(QemuOpts *); > int blockdev_attach(BlockDriverState *, DeviceState *); > void blockdev_detach(BlockDriverState *, DeviceState *); > > diff --git a/qemu-config.c b/qemu-config.c > index 5a4e61b..2c3814b 100644 > --- a/qemu-config.c > +++ b/qemu-config.c > @@ -84,6 +84,43 @@ QemuOptsList qemu_drive_opts = { > }, > }; > > +QemuOptsList qemu_blockdev_opts = { > + .name = "blockdev", > + .head = QTAILQ_HEAD_INITIALIZER(qemu_blockdev_opts.head), > + .desc = { > + { > + .name = "snapshot", > + .type = QEMU_OPT_BOOL, > + },{ > + .name = "file", > + .type = QEMU_OPT_STRING, > + .help = "disk image", > + },{ > + .name = "cache", > + .type = QEMU_OPT_STRING, > + .help = "host cache usage (none, writeback, writethrough, unsafe)", > + },{ > + .name = "aio", > + .type = QEMU_OPT_STRING, > + .help = "host AIO implementation (threads, native)", > + },{ > + .name = "format", > + .type = QEMU_OPT_STRING, > + .help = "disk format (raw, qcow2, ...)", > + },{ > + .name = "rerror", > + .type = QEMU_OPT_STRING, > + },{ > + .name = "werror", > + .type = QEMU_OPT_STRING, > + },{ > + .name = "readonly", > + .type = QEMU_OPT_BOOL, > + }, > + { /* end if list */ } > + }, > +}; > + > QemuOptsList qemu_chardev_opts = { > .name = "chardev", > .implied_opt_name = "backend", > @@ -338,6 +375,7 @@ QemuOptsList qemu_cpudef_opts = { > > static QemuOptsList *vm_config_groups[] = { > &qemu_drive_opts, > +&qemu_blockdev_opts, > &qemu_chardev_opts, > &qemu_device_opts, > &qemu_netdev_opts, > diff --git a/qemu-config.h b/qemu-config.h > index dca69d4..e6214d6 100644 > --- a/qemu-config.h > +++ b/qemu-config.h > @@ -2,6 +2,7 @@ > #define QEMU_CONFIG_H > > extern QemuOptsList qemu_drive_opts; > +extern QemuOptsList qemu_blockdev_opts; > extern QemuOptsList qemu_chardev_opts; > #ifdef CONFIG_LINUX > extern QemuOptsList qemu_fsdev_opts; > diff --git a/qemu-options.hx b/qemu-options.hx > index a6928b7..38d0573 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1477,6 +1477,55 @@ ETEXI > > DEFHEADING() > > +DEFHEADING(Block device options:) > + > +DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev, > + "-blockdev id=id[,file=file][,format=format][,snapshot=on|off]\n" > + " [,cache=writethrough|writeback|unsafe|none][,aio=threads|native]\n" > + " [,rerror=ignore|report|stop][,werror=enospc|ignore|report|stop]\n" > + " [,readonly=on|off]\n" > + " define a host block device\n", QEMU_ARCH_ALL) > +STEXI > + > +The general form of a block device option is: > + > +@table @option > + > +@item -blockdev id=@var{id}[,@var{options}] > +@findex -blockdev > + > +All block devices must have an id. It is used to uniquely identify > +this device in other command line directives. > + > +Available options are: > + > +@table @option > +@item format=@var{format} > +This specifies the disk format to use. If left out, QEMU will attempt > +to determine it automatically. Interpreting an untrusted format > +header is insecure. Use @option{-blockdev format=?} to list available > +formats. > +@item file=@var{file} > +This option defines which disk image (@pxref{disk_images}) to use as > +media for this block device. You can define a block device without > +media. > +@item snapshot=on|off > +Whether to snapshot this block device (see @option{-snapshot}). > +@item cache=none|writeback|unsafe|writethrough > +Control use of host page cache. > +@item aio=threads|native > +Specify whether to use pthread based disk I/O or native Linux AIO. > +@item rerror=ignore|report|stop > +What to do on read error. > +@item werror=enospc|ignore|report|stop > +What to do on write error. > +@item readonly=on|off > +@end table > +@end table > +ETEXI > + > +DEFHEADING() > + > DEFHEADING(Bluetooth(R) options:) > > DEF("bt", HAS_ARG, QEMU_OPTION_bt, \ > diff --git a/vl.c b/vl.c > index 0a9862f..5ee6024 100644 > --- a/vl.c > +++ b/vl.c > @@ -662,7 +662,7 @@ static int drive_init_func(QemuOpts *opts, void *opaque) > return 0; > } > > -static int drive_enable_snapshot(QemuOpts *opts, void *opaque) > +static int opts_enable_snapshot(QemuOpts *opts, void *opaque) > { > if (NULL == qemu_opt_get(opts, "snapshot")) { > qemu_opt_set(opts, "snapshot", "on"); > @@ -1806,6 +1806,17 @@ static int device_init_func(QemuOpts *opts, void *opaque) > return 0; > } > > +static int blockdev_init_func(QemuOpts *opts, void *opaque) > +{ > + if (blockdev_format_help(opts)) { > + exit(0); > + } > + if (!blockdev_open(opts)) { > + return -1; > + } > + return 0; > +} > + > static int chardev_init_func(QemuOpts *opts, void *opaque) > { > CharDriverState *chr; > @@ -2257,6 +2268,12 @@ int main(int argc, char **argv, char **envp) > case QEMU_OPTION_drive: > drive_add(NULL, "%s", optarg); > break; > + case QEMU_OPTION_blockdev: > + opts = qemu_opts_parse(&qemu_blockdev_opts, optarg, 0); > + if (!opts) { > + exit(1); > + } > + break; > A good test of whether blockdev is an adequate replacement for -drive is to implement -drive in terms of -blockdev. Of course, it's not clear how we translate from ,if=virtio to virtio-blk-pci in a machine neutral way. Regards, Anthony Liguori > case QEMU_OPTION_set: > if (qemu_set_option(optarg) != 0) > exit(1); > @@ -3156,8 +3173,14 @@ int main(int argc, char **argv, char **envp) > } > > /* open the virtual block devices */ > - if (snapshot) > - qemu_opts_foreach(&qemu_drive_opts, drive_enable_snapshot, NULL, 0); > + if (snapshot) { > + qemu_opts_foreach(&qemu_blockdev_opts, opts_enable_snapshot, NULL, 0); > + qemu_opts_foreach(&qemu_drive_opts, opts_enable_snapshot, NULL, 0); > + } > + if (qemu_opts_foreach(&qemu_blockdev_opts, blockdev_init_func, NULL, 1)) { > + exit(1); > + } > + qemu_opts_reset(&qemu_blockdev_opts); > if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func,&machine->use_scsi, 1) != 0) > exit(1); > >