Re: [Qemu-devel] [PATCH v6] block:add-cow file format

[Stefan Hajnoczi]

On Sat, Dec 31, 2011 at 9:17 AM, Dong Xu Wang
<address@hidden> wrote:
> On Fri, Dec 30, 2011 at 22:09, Stefan Hajnoczi <address@hidden> wrote:
>> On Thu, Dec 29, 2011 at 05:36:59PM +0800, Dong Xu Wang wrote:
>> > +    ret = bdrv_file_open(&backing_bs, bs->backing_file, 0);
>> > +    if (ret < 0) {
>> > +        return ret;
>> > +    }
>> > +    bdrv_delete(backing_bs);
>>
>> What does this do?  (It leaks s->bitmap when it fails.)
>
>
> I wanna make sure backing_file exists while opening.

block.c:bdrv_open() opens the backing file after .bdrv_open() has
returned.  It fails if the backing file does not exist.  There's no
need to duplicate this.

>> > +    s->image_hd = bdrv_new("");
>> > +
>> > +    if (path_has_protocol(s->image_file)) {
>> > +        pstrcpy(image_filename, sizeof(image_filename),
>> > +                s->image_file);
>> > +    } else {
>> > +        path_combine(image_filename, sizeof(image_filename),
>> > +                     bs->filename, s->image_file);
>> > +    }
>> > +
>> > +    image_drv = bdrv_find_format("raw");
>> > +    ret = bdrv_open(s->image_hd, image_filename, flags, image_drv);
>> > +    if (ret < 0) {
>> > +        bdrv_delete(s->image_hd);
>> > +        s->image_hd = NULL;
>> > +        goto fail;
>> > +    }
>>
>> TODO

If you were wondering why I put "TODO" here it's because I had some
thoughts when reviewing but didn't fully investigate it yet.  When I
send the rest of my feedback I'll include my comment here.  (I should
have removed this before replying :))

>> > +         1036 - 2059:   image_file
>> > +                        image_file is a raw file, While using
>> > image_file, must
>> > +                        together with image_file. All unused bytes are
>> > padded
>>
>> "While using image_file, must together with image_file"
>>
>> What does this mean?
>
>
> I mean while using add-cow, must together with image_file and backing_file.
> Both of them can not be missing.
> Errors with sentences like that, I will correct them in v7.

That sounds like qemu-img create behavior which should not be part of
the file format specification.

The only impact on the file format speficiation is that backing_file
can be an empty string but image_file must always be a valid filename.