[13/13] blockdev: New -blockdev to define a host block device

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(-)

On Wed, Jun 02, 2010 at 06:55:29PM +0200, 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:

One thing we really needs is a protocol option.  The current colon
syntax means we can't support filenames with colons in them which
users keep requesting.  By making the protocol a separate option
we can sort this out.

Christoph Hellwig <hch@lst.de> writes:

> On Wed, Jun 02, 2010 at 06:55:29PM +0200, 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:
>
> One thing we really needs is a protocol option.  The current colon
> syntax means we can't support filenames with colons in them which
> users keep requesting.  By making the protocol a separate option
> we can sort this out.

You're absolutely right.  I'll look into it.

On 06/02/2010 06:55 PM, Markus Armbruster wrote:
> * Like -drive, -blockdev ignores cache= silently when snapshot=on.  Do
>    we really want that?

Yes, the changes are throw-away by definition.  Might as well use 
cache=unsafe.

> +    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;

Cut and paste?

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/02/2010 06:55 PM, Markus Armbruster wrote:
>> * Like -drive, -blockdev ignores cache= silently when snapshot=on.  Do
>>    we really want that?
>
> Yes, the changes are throw-away by definition.  Might as well use
> cache=unsafe.

I understand why that's the most sensible cache setting.  But if the
user explicitly asks for something else, I think we better give it to
him, or tell him no.  Ignoring him silently isn't nice.

>> +    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;
>
> Cut and paste?

Pasto indeed, will fix.  Thanks!

On 06/10/2010 06:32 PM, Markus Armbruster wrote:
> I understand why that's the most sensible cache setting.  But if the
> user explicitly asks for something else, I think we better give it to
> him, or tell him no.  Ignoring him silently isn't nice.

Ah, it's clearer now...

I guess one could use cache=something together with snapshot to do 
benchmarking.  Actually the same changes in behavior (unsafe as default, 
but observe a non-default value) can be done to -drive ...,snapshot too. 
  Maybe I'll give it a shot.

Paolo

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);
>
>