[Qemu-devel] [PATCH V4 0/7] Block Conversion

[Qemu-devel] [PATCH V4 0/7] Block Conversion

[Devin Nakamura]

This patch series introduces the api to support block conversion.

Devin Nakamura (7):
  block: add block conversion api
  block: make bdrv_open_common more ready to be called by
    bdrv_open_conversion_target
  block: add bdrv_get_conversion_options()
  block: add bdrv_open_conversion_target()
  block: add bdrv_get_mapping()
  block: add bdrv_map()
  block: add bdrv_copy_header()

 block.c     |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 block.h     |   16 +++++++
 block_int.h |   82 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+), 6 deletions(-)

-- 
1.7.6.rc1

[Qemu-devel] [PATCH V4 1/7] block: add block conversion api

[Devin Nakamura]

add functions to block driver interface to support inplace image conversion

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.h     |    3 ++
 block_int.h |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/block.h b/block.h
index a3bfaaf..4bdd82e 100644
--- a/block.h
+++ b/block.h
@@ -35,6 +35,7 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_O_NATIVE_AIO  0x0080 /* use native AIO instead of the thread pool */
 #define BDRV_O_NO_BACKING  0x0100 /* don't open the backing file */
 #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
+#define BDRV_O_CONVERSION  0x0400 /* open as conversion target */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
@@ -254,6 +255,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);
 
+typedef struct BlockConversionOptions BlockConversionOptions;
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
diff --git a/block_int.h b/block_int.h
index f6d02b3..66660f4 100644
--- a/block_int.h
+++ b/block_int.h
@@ -42,6 +42,9 @@
 #define BLOCK_OPT_PREALLOC      "preallocation"
 #define BLOCK_OPT_SUBFMT        "subformat"
 
+#define BLOCK_CRYPT_NONE    0
+#define BLOCK_CRYPT_AES     1
+
 typedef struct AIOPool {
     void (*cancel)(BlockDriverAIOCB *acb);
     int aiocb_size;
@@ -145,6 +148,79 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /* In-place image conversion */
+
+    /**
+     * Opens an image conversion target.
+     *
+     * @param bs          Basic Initialization done by
+     *                    bdrv_open_conversion_target() Still need to set format
+     *                    specific data.
+     * @param usr_options Creation options.
+     * @param drv_options Conversion Options
+     * @return            Returns non-zero on failure.
+     */
+    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
+        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options,
+        bool force);
+
+    /**
+     * Gets a mapping from an offset in the image to an offset within a file
+     *
+     * All offsets are in bytes.
+     *
+     * @param guest_offset          The starting offset of the mapping
+     * @param host_offset[out]      The starting file offset of the mapping.
+     *                              A value of -1 is invalid and means the
+     *                              cluster is unallocated (contiguous_bytes is
+     *                              still valid)
+     * @param contiguous_bytes[out] The number of bytes for which this mapping
+     *                              is contiguous. If 0, there are no more
+     *                              mapppings in the image
+     * @return                      Returns 0 on error;
+     */
+
+    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t guest_offset,
+        uint64_t *host_offset, uint64_t *contiguous_bytes);
+
+    /**
+     * Sets a mapping in the image file.
+     *
+     * @param bs               Usually opened with bdrv_open_conversion_target
+     * @param guest_offset     The starting guest offset of the mapping
+     *                         (in bytes).
+     * @param host_offset      The starting image offset of the mapping
+     *                         (in bytes).
+     * @param contiguous_bytes The number of bytes for which this mapping exists
+     * @return                 Returns non-zero on error. Will return -EINVAL if
+     *                         guest_offset, host_offset, or contiguous_bytes
+     *                         not cluster aligned
+     */
+    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
+        uint64_t host_offset, uint64_t contiguous_bytes);
+
+    /**
+     * Copies out the header of a conversion target
+     *
+     * Saves the current header for the image in a temporary file and overwrites
+     * it with the header for the new format (at the moment the header is
+     * assumed to be 1 sector)
+     * @param bs        Usualy opened with bdrv_open_conversion_target()
+     * @param filename  The name of the file to copy the header into
+     * @return          Returns non-zero on failure
+     */
+    int (*bdrv_copy_header) (BlockDriverState *bs, char* filename);
+
+    /**
+     * Asks the block driver what options should be used to create a conversion
+     * target.
+     *
+     * @param options[out] Block conversion options
+     */
+    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
+         BlockConversionOptions *options);
+
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
@@ -269,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT32("discard_granularity", _state, \
                        _conf.discard_granularity, 0)
 
