Patchwork Re: [PATCH] add support for protocol driver create_options

login
register
mail settings
Submitter MORITA Kazutaka
Date May 26, 2010, 2:35 a.m.
Message ID <lpr5kz4cg7.wl%morita.kazutaka@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/53586/
State New
Headers show

Comments

MORITA Kazutaka - May 26, 2010, 2:35 a.m.
At Tue, 25 May 2010 15:43:17 +0200,
Kevin Wolf wrote:
> 
> Am 24.05.2010 08:34, schrieb MORITA Kazutaka:
> > At Fri, 21 May 2010 18:57:36 +0200,
> > Kevin Wolf wrote:
> >>
> >> Am 20.05.2010 07:36, schrieb MORITA Kazutaka:
> >>> +
> >>> +/*
> >>> + * 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.
> >>
> > 
> > I forgot to add it, sorry.
> > Fixed version is below.
> > 
> > Thanks,
> > 
> > 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>
> 
> $ ./qemu-img create -f qcow2 -o cluster_size=4k /tmp/test.qcow2 4G
> Unknown option 'cluster_size'
> qemu-img: Invalid options for file format 'qcow2'.
> 
> I think you added another num_dest_options++ which shouldn't be there.
> 

Sorry again.  I wrongly added `dest[num_dest_options++].name = NULL;'
instead of `dest[num_dest_options].name = NULL;'.

Thanks,

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 |   53 ++++++++++++++++++++++++++++++++++++++++++++++-------
 qemu-option.h |    2 ++
 5 files changed, 86 insertions(+), 26 deletions(-)
Kevin Wolf - May 26, 2010, 9:02 a.m.
Am 26.05.2010 04:35, schrieb MORITA Kazutaka:
> At Tue, 25 May 2010 15:43:17 +0200,
> Kevin Wolf wrote:
>>
>> Am 24.05.2010 08:34, schrieb MORITA Kazutaka:
>>> At Fri, 21 May 2010 18:57:36 +0200,
>>> Kevin Wolf wrote:
>>>>
>>>> Am 20.05.2010 07:36, schrieb MORITA Kazutaka:
>>>>> +
>>>>> +/*
>>>>> + * 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.
>>>>
>>>
>>> I forgot to add it, sorry.
>>> Fixed version is below.
>>>
>>> Thanks,
>>>
>>> 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>
>>
>> $ ./qemu-img create -f qcow2 -o cluster_size=4k /tmp/test.qcow2 4G
>> Unknown option 'cluster_size'
>> qemu-img: Invalid options for file format 'qcow2'.
>>
>> I think you added another num_dest_options++ which shouldn't be there.
>>
> 
> Sorry again.  I wrongly added `dest[num_dest_options++].name = NULL;'
> instead of `dest[num_dest_options].name = NULL;'.

Thanks, now it looks good and passes all qemu-iotests tests again.
Applied the patch to the block branch.

Kevin

Patch

diff --git a/block.c b/block.c
index 6e7766a..f881f10 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];
@@ -478,7 +477,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 cb007b7..ea091f0 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..acd74f9 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -346,6 +346,51 @@  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;
+            dest[num_dest_options].name = NULL;
+        }
+        list++;
+    }
+
+    return dest;
+}
+
+/*
  * Parses a parameter string (param) into an option list (dest).
  *
  * list is the templace is. If dest is NULL, a new copy of list is created for
@@ -365,7 +410,6 @@  void free_option_parameters(QEMUOptionParameter *list)
 QEMUOptionParameter *parse_option_parameters(const char *param,
     QEMUOptionParameter *list, QEMUOptionParameter *dest)
 {
-    QEMUOptionParameter *cur;
     QEMUOptionParameter *allocated = NULL;
     char name[256];
     char value[256];
@@ -379,12 +423,7 @@  QEMUOptionParameter *parse_option_parameters(const char *param,
 
     if (dest == NULL) {
         // Count valid options
-        num_options = 0;
-        cur = list;
-        while (cur->name) {
-            num_options++;
-            cur++;
-        }
+        num_options = count_option_parameters(list);
 
         // Create a copy of the option list to fill in values
         dest = qemu_mallocz((num_options + 1) * sizeof(QEMUOptionParameter));
diff --git a/qemu-option.h b/qemu-option.h
index 58136f3..4823219 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -70,6 +70,8 @@  int set_option_parameter(QEMUOptionParameter *list, const char *name,
     const char *value);
 int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
     uint64_t value);
+QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
+    QEMUOptionParameter *list);
 QEMUOptionParameter *parse_option_parameters(const char *param,
     QEMUOptionParameter *list, QEMUOptionParameter *dest);
 void free_option_parameters(QEMUOptionParameter *list);