qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC 01/24] block: add block conversion api


From: Kevin Wolf
Subject: Re: [Qemu-devel] [RFC 01/24] block: add block conversion api
Date: Mon, 01 Aug 2011 15:34:51 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20110707 Thunderbird/5.0

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <address@hidden>
> ---
>  block.h     |    2 +
>  block_int.h |   88 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 90 insertions(+), 0 deletions(-)
> 
> diff --git a/block.h b/block.h
> index 59cc410..a1c4cc8 100644
> --- a/block.h
> +++ b/block.h
> @@ -251,6 +251,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 efb6803..84bf89e 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -41,6 +41,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;
> @@ -139,6 +142,85 @@ 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);
> +
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes
> +     *
> +     * @param bs[in]                   The image in which to find mapping.
> +     * @param guest_offset[in,out]     On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the starting
> +     *                                 guest offset of the mapping.
> +     * @param host_offset[out]         The starting image file offset for the
> +     *                                 mapping.
> +     * @param contiguous_bytes[in,out] On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the number 
> of
> +     *                                 bytes for which this mapping is valid.
> +     *                                 A value of 0 means there are no more
> +     *                                 mappings in the image.
> +     * @return                         Returns non-zero on error.
> +     */
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);

Last time I suggested to change the semantics of this function as follows:

> I think it would be less surprising if the function worked like
> bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
> always returns information for exactly the given offset. It would return
> if the range starting at the given offset is used or free.
> 
> If the caller wants to find the next existing mapping, he can just take
> contiguous_bytes of the free region and add it to his current offset.

Do you believe that this change would be a bad idea or did you just
forget it?

> +    /**
> +     * Sets a mapping in the image file.
> +     *
> +     * @param bs               Usualy opened with bdrv_open_conversion_target

Typo: "Usually"

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes). Function fails and returns 
> -EINVAL if
> +     *                         not cluster aligned.
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes). Function fails and returns 
> -EINVAL if
> +     *                         not cluster aligned.
> +     * @param contiguous_bytes The number of bytes for which this mapping 
> exists
> +     *                         Function fails and returns -EINVAL if not 
> cluster
> +     *                         aligned
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

Usually you would describe the -EINVAL part as part of @return. Not that
it's unclear this way, just gets a bit longer than necessary.

> +
> +    /**
> +     * 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().
> +     * @return    Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

Is it true with the current implementation that the old header is saved?
I think it's supposed to be used if updating the header goes wrong. Who
will read the temporary file in this case? Does the user need to know
where it is or even have control over it?

> +
> +    /**
> +     * 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;
>  };
>  
> @@ -263,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;

This could be an enum.

> +    uint64_t image_size;
> +    uint64_t cluster_size;
> +    uint64_t allocation_size;
> +};

Comments explaining the difference between cluster_size and allocation_size?

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]