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 <address@hidden>
> ---
>  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