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
