Re: [Qemu-devel] [PATCH 06/12] VMDK: vmdk_open for mono flat

[Stefan Hajnoczi]

On Sat, Jun 4, 2011 at 1:42 AM, Fam Zheng <address@hidden> wrote:
> Vmdk_open for mono flat image.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/vmdk.c |  134 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 128 insertions(+), 6 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index b02a7b7..f1233cf 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -355,24 +355,110 @@ static int vmdk_parent_open(BlockDriverState *bs)
>     char desc[DESC_SIZE];
>     BDRVVmdkState *s = bs->opaque;
>
> -    if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE)
> +    if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
>         return -1;
> +    }
>
>     if ((p_name = strstr(desc,"parentFileNameHint")) != NULL) {

Same strstr(3) issue here in the existing vmdk code.

I suggest increasing the size by one, desc[DESC_SIZE + 1], and setting
the last char to '\0' so that strstr(3) never runs off the end of the
desc[] buffer.

>         char *end_name;
>
>         p_name += sizeof("parentFileNameHint") + 1;
> -        if ((end_name = strchr(p_name,'\"')) == NULL)
> +        if ((end_name = strchr(p_name,'\"')) == NULL) {
>             return -1;
> -        if ((end_name - p_name) > sizeof (bs->backing_file) - 1)
> +        }
> +        if ((end_name - p_name) > sizeof (bs->backing_file) - 1) {
>             return -1;
> -
> +        }
>         pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
>     }
> -
>     return 0;
>  }
>
> +/* 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 = strstr(desc, opt_name);
> +    int r;
> +    if (!opt_pos) {
> +        return -1;
> +    }
> +    opt_pos += strlen(opt_name) + 2;

Couldn't the opt_name match at the end of the descriptor?  This would
jump beyond the NUL terminator!

> +    r = sscanf(opt_pos, "%[^\"]s", buf);
> +    assert(r <= buf_size);

You cannot use assert for input validation.  File format parsing needs
to be bulletproof, there can be no crashes or asserts.

> +    return r <= 0;
> +}
> +
> +
> +static int vmdk_parse_extents(const char *desc, VmdkExtent extents[],
> +        const char *desc_file_path)
> +{
> +    int ret = 0;
> +    int r;
> +    char access[11];
> +    char type[11];
> +    char fname[512];
> +    VmdkExtent *extent;
> +    const char *p = desc;
> +    int64_t sectors = 0;
> +
> +    while (*p) {
> +        if (strncmp(p, "RW", strlen("RW")) == 0) {
> +            /* parse extent line:
> +             * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
> +             * or
> +             * RW [size in sectors] SPARSE "file-name.vmdk"
> +             */
> +
> +            sscanf(p, "%10s %lld %10s \"%[^\"]512s\"",
> +                    access, &sectors, type, fname);

You can combine the "RW" strncmp into the format string and then check
the sscanf(3) return value to see if the line matched.  Then you can
also avoid the big if statement.

> +            if (!(strlen(access) && sectors && strlen(type) &&
> strlen(fname))) {
> +                goto cont;
> +            }
> +            if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) {
> +                goto cont;
> +            }
> +            if (strcmp(access, "RW")) {
> +                goto cont;
> +            }
> +            ret++;
> +            if (!extents) {
> +                goto cont;
> +            }
> +
> +            /* save to extents array */
> +            if (!strcmp(type, "FLAT")) {
> +                /* FLAT extent */
> +                char extent_path[1024];

There is a constant for this: PATH_MAX

> +                path_combine(extent_path, sizeof(extent_path),
> +                        desc_file_path, fname);
> +                extent = &extents[ret - 1];
> +                extent->flat = true;
> +                extent->sectors = sectors;
> +                extent->cluster_sectors = sectors;
> +                extent->file = bdrv_new("");
> +                if (!extent->file) {
> +                    return -1;
> +                }
> +                r = bdrv_open(extent->file, extent_path,
> +                        BDRV_O_RDWR | BDRV_O_NO_BACKING, NULL);
> +                if (r) {
> +                    return -1;
> +                }
> +            } else {
> +                /* SPARSE extent, should not be here */
> +                fprintf(stderr, "VMDK: Not supported extent type.\n");

For troubleshooting purposes it would be useful to print the
unsupported extent type.

> +                return -1;
> +            }
> +        }
> +cont:
> +        /* move to next line */
> +        while (*p && *p != '\n') p++;
> +        p++;
> +    }
> +    return ret;
> +}
> +
>  static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
>  {
>     int l1_size, i;
> @@ -496,6 +582,42 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int 
> flags)
>     return -1;
>  }
>
> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags)

I think extent creation would be simpler if you introduced:
int vmdk_add_extent(BDRVVmdkState *s, const char *filename, bool flat, ...);

This function adds a new extent (perhaps use a linked list instead of
an array) and can be used from vmdk_parse_extents() so you don't need
to call it twice to figure out the number of extents and then to parse
it.

This function should also be used by the monolithic open functions
instead of assiging extent fields manually.

> +{
> +    char buf[2048];
> +    char ct[128];
> +    VmdkExtent *extent;
> +    BDRVVmdkState *s = bs->opaque;
> +
> +    if (bdrv_pread(bs->file, 0, buf, sizeof(buf)) == 0) {

bdrv_pread() returns negative on error.

> +        goto fail;
> +    }
> +    if (0 != vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> +        goto fail;
> +    }
> +    if (0 != strcmp(ct, "monolithicFlat")) {
> +        goto fail;
> +    }
> +    s->desc_offset = 0;
> +    s->num_extents = vmdk_parse_extents(buf, NULL, NULL);
> +    if  (!s->num_extents)
> +        goto fail;

Coding style {}.  Please run scripts/checkpatch.pl before submitting patches.

Stefan