Re: [PATCH] add support for protocol driver create_options

[Kevin Wolf <kwolf-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> ---
>  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