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

login
register
mail settings
Submitter Feiran Zheng
Date June 27, 2011, 3:48 a.m.
Message ID <1309146518-8998-10-git-send-email-famcool@gmail.com>
Download mbox | patch
Permalink /patch/102096/
State New
Headers show

Comments

Feiran Zheng - June 27, 2011, 3:48 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 |  182 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 169 insertions(+), 13 deletions(-)
Stefan Hajnoczi - June 27, 2011, 4:54 a.m.
On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <famcool@gmail.com> wrote:
> Parse vmdk decriptor file and open mono flat image.
> @@ -598,6 +600,154 @@ 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 = strstr(desc, opt_name);
> +    int r;
> +    const char *end = desc + strlen(desc);
> +
> +    if (!opt_pos) {
> +        return -1;
> +    }
> +    opt_pos += strlen(opt_name) + 2;
> +    if (opt_pos >= end) {
> +        return -1;
> +    }
> +    r = sscanf(opt_pos, "%[^\"]s", buf);
> +    return r <= 0;
> +}

This is still unsafe.  Please see my comments on the previous version
of this patch.

> +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));
> +    ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));

Merge error?  Only need to bdrv_pread() once :).

Stefan
Feiran Zheng - June 27, 2011, 7 a.m.
On Mon, Jun 27, 2011 at 12:54 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <famcool@gmail.com> wrote:
>> Parse vmdk decriptor file and open mono flat image.
>> @@ -598,6 +600,154 @@ 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 = strstr(desc, opt_name);
>> +    int r;
>> +    const char *end = desc + strlen(desc);
>> +
>> +    if (!opt_pos) {
>> +        return -1;
>> +    }
>> +    opt_pos += strlen(opt_name) + 2;
>> +    if (opt_pos >= end) {
>> +        return -1;
>> +    }
>> +    r = sscanf(opt_pos, "%[^\"]s", buf);
>> +    return r <= 0;
>> +}
>
> This is still unsafe.  Please see my comments on the previous version
> of this patch.
How about this:

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;
    }
    strncpy(buf, opt_pos, opt_end - opt_pos);
    buf[opt_end - opt_pos] = '\0';
    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));
>> +    ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));
>
> Merge error?  Only need to bdrv_pread() once :).
>
> Stefan
>
Stefan Hajnoczi - June 27, 2011, 8:10 a.m.
On Mon, Jun 27, 2011 at 8:00 AM, Fam Zheng <famcool@gmail.com> wrote:
> On Mon, Jun 27, 2011 at 12:54 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <famcool@gmail.com> wrote:
>>> Parse vmdk decriptor file and open mono flat image.
>>> @@ -598,6 +600,154 @@ 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 = strstr(desc, opt_name);
>>> +    int r;
>>> +    const char *end = desc + strlen(desc);
>>> +
>>> +    if (!opt_pos) {
>>> +        return -1;
>>> +    }
>>> +    opt_pos += strlen(opt_name) + 2;
>>> +    if (opt_pos >= end) {
>>> +        return -1;
>>> +    }
>>> +    r = sscanf(opt_pos, "%[^\"]s", buf);
>>> +    return r <= 0;
>>> +}
>>
>> This is still unsafe.  Please see my comments on the previous version
>> of this patch.
> How about this:
>
> 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);

Already game over here because desc is not NUL-terminated.  Either
make desc NUL-terminated or add a desc_size argument.

>
>    opt_pos = strstr(desc, opt_name);

And again here.

>    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;
>    }
>    strncpy(buf, opt_pos, opt_end - opt_pos);
>    buf[opt_end - opt_pos] = '\0';

cutils.c:pstrcpy() is easier to use than strncpy(), no need for the
explicit NUL-termination.

Stefan

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index c5a02b3..3efad89 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;
@@ -384,9 +385,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;
     }
@@ -598,6 +600,154 @@  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 = strstr(desc, opt_name);
+    int r;
+    const char *end = desc + strlen(desc);
+
+    if (!opt_pos) {
+        return -1;
+    }
+    opt_pos += strlen(opt_name) + 2;
+    if (opt_pos >= end) {
+        return -1;
+    }
+    r = sscanf(opt_pos, "%[^\"]s", buf);
+    return r <= 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;
+    BDRVVmdkState *s = bs->opaque;
+
+    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;
+            }
+            ret = vmdk_add_extent(bs, extent_file, true, sectors,
+                            0, 0, 0, 0, sectors, &extent);
+            if (ret) {
+                qemu_free(s->extents);
+                return ret;
+            }
+            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));
+    ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));
+    if (ret < 0) {
+        return ret;
+    }
+    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;
@@ -612,7 +762,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);
     }
 }
 
@@ -693,7 +843,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;
     }
 
@@ -846,16 +996,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;
@@ -879,7 +1033,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) {
@@ -902,16 +1056,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;