[Qemu-devel] [PATCH 1/5] block_int: add basic conversion api

[Qemu-devel] [PATCH 1/5] block_int: add basic conversion api

[Devin Nakamura]

add functions to block driver interface to support inplace image conversion

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block_int.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/block_int.h b/block_int.h
index 1e265d2..ef311c7 100644
--- a/block_int.h
+++ b/block_int.h
@@ -136,6 +136,16 @@ struct BlockDriver {
      * zeros, 0 otherwise.
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
+    
+    /*  Image conversion stuff */
+    int (*bdrv_open_conversion_target)(BlockDriverState *bs, char *filename,
+        QEMUOptionParameter *options);
+    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
+        uint64_t *host_offset, uint64_t *contiguous_bytes);
+    int (*bdrv_map)(BlockDriverState *bs, uint64_t *guest_offset, 
+        uint64_t *host_offset, uint64_t *contiguous_bytes);
+    int (*bdrv_copy_header) (BlockDriverState *bs);
+    
 
     QLIST_ENTRY(BlockDriver) list;
 };
-- 
1.7.6.rc1

[Qemu-devel] [PATCH 2/5] block: add bdrv_open_conversion_target

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   19 +++++++++++++++++++
 block.h |    2 ++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 24a25d5..e7699a6 100644
--- a/block.c
+++ b/block.c
@@ -3018,3 +3018,22 @@ out:
 
     return ret;
 }
+
+
+int bdrv_open_conversion_target(BlockDriverState **bs,
+        char *filename, char *target_fmt, QEMUOptionParameter *options)
+{
+    BlockDriver *drv;
+
+    drv = bdrv_find_format(target_fmt);
+    if(!drv){
+        return -ENOENT;
+    }
+
+    if(!drv->bdrv_open_conversion_target){
+        return -ENOTSUP;
+    }
+    *bs = bdrv_new("");
+    (*bs)->opaque = qemu_malloc(drv->instance_size);
+    return drv->bdrv_open_conversion_target(*bs, filename, options);
+}
diff --git a/block.h b/block.h
index 859d1d9..da87afb 100644
--- a/block.h
+++ b/block.h
@@ -250,6 +250,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+int bdrv_open_conversion_target(BlockDriverState **bs, char *filename,
+                                char *target_fmt, QEMUOptionParameter *options);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

Re: [Qemu-devel] [PATCH 2/5] block: add bdrv_open_conversion_target

[Kevin Wolf]

Am 05.07.2011 06:41, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.c |   19 +++++++++++++++++++
>  block.h |    2 ++
>  2 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 24a25d5..e7699a6 100644
> --- a/block.c
> +++ b/block.c
> @@ -3018,3 +3018,22 @@ out:
>  
>      return ret;
>  }
> +
> +
> +int bdrv_open_conversion_target(BlockDriverState **bs,
> +        char *filename, char *target_fmt, QEMUOptionParameter *options)
> +{
> +    BlockDriver *drv;
> +
> +    drv = bdrv_find_format(target_fmt);
> +    if(!drv){

I'll comment on it here, but you need to fix the coding style in the
whole series. You may want to try scripts/checkpatch.pl.

> +        return -ENOENT;
> +    }
> +
> +    if(!drv->bdrv_open_conversion_target){
> +        return -ENOTSUP;
> +    }
> +    *bs = bdrv_new("");
> +    (*bs)->opaque = qemu_malloc(drv->instance_size);
> +    return drv->bdrv_open_conversion_target(*bs, filename, options);

Is this actually enough? The normal bdrv_open() sets much more fields of
the BlockDriverState, and I would expect that the block drivers rely on
at least some of them.

Kevin

Re: [Qemu-devel] [PATCH 2/5] block: add bdrv_open_conversion_target

[Stefan Hajnoczi]

On Tue, Jul 05, 2011 at 12:41:21AM -0400, Devin Nakamura wrote:
> +int bdrv_open_conversion_target(BlockDriverState **bs,
> +        char *filename, char *target_fmt, QEMUOptionParameter *options)
> +{
> +    BlockDriver *drv;
> +
> +    drv = bdrv_find_format(target_fmt);
> +    if(!drv){

Please run scripts/checkpatch.pl before submitting patches.  It will
report coding style issues.

> +        return -ENOENT;
> +    }
> +
> +    if(!drv->bdrv_open_conversion_target){
> +        return -ENOTSUP;
> +    }
> +    *bs = bdrv_new("");
> +    (*bs)->opaque = qemu_malloc(drv->instance_size);

Please use qemu_mallocz() like bdrv_open_common() does.  This is safer
in case a block driver relies on the memory being zeroed (which is a
useful property).

> +    return drv->bdrv_open_conversion_target(*bs, filename, options);

Have you considered using bdrv_open_common() with a flag to avoid
duplicating bs initialization code?  You have not dealt with several
fields that bdrv_open_common() initializes.

Stefan

[Qemu-devel] [PATCH 3/5] block: add bdrv_get_mapping()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   12 ++++++++++++
 block.h |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index e7699a6..302b0d5 100644
--- a/block.c
+++ b/block.c
@@ -3037,3 +3037,15 @@ int bdrv_open_conversion_target(BlockDriverState **bs,
     (*bs)->opaque = qemu_malloc(drv->instance_size);
     return drv->bdrv_open_conversion_target(*bs, filename, options);
 }
+
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv)
+        return -ENOMEDIUM;
+    if (!drv->bdrv_get_mapping)
+        return -ENOTSUP;
+    return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index da87afb..8207389 100644
--- a/block.h
+++ b/block.h
@@ -252,6 +252,8 @@ int bdrv_in_use(BlockDriverState *bs);
 
 int bdrv_open_conversion_target(BlockDriverState **bs, char *filename,
                                 char *target_fmt, QEMUOptionParameter *options);
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [PATCH v2 3/5] block: add bdrv_get_mapping()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   14 ++++++++++++++
 block.h |    2 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 97d5a6b..3874ed5 100644
--- a/block.c
+++ b/block.c
@@ -3037,3 +3037,17 @@ int bdrv_open_conversion_target(BlockDriverState **bs,
     (*bs)->opaque = qemu_malloc(drv->instance_size);
     return drv->bdrv_open_conversion_target(*bs, filename, options);
 }
+
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_get_mapping) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index da87afb..8207389 100644
--- a/block.h
+++ b/block.h
@@ -252,6 +252,8 @@ int bdrv_in_use(BlockDriverState *bs);
 
 int bdrv_open_conversion_target(BlockDriverState **bs, char *filename,
                                 char *target_fmt, QEMUOptionParameter *options);
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

