[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-26 Thread Kevin Wolf 
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.kazut...@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

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-25 Thread Kevin Wolf 
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.kazut...@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.

Kevin

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-25 Thread 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.kazut...@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.kazut...@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(-)

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

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-24 Thread MORITA Kazutaka 
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.kazut...@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

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-24 Thread 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.kazut...@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(-)

diff --git a/block.c b/block.c
index 202f895..3ed35ed 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 b95a9c0..bd11cc0 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,

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-21 Thread Kevin Wolf 
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.kazut...@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

[Qemu-devel] Re: [PATCH] add support for protocol driver create_options

2010-05-21 Thread Kevin Wolf 
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.kazut...@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,