+struct BlockConversionOptions {
+    int encryption_type;
+    uint64_t image_size;
+    uint64_t cluster_size;
+    int nb_snapshots;
+};
 #endif /* BLOCK_INT_H */
-- 
1.7.6.rc1

[Qemu-devel] [PATCH V4 2/7] block: make bdrv_open_common more ready to be called by bdrv_open_conversion_target

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a8a013a..ac5a103 100644
--- a/block.c
+++ b/block.c
@@ -441,14 +441,17 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
  * Common part for opening disk images and files
  */
 static int bdrv_open_common(BlockDriverState *bs, const char *filename,
-    int flags, BlockDriver *drv)
+    int flags, BlockDriver *drv, BlockConversionOptions *drv_options,
+    QEMUOptionParameter *usr_options, bool force)
 {
     int ret, open_flags;
 
     assert(drv != NULL);
 
-    bs->file = NULL;
-    bs->total_sectors = 0;
+    if (~flags & BDRV_O_CONVERSION) {
+        bs->file = NULL;
+        bs->total_sectors = 0;
+    }
     bs->encrypted = 0;
     bs->valid_key = 0;
     bs->open_flags = flags;
@@ -481,7 +484,10 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     }
 
     /* Open the image, either directly or using a protocol */
-    if (drv->bdrv_file_open) {
+    if (flags & BDRV_O_CONVERSION) {
+        ret = drv->bdrv_open_conversion_target(bs, drv_options, usr_options,
+                                               force);
+    } else if (drv->bdrv_file_open) {
         ret = drv->bdrv_file_open(bs, filename, open_flags);
     } else {
         ret = bdrv_file_open(&bs->file, filename, open_flags);
@@ -534,7 +540,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     }
 
     bs = bdrv_new("");
-    ret = bdrv_open_common(bs, filename, flags, drv);
+    ret = bdrv_open_common(bs, filename, flags, drv, NULL, NULL, false);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;
@@ -618,7 +624,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
     }
 
     /* Open the image */
-    ret = bdrv_open_common(bs, filename, flags, drv);
+    ret = bdrv_open_common(bs, filename, flags, drv, NULL, NULL, false);
     if (ret < 0) {
         goto unlink_and_fail;
     }
-- 
1.7.6.rc1

[Qemu-devel] [PATCH V4 3/7] block: add bdrv_get_conversion_options()

[Devin Nakamura]

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

diff --git a/block.c b/block.c
index ac5a103..a3ea039 100644
--- a/block.c
+++ b/block.c
@@ -3281,3 +3281,16 @@ out:
 
     return ret;
 }
+
+int bdrv_get_conversion_options(BlockDriverState *bs,
+                                BlockConversionOptions *options)
+{
+    if (!bs->drv) {
+        return -ENOENT;
+    }
+
+    if (!bs->drv->bdrv_get_conversion_options) {
+        return -ENOTSUP;
+    }
+    return bs->drv->bdrv_get_conversion_options(bs, options);
+}
diff --git a/block.h b/block.h
index 4bdd82e..90720d9 100644
--- a/block.h
+++ b/block.h
@@ -257,6 +257,8 @@ int bdrv_in_use(BlockDriverState *bs);
 
 typedef struct BlockConversionOptions BlockConversionOptions;
 
+int bdrv_get_conversion_options(BlockDriverState *bs,
+                                BlockConversionOptions *options);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [PATCH V4 4/7] block: add bdrv_open_conversion_target()