Re: [Qemu-devel] [PATCH v2 3/5] block: add bdrv_get_mapping()

[Kevin Wolf]

Am 11.07.2011 21:55, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.c |   14 ++++++++++++++
>  block.h |    2 ++
>  2 files changed, 16 insertions(+), 0 deletions(-)

I only received patches 3-5 for v2, the first two seem to be missing.

Kevin

[Qemu-devel] [PATCH 4/5] block: add bdrv_map()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   12 ++++++++++++
 block.h |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 302b0d5..c9ea201 100644
--- a/block.c
+++ b/block.c
@@ -3049,3 +3049,15 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv)
+        return -ENOMEDIUM;
+    if (!drv->bdrv_map)
+        return -ENOTSUP;
+    return drv->bdrv_map(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index 8207389..ed21ead 100644
--- a/block.h
+++ b/block.h
@@ -254,6 +254,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, char *filename,
                                 char *target_fmt, QEMUOptionParameter *options);
 int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [PATCH v2 4/5] block: add bdrv_map()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   14 ++++++++++++++
 block.h |    2 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 3874ed5..39510cb 100644
--- a/block.c
+++ b/block.c
@@ -3051,3 +3051,17 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_map) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_map(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index 8207389..ed21ead 100644
--- a/block.h
+++ b/block.h
@@ -254,6 +254,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, char *filename,
                                 char *target_fmt, QEMUOptionParameter *options);
 int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [PATCH 5/5] block: add bdrv_copy_header()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   10 ++++++++++
 block.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index c9ea201..c6c36e7 100644
--- a/block.c
+++ b/block.c
@@ -3061,3 +3061,13 @@ int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_map(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_copy_header(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv)
+        return -ENOMEDIUM;
+    if (!drv->bdrv_copy_header)
+        return -ENOTSUP;
+    return drv->bdrv_copy_header(bs);
+}
diff --git a/block.h b/block.h
index ed21ead..0583875 100644
--- a/block.h
+++ b/block.h
@@ -256,6 +256,8 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
 int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
              uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_copy_header(BlockDriverState *bs);
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [PATCH v2 5/5] block: add bdrv_copy_header()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   12 ++++++++++++
 block.h |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 39510cb..69a3e13 100644
--- a/block.c
+++ b/block.c
@@ -3065,3 +3065,15 @@ int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_map(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_copy_header(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_copy_header) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_copy_header(bs);
+}
diff --git a/block.h b/block.h
index ed21ead..0583875 100644
--- a/block.h
+++ b/block.h
@@ -256,6 +256,8 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
 int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
              uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_copy_header(BlockDriverState *bs);
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

Re: [Qemu-devel] [PATCH 1/5] block_int: add basic conversion api

[Stefan Hajnoczi]

On Tue, Jul 05, 2011 at 12:41:20AM -0400, Devin Nakamura wrote:

Please use 'block' instead of 'block_int' as the tag for this patch.  I
usually do git log <filename> to find out which tag is used for a
particular file.  'block_int' has never been used in qemu.git but
'block' is used on block layer changes.

> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block_int.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/block_int.h b/block_int.h
> index 1e265d2..ef311c7 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -136,6 +136,16 @@ struct BlockDriver {
>       * zeros, 0 otherwise.
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
> +    
> +    /*  Image conversion stuff */

How about:

/* In-place image conversion */

> +    int (*bdrv_open_conversion_target)(BlockDriverState *bs, char *filename,
> +        QEMUOptionParameter *options);
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t *guest_offset, 
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

Please document these functions so people working on block drivers know what
semantics they must implement.  Unfortunately other interfaces are not
documented but I don't see a reason to avoid documentation in the future.

Important bits would be:
 * How is .bdrv_open_conversion_target() different from a regular .bdrv_open()?
 * Which BlockDriver functions are fair game to be called while opened as a conversion target?
 * Any constraints or assumptions when a block device is in conversion target mode?
 * How do the pointer arguments of .bdrv_get_mapping() behave?  Which arguments are inputs, outputs, input/output?
 * Does .bdrv_copy_header() actually copy any header or does it "install" or "write out" a new header?
 * After .bdrv_copy_header() is the block device upgraded to first-class operation or do you need to close and then reopen the image file?

Stefan