Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image

[Stefan Hajnoczi]

On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <address@hidden> wrote:
> +/* find an option value out of descriptor file */
> +static int vmdk_parse_description(const char *desc, const char *opt_name,
> +        char *buf, int buf_size)
> +{
> +    char *opt_pos, *opt_end;
> +    const char *end = desc + strlen(desc);
> +
> +    opt_pos = strstr(desc, opt_name);
> +    if (!opt_pos) {
> +        return -1;
> +    }
> +    /* Skip "=\"" following opt_name */
> +    opt_pos += strlen(opt_name) + 2;
> +    if (opt_pos >= end) {
> +        return -1;
> +    }

This can produce false positives because strstr(desc, opt_name) will
match anything that contains the opt_name string.  Also we don't
verify that "=\"" follow the opt_name.  Luckily we only use this for
"createType" which will hopefully never cause false positives.

> +    opt_end = opt_pos;
> +    while (opt_end < end && *opt_end != '"') {
> +        opt_end++;
> +    }
> +    if (opt_end == end || buf_size < opt_end - opt_pos + 1) {
> +        return -1;
> +    }
> +    pstrcpy(buf, opt_end - opt_pos + 1, opt_pos);
> +    return 0;
> +}
> +
> +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> +        const char *desc_file_path)
> +{
> +    int ret = 0;
> +    int r;
> +    char access[11];
> +    char type[11];
> +    char fname[512];
> +    const char *p = desc;
> +    int64_t sectors = 0;
> +    int64_t flat_offset;
> +
> +    while (*p) {
> +        if (strncmp(p, "RW", strlen("RW"))) {
> +            goto next_line;
> +        }

This check is duplicated below and can be removed.

> +        /* parse extent line:
> +         * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
> +         * or
> +         * RW [size in sectors] SPARSE "file-name.vmdk"
> +         */
> +        flat_offset = -1;
> +        sscanf(p, "%10s %lld %10s %512s",
> +                access, &sectors, type, fname);

Please check the sscanf(3) return value to ensure the format string
matched.  The value of access[], type[], fname[], sectors will be
unchanged at the point where sscanf(3) fails so your checks will not
work.

Declared as char fname[512] but using "%512s" format string.  The
format string needs to be 511 to leave space for the NUL terminator.

> +        if (!strcmp(type, "FLAT")) {
> +            sscanf(p, "%10s %lld %10s %512s %lld",
> +                access, &sectors, type, fname, &flat_offset);
> +            if (flat_offset == -1) {
> +                return -EINVAL;
> +            }
> +        }
> +
> +        /* trim the quotation marks around */
> +        if (fname[0] == '"') {
> +            memmove(fname, fname + 1, strlen(fname) + 1);

This copies 1 byte too many, just strlen(fname) will do.

> +            if (fname[strlen(fname) - 1] == '"') {
> +                fname[strlen(fname) - 1] = '\0';
> +            }
> +        }

If starts as fname[] = {'"', '\0'} then this if statement checks
fname[-1] == '"'!

> +        if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) {
> +            goto next_line;
> +        }

These can be replaced by checking the sscanf(3) return value above.
Validating sectors is still a good idea though.

> +        if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) {
> +            goto next_line;
> +        }
> +        if (strcmp(access, "RW")) {
> +            goto next_line;
> +        }
> +        ret++;

This variable is unused.

> +        /* save to extents array */
> +        if (!strcmp(type, "FLAT")) {
> +            /* FLAT extent */
> +            char extent_path[PATH_MAX];
> +            BlockDriverState *extent_file;
> +            BlockDriver *drv;
> +            VmdkExtent *extent;
> +
> +            extent_file = bdrv_new("");
> +            drv = bdrv_find_format("file");
> +            if (!drv) {

bdrv_delete(extent_file);

> +                return -EINVAL;
> +            }
> +            path_combine(extent_path, sizeof(extent_path),
> +                    desc_file_path, fname);
> +            r = bdrv_open(extent_file, extent_path,
> +                    BDRV_O_RDWR | BDRV_O_NO_BACKING, drv);

We should honor the bs->open_flags.  Otherwise
cache=none|writeback|writethrough|unsafe is ignored on the actual
extent files.

> +            if (r) {

bdrv_delete(extent_file);

> +                return -EINVAL;
> +            }
> +            extent = vmdk_add_extent(bs, extent_file, true, sectors,
> +                            0, 0, 0, 0, sectors);
> +            extent->flat_start_offset = flat_offset;
> +        } else {
> +            /* SPARSE extent, not supported for now */
> +            fprintf(stderr,
> +                "VMDK: Not supported extent type \"%s\""".\n", type);
> +            return -ENOTSUP;
> +        }
> +next_line:
> +        /* move to next line */
> +        while (*p && *p != '\n') {
> +            p++;
> +        }
> +        p++;
> +    }
> +    return 0;
> +}
> +
> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
> +{
> +    int ret;
> +    char buf[2048];
> +    char ct[128];
> +    BDRVVmdkState *s = bs->opaque;
> +
> +    ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    buf[2047] = '\0';
> +    if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> +        return -EINVAL;
> +    }
> +    if (strcmp(ct, "monolithicFlat")) {
> +        fprintf(stderr,
> +                "VMDK: Not supported image type \"%s\""".\n", ct);
> +        return -ENOTSUP;
> +    }
> +    s->desc_offset = 0;
> +    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    /* try to open parent images, if exist */
> +    if (vmdk_parent_open(bs)) {
> +        qemu_free(s->extents);

This duplicates extent freeing code from vmdk_close().  Please reuse
that (maybe by moving it into a vmdk_free_extents() function), it also
frees l1_table, l2_cache, and l1_backup_table.

Stefan