[Devin Nakamura]

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

diff --git a/block.c b/block.c
index a3ea039..5ca396b 100644
--- a/block.c
+++ b/block.c
@@ -3282,6 +3282,32 @@ out:
     return ret;
 }
 
+int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
+                                BlockConversionOptions *drv_options,
+                                QEMUOptionParameter *usr_options,
+                                const char *target_fmt,
+                                bool force)
+{
+    BlockDriver *drv;
+    BlockDriverState *bss;
+    *bs = NULL;
+
+    drv = bdrv_find_format(target_fmt);
+    if (!drv) {
+        return -ENOENT;
+    }
+
+    if (!drv->bdrv_open_conversion_target) {
+        return -ENOTSUP;
+    }
+
+    *bs = bdrv_new("");
+    bss = *bs;
+    bss->file = file;
+    return bdrv_open_common(bss, file->filename, BDRV_O_CONVERSION, drv,
+                            drv_options, usr_options, force);
+}
+
 int bdrv_get_conversion_options(BlockDriverState *bs,
                                 BlockConversionOptions *options)
 {
diff --git a/block.h b/block.h
index 90720d9..77e3135 100644
--- a/block.h
+++ b/block.h
@@ -259,6 +259,11 @@ typedef struct BlockConversionOptions BlockConversionOptions;
 
 int bdrv_get_conversion_options(BlockDriverState *bs,
                                 BlockConversionOptions *options);
+int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
+                                BlockConversionOptions *drv_options,
+                                QEMUOptionParameter *usr_options,
+                                const char *target_fmt,
+                                bool force);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

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

[Devin Nakamura]

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

diff --git a/block.c b/block.c
index 5ca396b..ed62c4a 100644
--- a/block.c
+++ b/block.c
@@ -3320,3 +3320,18 @@ int bdrv_get_conversion_options(BlockDriverState *bs,
     }
     return bs->drv->bdrv_get_conversion_options(bs, 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 77e3135..c983d50 100644
--- a/block.h
+++ b/block.h
@@ -264,6 +264,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
                                 QEMUOptionParameter *usr_options,
                                 const char *target_fmt,
                                 bool force);
+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 V4 6/7] 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 ed62c4a..aff1d34 100644
--- a/block.c
+++ b/block.c
@@ -3335,3 +3335,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 c983d50..558b9b8 100644
--- a/block.h
+++ b/block.h
@@ -266,6 +266,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
                                 bool force);
 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 V4 7/7] block: add bdrv_copy_header()

[Devin Nakamura]

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

diff --git a/block.c b/block.c
index aff1d34..76bd2e9 100644
--- a/block.c
+++ b/block.c
@@ -3349,3 +3349,44 @@ int bdrv_map(BlockDriverState *bs, uint64_t guest_offset,
     return drv->bdrv_map(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+/**
+ * Copies out the header of a conversion target
+ *
+ * Saves the current header for the image in a temporary file and overwrites
+ * it with the header for the new format (at the moment the header is
+ * assumed to be 1 sector)
+ *
+ * A filename can be specified by using filename. On entry if
+ * filename = NULL , or filename points to an empty string, a random file
+ * name will be generated.
+ *
+ * If a random filename was generated it will be returned to the user by
+ * copying it to the location pointed at by filename. Filename must already
+ * be allocated to a size of at least PATH_MAX
+ *
+ * @param bs                Usualy opened with bdrv_open_conversion_target()
+ * @param filename[in,out]  On entry is the optional filename to be used. On
+ *                          exit it will be the filename used (assuming
+ *                          filename != NULL at entry)
+ * @return                  Returns non-zero on failure
+ */
+int bdrv_copy_header(BlockDriverState *bs, char *filename)
+{
+    BlockDriver *drv = bs->drv;
+    char tmp_filename[PATH_MAX] = "";
+
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_copy_header) {
+        return -ENOTSUP;
+    }
+    if (!filename) {
+        filename = tmp_filename;
+    }
+    if (!filename[0]) {
+        get_tmp_filename(filename, PATH_MAX);
+    }
+    return drv->bdrv_copy_header(bs, filename);
+}
diff --git a/block.h b/block.h
index 558b9b8..2fc546a 100644
--- a/block.h
+++ b/block.h
@@ -268,6 +268,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, char *filename);
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

