Patchwork [v5,09/12] VMDK: open/read/write for monolithicFlat image

login
register
mail settings
Submitter Feiran Zheng
Date June 28, 2011, 1:32 a.m.
Message ID <1309224777-31024-10-git-send-email-famcool@gmail.com>
Download mbox | patch
Permalink /patch/102306/
State New
Headers show

Comments

Feiran Zheng - June 28, 2011, 1:32 a.m.
Parse vmdk decriptor file and open mono flat image.
Read/write the flat extent.

Signed-off-by: Fam Zheng <famcool@gmail.com>
---
 block/vmdk.c |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 172 insertions(+), 13 deletions(-)
Stefan Hajnoczi - June 29, 2011, 3:57 p.m.
On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> 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
Feiran Zheng - June 30, 2011, 1:57 a.m.
On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> 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.
I meant to copy the NULL terminator too.
>
>> +            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
>
Stefan Hajnoczi - June 30, 2011, 6:13 a.m.
On Thu, Jun 30, 2011 at 2:57 AM, Fam Zheng <famcool@gmail.com> wrote:
> On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> wrote:
>>> +        /* 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.
> I meant to copy the NULL terminator too.

Yes.  The problem is the copying starts at fname + 1 but strlen(3)
starts at fname.  So there is already an extra byte.  The + 1 adds an
additional byte after the NUL.


Stefan

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index c84ea90..8083c31 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -65,6 +65,7 @@  typedef struct VmdkExtent {
     bool flat;
     int64_t sectors;
     int64_t end_sector;
+    int64_t flat_start_offset;
     int64_t l1_table_offset;
     int64_t l1_backup_table_offset;
     uint32_t *l1_table;
@@ -390,9 +391,10 @@  fail:
 static int vmdk_parent_open(BlockDriverState *bs)
 {
     char *p_name;
-    char desc[DESC_SIZE];
+    char desc[DESC_SIZE + 1];
     BDRVVmdkState *s = bs->opaque;
 
+    desc[DESC_SIZE] = '\0';
     if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
         return -1;
     }
@@ -588,6 +590,157 @@  static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
     return ret;
 }
 
+/* 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;
+    }
+    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;
+        }
+        /* 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);
+        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);
+            if (fname[strlen(fname) - 1] == '"') {
+                fname[strlen(fname) - 1] = '\0';
+            }
+        }
+        if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) {
+            goto next_line;
+        }
+        if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) {
+            goto next_line;
+        }
+        if (strcmp(access, "RW")) {
+            goto next_line;
+        }
+        ret++;
+
+        /* 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) {
+                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);
+            if (r) {
+                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);
+        return -EINVAL;
+    }
+    s->parent_cid = vmdk_read_cid(bs, 1);
+    return 0;
+}
+
 static int vmdk_open(BlockDriverState *bs, int flags)
 {
     uint32_t magic;
@@ -602,7 +755,7 @@  static int vmdk_open(BlockDriverState *bs, int flags)
     } else if (magic == VMDK4_MAGIC) {
         return vmdk_open_vmdk4(bs, flags);
     } else {
-        return -EINVAL;
+        return vmdk_open_desc_file(bs, flags);
     }
 }
 
@@ -683,7 +836,7 @@  static int get_cluster_offset(BlockDriverState *bs,
     if (m_data)
         m_data->valid = 0;
     if (extent->flat) {
-        *cluster_offset = 0;
+        *cluster_offset = extent->flat_start_offset;
         return 0;
     }
 
@@ -836,16 +989,20 @@  static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
             /* if not allocated, try to read from parent image, if exist */
             if (bs->backing_hd) {
                 if (!vmdk_is_cid_valid(bs))
-                    return -1;
+                    return -EINVAL;
                 ret = bdrv_read(bs->backing_hd, sector_num, buf, n);
                 if (ret < 0)
-                    return -1;
+                    return ret;
             } else {
                 memset(buf, 0, 512 * n);
             }
         } else {
-            if(bdrv_pread(bs->file, cluster_offset + index_in_cluster * 512, buf, n * 512) != n * 512)
-                return -1;
+            ret = bdrv_pread(extent->file,
+                            cluster_offset + index_in_cluster * 512,
+                            buf, n * 512);
+            if (ret != n * 512) {
+                return ret;
+            }
         }
         nb_sectors -= n;
         sector_num += n;
@@ -869,7 +1026,7 @@  static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
                 "(VMDK) Wrong offset: sector_num=0x%" PRIx64
                 " total_sectors=0x%" PRIx64 "\n",
                 sector_num, bs->total_sectors);
-        return -1;
+        return -EIO;
     }
 
     while (nb_sectors > 0) {
@@ -892,16 +1049,18 @@  static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
             n = nb_sectors;
         }
 
-        if (bdrv_pwrite(bs->file,
+       ret = bdrv_pwrite(extent->file,
                         cluster_offset + index_in_cluster * 512,
-                        buf, n * 512)
-                != n * 512) {
-            return -1;
+                        buf,
+                        n * 512);
+       if (ret != n * 512) {
+            ret = ret < 0 ? ret : 0;
+            return ret;
         }
         if (m_data.valid) {
             /* update L2 tables */
             if (vmdk_L2update(extent, &m_data) == -1) {
-                return -1;
+                return -EIO;
             }
         }
         nb_sectors -= n;