Re: [Qemu-devel] [PATCH 1/6 v10] docs: spec for add-cow file format

[Kevin Wolf]

Am 14.06.2012 05:06, schrieb Dong Xu Wang:
> On Wed, Jun 13, 2012 at 11:10 PM, Eric Blake <address@hidden> wrote:
>> On 06/13/2012 08:36 AM, Dong Xu Wang wrote:
>>> Introduce a new file format:add-cow. The usage can be found at this patch.
>>>
>>> Signed-off-by: Dong Xu Wang <address@hidden>
>>> ---
>>>  docs/specs/add-cow.txt |   87 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 87 insertions(+), 0 deletions(-)
>>>  create mode 100644 docs/specs/add-cow.txt

>>> +
>>> +== Header ==
>>> +
>>> +The Header is included in the first bytes:
>>> +
>>> +    Byte    0 -  7:     magic
>>> +                        add-cow magic string ("ADD_COW\xff")
>>> +
>>> +            8 -  11:    version
>>> +                        Version number (only valid value is 1 now)
>>> +
>>> +            12 - 15:    backing_filename_offset
>>> +                        Offset in the add-cow file at which the backing 
>>> file name
>>> +                        is stored (NB: The string is not null terminated). 
>>> 0 if the
>>> +                        image doesn't have a backing file.
>>
>> Mention that if this is not 0, then it must be between 36 and 4094 (a
>> file name must be at least 1 byte).  What are the semantics if the
>> filename is relative?
> 
> relative filename is ok, I tested it just now.

I believe Eric wanted to know what a relative path means, i.e. that it's
relative to the image file rather than relative to the working directory.

>>> +
>>> +            16 - 19:    backing_filename_size
>>> +                        Length of the backing file name in bytes. 
>>> Undefined if the
>>> +                        image doesn't have a backing file.
>>
>> Better to require 0 if backing_filename_offset is 0, than to leave this
>> field undefined; also if backing_filename_offset is non-zero, then this
>> must be non-zero.  Must be less than 4096-36 to fit in the reserved part
>> of the header.
>>
> 
> Okay.

Does an add-cow image without a backing file even make sense?

>>> +            28 - 35:    features
>>> +                        Currently only 2 feature bits are used:
>>> +                        Feature bits:
>>> +                            The image uses a backing file:
>>> +                            * ADD_COW_F_BACKING_FILE = 0x01.
>>> +                            The backing file's format is raw:
>>> +                            * ADD_COW_F_BACKING_FORMAT_NO_PROBE = 0x02.
>>
>> Should this follow the qcow2v3 proposal of splitting into mandatory vs.
>> optional feature bits?
>>
>> I agree that ADD_COW_F_BACKING_FORMAT_NO_PROBE is sufficient to avoid
>> security implications, but do we want the extra flexibility of
>> specifying the backing format file format rather than just requiring
>> probes on all but raw?
> 
> Kevin, or Stefan, can you give some comments for this? thanks.

I tend to agree that a format name is better than relying on probing.

Also, I think we need the same thing for image_file. add-cow is not only
useful for raw images, but also for other image format types for which
we don't support backing files.

>>> +== Reserved ==
>>> +
>>> +    Byte    36 - 4095:  Reserved field:
>>> +                        It is used to make sure COW bitmap field starts at 
>>> the
>>> +                        4096th byte, backing_file name and image_file name 
>>> will
>>> +                        be stored here.
>>
>> Do we want to keep a fixed-size header, or should we be planning on the
>> possibility of future extensions requiring enough other header
>> extensions that a variable-sized header would be wiser?  That is, I'm
>> fine with requiring that the header be a multiple of 4k, but maybe it
>> would make sense to have a mandatory header field that states how many
>> header pages are present before the COW bitmap begins.  In the first
>> round of implementation, this header field can be required to be 1 (that
>> is, for now, we require exactly 4k header), but having the field would
>> let us change in the future to a design with an 8k header to hold more
>> metadata as needed.

I have the impression that this simple add-cow hack is starting to get
seriously overengineered... :-)

Kevin