Re: [Qemu-devel] [PATCH V4 0/7] Block Conversion

[Devin Nakamura]

On Tue, Aug 23, 2011 at 12:27 AM, Devin Nakamura <devin122@gmail.com> wrote:
> This patch series introduces the api to support block conversion.
>
> Devin Nakamura (7):
>  block: add block conversion api
>  block: make bdrv_open_common more ready to be called by
>    bdrv_open_conversion_target
>  block: add bdrv_get_conversion_options()
>  block: add bdrv_open_conversion_target()
>  block: add bdrv_get_mapping()
>  block: add bdrv_map()
>  block: add bdrv_copy_header()
>
>  block.c     |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  block.h     |   16 +++++++
>  block_int.h |   82 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+), 6 deletions(-)
>
> --
> 1.7.6.rc1
>
>

ping?

Re: [Qemu-devel] [PATCH V4 1/7] block: add block conversion api

[Kevin Wolf]

Am 23.08.2011 06:27, schrieb Devin Nakamura:
> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.h     |    3 ++
>  block_int.h |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 0 deletions(-)
> 
> diff --git a/block.h b/block.h
> index a3bfaaf..4bdd82e 100644
> --- a/block.h
> +++ b/block.h
> @@ -35,6 +35,7 @@ typedef struct QEMUSnapshotInfo {
>  #define BDRV_O_NATIVE_AIO  0x0080 /* use native AIO instead of the thread pool */
>  #define BDRV_O_NO_BACKING  0x0100 /* don't open the backing file */
>  #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
> +#define BDRV_O_CONVERSION  0x0400 /* open as conversion target */
>  
>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>  
> @@ -254,6 +255,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);
>  
> +typedef struct BlockConversionOptions BlockConversionOptions;
> +
>  typedef enum {
>      BLKDBG_L1_UPDATE,
>  
> diff --git a/block_int.h b/block_int.h
> index f6d02b3..66660f4 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -42,6 +42,9 @@
>  #define BLOCK_OPT_PREALLOC      "preallocation"
>  #define BLOCK_OPT_SUBFMT        "subformat"
>  
> +#define BLOCK_CRYPT_NONE    0
> +#define BLOCK_CRYPT_AES     1
> +
>  typedef struct AIOPool {
>      void (*cancel)(BlockDriverAIOCB *acb);
>      int aiocb_size;
> @@ -145,6 +148,79 @@ struct BlockDriver {
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /* In-place image conversion */
> +
> +    /**
> +     * Opens an image conversion target.
> +     *
> +     * @param bs          Basic Initialization done by
> +     *                    bdrv_open_conversion_target() Still need to set format
> +     *                    specific data.

I find this hard to understand. Can we rephrase it? Maybe something like
"Image to be opened. Basic initialization that doesn't depend on format
specific data must be completed."

(An interface like this shouldn't refer to bdrv_open_conversion_target,
which is an implementation detail of the user of the interface)

> +     * @param usr_options Creation options.
> +     * @param drv_options Conversion Options

Lower case "options"

> +     * @return            Returns non-zero on failure.
> +     */
> +    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
> +        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options,
> +        bool force);
> +
> +    /**
> +     * Gets a mapping from an offset in the image to an offset within a file
> +     *
> +     * All offsets are in bytes.
> +     *
> +     * @param guest_offset          The starting offset of the mapping
> +     * @param host_offset[out]      The starting file offset of the mapping.
> +     *                              A value of -1 is invalid and means the
> +     *                              cluster is unallocated (contiguous_bytes is
> +     *                              still valid)
> +     * @param contiguous_bytes[out] The number of bytes for which this mapping
> +     *                              is contiguous. If 0, there are no more
> +     *                              mapppings in the image

Typo in  "mappings"

> +     * @return                      Returns 0 on error;
> +     */
> +
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);
> +
> +    /**
> +     * Sets a mapping in the image file.
> +     *
> +     * @param bs               Usually opened with bdrv_open_conversion_target
> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes).
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes).
> +     * @param contiguous_bytes The number of bytes for which this mapping exists
> +     * @return                 Returns non-zero on error. Will return -EINVAL if
> +     *                         guest_offset, host_offset, or contiguous_bytes
> +     *                         not cluster aligned
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);
> +
> +    /**
> +     * Copies out the header of a conversion target
> +     *
> +     * Saves the current header for the image in a temporary file and overwrites
> +     * it with the header for the new format (at the moment the header is
> +     * assumed to be 1 sector)
> +     * @param bs        Usualy opened with bdrv_open_conversion_target()

"Must be opened..."? I don't think it makes sense with any other image.

> +     * @param filename  The name of the file to copy the header into
> +     * @return          Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs, char* filename);

const char*?

> +
> +    /**
> +     * Asks the block driver what options should be used to create a conversion
> +     * target.
> +     *
> +     * @param options[out] Block conversion options
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +         BlockConversionOptions *options);
> +
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -269,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, 0)
>  
> +struct BlockConversionOptions {
> +    int encryption_type;
> +    uint64_t image_size;
> +    uint64_t cluster_size;
> +    int nb_snapshots;
> +};

If you put the two ints together, I think you would avoid some padding.
Not that it really matters here...

Kevin

Re: [Qemu-devel] [PATCH V4 2/7] block: make bdrv_open_common more ready to be called by bdrv_open_conversion_target

[Kevin Wolf]

Am 23.08.2011 06:27, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a8a013a..ac5a103 100644
> --- a/block.c
> +++ b/block.c
> @@ -441,14 +441,17 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
>   * Common part for opening disk images and files
>   */
>  static int bdrv_open_common(BlockDriverState *bs, const char *filename,
> -    int flags, BlockDriver *drv)
> +    int flags, BlockDriver *drv, BlockConversionOptions *drv_options,
> +    QEMUOptionParameter *usr_options, bool force)
>  {
>      int ret, open_flags;
>  
>      assert(drv != NULL);
>  
> -    bs->file = NULL;
> -    bs->total_sectors = 0;
> +    if (~flags & BDRV_O_CONVERSION) {

I prefer if ((flags & BDRV_O_CONVERSION) == 0), but that's a matter of
taste I guess.

> +        bs->file = NULL;
> +        bs->total_sectors = 0;
> +    }

Why are these a problem from BDRV_O_CONVERSION? (Reading the rest of the
patch, I understand bs->file, but still not bs->total_sectors) Maybe add
a comment.

Kevin

Re: [Qemu-devel] [PATCH V4 0/7] Block Conversion

[Kevin Wolf]

Am 23.08.2011 06:27, schrieb Devin Nakamura:
> This patch series introduces the api to support block conversion.
> 
> Devin Nakamura (7):
>   block: add block conversion api
>   block: make bdrv_open_common more ready to be called by
>     bdrv_open_conversion_target
>   block: add bdrv_get_conversion_options()
>   block: add bdrv_open_conversion_target()
>   block: add bdrv_get_mapping()
>   block: add bdrv_map()
>   block: add bdrv_copy_header()
> 
>  block.c     |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  block.h     |   16 +++++++
>  block_int.h |   82 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+), 6 deletions(-)

Looks good to me apart from the minor comments on patches 1 and 2.

We can't merge interfaces into master without having users, though. So
as the interface itself seems to be final now, please continue with
posting the patches implementing this interface. (You can just include
more patches in a v5, or send a second patch series and add a note in
PATCH 0/n that it depends on this one)

Kevin