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

[Devin Nakamura]

On Mon, Aug 1, 2011 at 9:34 AM, Kevin Wolf <address@hidden> wrote:
>> +    /**
>> +     * 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?
I was just kept putting it off because for some reason I had the idea
that it would be a lot of work to change it.  I've done it now and it
turns out I only had to change about 3 line of code.

>> +
>> +    /**
>> +     * 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?
Yes, the header is in fact save in the current implementation. Its
always saved to headerbackup.temp, or something to that effect. Soon I
plan on replacing that with a randomly generated temp file and
informing the user of the name.  Also, I plan on implementing the
ability to override it with a command line option

>> +    uint64_t image_size;
>> +    uint64_t cluster_size;
>> +    uint64_t allocation_size;
>> +};
>
> Comments explaining the difference between cluster_size and allocation_size?
>
At the moment there is none, and I don't think allocation_size has a
future since the allocation size of the source image isn't really
important.