Re: [Qemu-devel] [PATCH V13 5/6] add-cow file format core code.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Oct 18, 2012 at 05:51:34PM +0800, Dong Xu Wang wrote:
> +static void add_cow_header_cpu_to_le(const AddCowHeader *cpu, AddCowHeader *le)
> +{
> +    le->magic                       = cpu_to_le64(cpu->magic);
> +    le->version                     = cpu_to_le32(cpu->version);
> +
> +    le->backing_filename_offset     = cpu_to_le32(cpu->backing_filename_offset);
> +    le->backing_filename_size       = cpu_to_le32(cpu->backing_filename_size);
> +
> +    le->image_filename_offset       = cpu_to_le32(cpu->image_filename_offset);
> +    le->image_filename_size         = cpu_to_le32(cpu->image_filename_size);
> +
> +    le->cluster_bits                = cpu_to_le32(cpu->cluster_bits);
> +    le->features                    = cpu_to_le64(cpu->features);
> +    le->optional_features           = cpu_to_le64(cpu->optional_features);
> +    le->header_pages_size           = cpu_to_le32(cpu->header_pages_size);
> +    memcpy(le->backing_fmt, cpu->backing_fmt, sizeof(cpu->backing_fmt));
> +    memcpy(le->image_fmt, cpu->image_fmt, sizeof(cpu->image_fmt));

Minor style issue: sizeof(le->backing_fmt) is safer than
sizeof(cpu->image_fmt) in case the types change or this code is
copy-pasted elsewhere.  Always use the size of the destination buffer.

> +}
> +
> +static int add_cow_probe(const uint8_t *buf, int buf_size, const char *filename)
> +{
> +    const AddCowHeader *header = (const AddCowHeader *)buf;
> +

In case .bdrv_probe() is exposed in a future stand-alone block libary
like libqblock.so where we cannot make assumptions about buf_size:

if (buf_size < sizeof(*header)) {
    return 0;
}

> +    ret = bdrv_file_open(&bs, filename, BDRV_O_RDWR);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    snprintf(header.backing_fmt, sizeof(header.backing_fmt),
> +             "%s", backing_fmt ? backing_fmt : "");
> +    snprintf(header.image_fmt, sizeof(header.image_fmt),
> +             "%s", image_format ? image_format : "raw");
> +    add_cow_header_cpu_to_le(&header, &le_header);
> +    ret = bdrv_pwrite(bs, 0, &le_header, sizeof(le_header));
> +    if (ret < 0) {
> +        bdrv_delete(bs);
> +        return ret;
> +    }

Once...

> +    if (ret < 0) {
> +        bdrv_delete(bs);
> +        return ret;
> +    }

...twice.  This can be dropped.

> +
> +    if (backing_filename) {
> +        ret = bdrv_pwrite(bs, header.backing_filename_offset,
> +                          backing_filename, header.backing_filename_size);
> +        if (ret < 0) {
> +            bdrv_delete(bs);
> +            return ret;
> +        }
> +    }
> +
> +    ret = bdrv_pwrite(bs, header.image_filename_offset,
> +                      image_filename, header.image_filename_size);
> +    if (ret < 0) {
> +        bdrv_delete(bs);
> +        return ret;
> +    }

I suggest writing the image filename before the backing filename so it's
easier to implement .bdrv_change_backing_file() in the future.

> +
> +    ret = bdrv_open(bs, filename, BDRV_O_RDWR | BDRV_O_NO_FLUSH, drv);

Forgot to bdrv_close(bs) before opening as add-cow.

> +    if ((s->header.features & ADD_COW_F_ALL_ALLOCATED) == 0) {
> +        ret = bdrv_read_string(bs->file, sizeof(s->header),
> +                               sizeof(bs->backing_format) - 1,
> +                               bs->backing_format,
> +                               sizeof(bs->backing_format));

This looks wrong:

1. The header contains the backing format field, we've already read it.
   Now we just need to put a NUL-terminated string into
   bs->backing_format.  No need for bdrv_read_string().

2. offset = sizeof(s->header) does not make sense because the
   backing_format field is part of the header.

3. n = sizeof(bs->backing_format) - 1 should be the size of the header
   backing_format field, not the destination buffer.

I'm wondering if I missed something or why add-cow files open
successfully in your testing, because I think this line of code would
cause it to use a junk bs->backing_format.

> +    s->image_hd = bdrv_new("");
> +    if (path_has_protocol(image_filename)) {

image_filename[] is uninitialized.  Did you mean tmp_name?

> +        pstrcpy(image_filename, sizeof(image_filename), tmp_name);
> +    } else {
> +        path_combine(image_filename, sizeof(image_filename),
> +                     bs->filename, tmp_name);
> +    }
> +
> +    ret = bdrv_open(s->image_hd, image_filename, flags, NULL);

What about header->image_format?

> +    if (ret < 0) {
> +        bdrv_delete(s->image_hd);
> +        goto fail;
> +    }
> +
> +    bs->total_sectors = bdrv_getlength(s->image_hd) >> 9;

/ BDRV_SECTOR_SIZE

> +    s->cluster_size = 1 << s->header.cluster_bits;
> +    sector_per_byte = SECTORS_PER_CLUSTER * 8;

SECTORS_PER_CLUSTER does not take s->cluster_size into account.

The add_cow_open() issues should have been visible during
development/testing (backing_format, unitialized image_filename[],
unused header->image_format, SECTORS_PER_CLUSTER).  It looks like not
much testing of image creation options has been done.  I'll review more
of this series in the next version, please test more.

Stefan