add support for protocol driver create_options

This patch enables protocol drivers to use their create options which
are not supported by the format.  For example, protcol drivers can use
a backing_file option with raw format.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---
 block.c       |    7 +++----
 block.h       |    1 +
 qemu-img.c    |   49 ++++++++++++++++++++++++++++++++++---------------
 qemu-option.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-------
 qemu-option.h |    2 ++
 5 files changed, 85 insertions(+), 26 deletions(-)

Hi, Folks:

Could you provide pointer to the kvm device passthrough howto/FAQ?

I have two questions:

1. my host os, the Linux doesn't have the native device driver for some home grown pci devices, the driver is in the guest os, does device passthrough work in this case? Assuming I have VT-d.

2. How about i2c devices?

Thanks

Mu

Am 20.05.2010 07:36, schrieb MORITA Kazutaka:
> This patch enables protocol drivers to use their create options which
> are not supported by the format.  For example, protcol drivers can use
> a backing_file option with raw format.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

Hm, this is not stackable, right? Though I do see that making it
stackable would require some bigger changes, so maybe we can get away
with claiming that this approach covers everything that happens in practice.

If we accept that this is the desired behaviour, the code looks good to me.

Kevin

Am 20.05.2010 07:36, schrieb MORITA Kazutaka:
> This patch enables protocol drivers to use their create options which
> are not supported by the format.  For example, protcol drivers can use
> a backing_file option with raw format.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> ---
>  block.c       |    7 +++----
>  block.h       |    1 +
>  qemu-img.c    |   49 ++++++++++++++++++++++++++++++++++---------------
>  qemu-option.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-------
>  qemu-option.h |    2 ++
>  5 files changed, 85 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 48d8468..0ab9424 100644
> --- a/block.c
> +++ b/block.c
> @@ -56,7 +56,6 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t sector_num,
>                          uint8_t *buf, int nb_sectors);
>  static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
>                           const uint8_t *buf, int nb_sectors);
> -static BlockDriver *find_protocol(const char *filename);
>  
>  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>      QTAILQ_HEAD_INITIALIZER(bdrv_states);
> @@ -210,7 +209,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
>  {
>      BlockDriver *drv;
>  
> -    drv = find_protocol(filename);
> +    drv = bdrv_find_protocol(filename);
>      if (drv == NULL) {
>          drv = bdrv_find_format("file");
>      }
> @@ -283,7 +282,7 @@ static BlockDriver *find_hdev_driver(const char *filename)
>      return drv;
>  }
>  
> -static BlockDriver *find_protocol(const char *filename)
> +BlockDriver *bdrv_find_protocol(const char *filename)
>  {
>      BlockDriver *drv1;
>      char protocol[128];
> @@ -469,7 +468,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
>      BlockDriver *drv;
>      int ret;
>  
> -    drv = find_protocol(filename);
> +    drv = bdrv_find_protocol(filename);
>      if (!drv) {
>          return -ENOENT;
>      }
> diff --git a/block.h b/block.h
> index 24efeb6..9034ebb 100644
> --- a/block.h
> +++ b/block.h
> @@ -54,6 +54,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>  
>  void bdrv_init(void);
>  void bdrv_init_with_whitelist(void);
> +BlockDriver *bdrv_find_protocol(const char *filename);
>  BlockDriver *bdrv_find_format(const char *format_name);
>  BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
>  int bdrv_create(BlockDriver *drv, const char* filename,
> diff --git a/qemu-img.c b/qemu-img.c
> index d3c30a7..8ae7184 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -252,8 +252,8 @@ static int img_create(int argc, char **argv)
>      const char *base_fmt = NULL;
>      const char *filename;
>      const char *base_filename = NULL;
> -    BlockDriver *drv;
> -    QEMUOptionParameter *param = NULL;
> +    BlockDriver *drv, *proto_drv;
> +    QEMUOptionParameter *param = NULL, *create_options = NULL;
>      char *options = NULL;
>  
>      flags = 0;
> @@ -286,33 +286,42 @@ static int img_create(int argc, char **argv)
>          }
>      }
>  
> +    /* Get the filename */
> +    if (optind >= argc)
> +        help();
> +    filename = argv[optind++];
> +
>      /* Find driver and parse its options */
>      drv = bdrv_find_format(fmt);
>      if (!drv)
>          error("Unknown file format '%s'", fmt);
>  
> +    proto_drv = bdrv_find_protocol(filename);
> +    if (!proto_drv)
> +        error("Unknown protocol '%s'", filename);
> +
> +    create_options = append_option_parameters(create_options,
> +                                              drv->create_options);
> +    create_options = append_option_parameters(create_options,
> +                                              proto_drv->create_options);
> +
>      if (options && !strcmp(options, "?")) {
> -        print_option_help(drv->create_options);
> +        print_option_help(create_options);
>          return 0;
>      }
>  
>      /* Create parameter list with default values */
> -    param = parse_option_parameters("", drv->create_options, param);
> +    param = parse_option_parameters("", create_options, param);
>      set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
>  
>      /* Parse -o options */
>      if (options) {
> -        param = parse_option_parameters(options, drv->create_options, param);
> +        param = parse_option_parameters(options, create_options, param);
>          if (param == NULL) {
>              error("Invalid options for file format '%s'.", fmt);
>          }
>      }
>  
> -    /* Get the filename */
> -    if (optind >= argc)
> -        help();
> -    filename = argv[optind++];
> -
>      /* Add size to parameters */
>      if (optind < argc) {
>          set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
> @@ -362,6 +371,7 @@ static int img_create(int argc, char **argv)
>      puts("");
>  
>      ret = bdrv_create(drv, filename, param);
> +    free_option_parameters(create_options);
>      free_option_parameters(param);
>  
>      if (ret < 0) {
> @@ -543,14 +553,14 @@ static int img_convert(int argc, char **argv)
>  {
>      int c, ret, n, n1, bs_n, bs_i, flags, cluster_size, cluster_sectors;
>      const char *fmt, *out_fmt, *out_baseimg, *out_filename;
> -    BlockDriver *drv;
> +    BlockDriver *drv, *proto_drv;
>      BlockDriverState **bs, *out_bs;
>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>      uint64_t bs_sectors;
>      uint8_t * buf;
>      const uint8_t *buf1;
>      BlockDriverInfo bdi;
> -    QEMUOptionParameter *param = NULL;
> +    QEMUOptionParameter *param = NULL, *create_options = NULL;
>      char *options = NULL;
>  
>      fmt = NULL;
> @@ -615,19 +625,27 @@ static int img_convert(int argc, char **argv)
>      if (!drv)
>          error("Unknown file format '%s'", out_fmt);
>  
> +    proto_drv = bdrv_find_protocol(out_filename);
> +    if (!proto_drv)
> +        error("Unknown protocol '%s'", out_filename);
> +
> +    create_options = append_option_parameters(create_options,
> +                                              drv->create_options);
> +    create_options = append_option_parameters(create_options,
> +                                              proto_drv->create_options);
>      if (options && !strcmp(options, "?")) {
> -        print_option_help(drv->create_options);
> +        print_option_help(create_options);
>          free(bs);
>          return 0;
>      }
>  
>      if (options) {
> -        param = parse_option_parameters(options, drv->create_options, param);
> +        param = parse_option_parameters(options, create_options, param);
>          if (param == NULL) {
>              error("Invalid options for file format '%s'.", out_fmt);
>          }
>      } else {
> -        param = parse_option_parameters("", drv->create_options, param);
> +        param = parse_option_parameters("", create_options, param);
>      }
>  
>      set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> @@ -649,6 +667,7 @@ static int img_convert(int argc, char **argv)
>  
>      /* Create the new image */
>      ret = bdrv_create(drv, out_filename, param);
> +    free_option_parameters(create_options);
>      free_option_parameters(param);
>  
>      if (ret < 0) {
> diff --git a/qemu-option.c b/qemu-option.c
> index 076dddf..3ef4abc 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -346,6 +346,50 @@ void free_option_parameters(QEMUOptionParameter *list)
>  }
>  
>  /*
> + * Count valid options in list
> + */
> +static size_t count_option_parameters(QEMUOptionParameter *list)
> +{
> +    size_t num_options = 0;
> +
> +    while (list && list->name) {
> +        num_options++;
> +        list++;
> +    }
> +
> +    return num_options;
> +}
> +
> +/*
> + * Append an option list (list) to an option list (dest).
> + *
> + * If dest is NULL, a new copy of list is created.
> + *
> + * Returns a pointer to the first element of dest (or the newly allocated copy)
> + */
> +QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
> +    QEMUOptionParameter *list)
> +{
> +    size_t num_options, num_dest_options;
> +
> +    num_options = count_option_parameters(dest);
> +    num_dest_options = num_options;
> +
> +    num_options += count_option_parameters(list);
> +
> +    dest = qemu_realloc(dest, (num_options + 1) * sizeof(QEMUOptionParameter));
> +
> +    while (list && list->name) {
> +        if (get_option_parameter(dest, list->name) == NULL) {
> +            dest[num_dest_options++] = *list;

You need to add a dest[num_dest_options].name = NULL; here. Otherwise
the next loop iteration works on uninitialized memory and possibly an
unterminated list. I got a segfault for that reason.

Kevin

At Fri, 21 May 2010 13:40:31 +0200,
Kevin Wolf wrote:
> 
> Am 20.05.2010 07:36, schrieb MORITA Kazutaka:
> > This patch enables protocol drivers to use their create options which
> > are not supported by the format.  For example, protcol drivers can use
> > a backing_file option with raw format.
> > 
> > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> 
> Hm, this is not stackable, right? Though I do see that making it
> stackable would require some bigger changes, so maybe we can get away
> with claiming that this approach covers everything that happens in practice.
> 
> If we accept that this is the desired behaviour, the code looks good to me.
> 
As you say, this patch isn't stackable; we must specify a image name
with at most 1 format and 1 protocol.  I cannot think of a situation
where we want to use more than one protocol to create qemu images, so
this seems to be enough to me.

Thanks,

Kazutaka