Re: [Qemu-devel] [PATCH 13/13] blockdev: New -blockdev to define a host block device

[Anthony Liguori <anthony@codemonkey.ws>]

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<armbru@redhat.com>
> ---
>   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);
>
>