Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats

[Stefan Hajnoczi]

On Tue, Jul 5, 2011 at 12:31 PM, Fam Zheng <address@hidden> wrote:
> Add create option 'format', with enums:

The -drive format=... option exists in QEMU today to specify the image
format of a file.  I think adding a format=... creation option may
lead to confusion.

How about subformat=... or type=...?

> Each creates a subformat image file. The default is monolithiSparse.

s/monolithiSparse/monolithicSparse/

> @@ -243,168 +243,6 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
>     return 1;
>  }
>
> -static int vmdk_snapshot_create(const char *filename, const char 
> *backing_file)

Is this function really not needed anymore?

> @@ -1189,28 +990,317 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options)
>         }
>     }
>
> -    /* compose the descriptor */
> -    real_filename = filename;
> -    if ((temp_str = strrchr(real_filename, '\\')) != NULL)
> -        real_filename = temp_str + 1;
> -    if ((temp_str = strrchr(real_filename, '/')) != NULL)
> -        real_filename = temp_str + 1;
> -    if ((temp_str = strrchr(real_filename, ':')) != NULL)
> -        real_filename = temp_str + 1;
> -    snprintf(desc, sizeof(desc), desc_template, (unsigned int)time(NULL),
> -             total_size, real_filename,
> -             (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> -             total_size / (int64_t)(63 * 16));
> -
> -    /* write the descriptor */
> -    lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
> -    ret = qemu_write_full(fd, desc, strlen(desc));
> -    if (ret != strlen(desc)) {
> +    filesize -= filesize;

What is the point of setting filesize to zero?

> +    ret = 0;
> + exit:
> +    close(fd);
> +    return ret;
> +}
> +
> +static int vmdk_create_flat(const char *filename, int64_t filesize)
> +{
> +    int fd, ret;
> +
> +    fd = open(
> +            filename,
> +            O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +            0644);
> +    if (fd < 0) {
> +        return -errno;
> +    }
> +    ret = ftruncate(fd, filesize);
> +    if (ret) {
>         ret = -errno;
> -        goto exit;
> +        close(fd);
> +        return -errno;

errno is a global variable that may be modified by any errno-using
library function.  Its value may be changed by close(2) (even if there
is no error closing the fd).  Therefore please do return ret instead
of return -errno.

>     }
> +    close(fd);
> +    return 0;
> +}
>
> -    ret = 0;
> +static int filename_decompose(const char *filename, char *path, char *prefix,
> +        char *postfix, int buf_len)

Memory sizes (e.g. buffer size) should be size_t (which is unsigned)
instead of int.

> +{
> +    const char *p, *q;
> +
> +    if (filename == NULL || !strlen(filename)) {
> +        fprintf(stderr, "Vmdk: wrong filename (%s)\n", filename);

Printing filename doesn't make sense since filename is either NULL or
"".  Also note that fprintf(..., "%s", NULL) is undefined and may
crash on some platforms (e.g. Solaris).

> +        return -1;
> +    }
> +    p = strrchr(filename, '/');
> +    if (p == NULL) {
> +        p = strrchr(filename, '\\');
> +    }
> +    if (p == NULL) {
> +        p = strrchr(filename, ':');
> +    }
> +    if (p != NULL) {
> +        p++;
> +        if (p - filename >= buf_len) {
> +            return -1;
> +        }
> +        strncpy(path, filename, p - filename);
> +        path[p - filename] = 0;
> +    } else {
> +        p = filename;
> +        path[0] = '\0';
> +    }
> +    q = strrchr(p, '.');
> +    if (q == NULL) {
> +        pstrcpy(prefix, buf_len, p);
> +        postfix[0] = '\0';
> +    } else {

No check for prefix buf_len here.  Imagine filename has no '/', '\\',
or ':' but it does have a '.'.  It is possible to overflow prefix.

> +        strncpy(prefix, p, q - p);
> +        prefix[q - p] = '\0';
> +        pstrcpy(postfix, buf_len, q);
> +    }
> +    return 0;
> +}
> +
> +static int relative_path(char *dest, int dest_size,
> +        const char *base, const char *target)
> +{
> +    int i = 0;
> +    int n = 0;
> +    const char *p, *q;
> +#ifdef _WIN32
> +    const char *sep = "\\";
> +#else
> +    const char *sep = "/";
> +#endif
> +
> +    if (!(dest && base && target)) {
> +        return -1;
> +    }
> +    if (path_is_absolute(target)) {
> +        dest[dest_size - 1] = '\0';
> +        strncpy(dest, target, dest_size - 1);
> +        return 0;
> +    }
> +    while (base[i] == target[i]) {
> +        i++;
> +    }
> +    p = &base[i];
> +    q = &target[i];
> +    while (*p) {
> +        if (*p == *sep) {
> +            n++;
> +        }
> +        p++;
> +    }
> +    dest[0] = '\0';
> +    for (; n; n--) {
> +        pstrcat(dest, dest_size, "..");
> +        pstrcat(dest, dest_size, sep);
> +    }
> +    pstrcat(dest, dest_size, q);
> +    return 0;
> +}
> +
> +static int vmdk_create(const char *filename, QEMUOptionParameter *options)
> +{
> +    int fd = -1;
> +    char desc[4096];
> +    int64_t total_size = 0;
> +    const char *backing_file = NULL;
> +    const char *fmt = NULL;
> +    int flags = 0;
> +    int ret = 0;
> +    char ext_desc_lines[1024] = "";
> +    char path[1024], prefix[1024], postfix[1024];
> +    const int64_t split_size = 0x80000000;  /* VMDK has constant split size 
> */
> +    const char desc_template[] =
> +        "# Disk DescriptorFile\n"
> +        "version=1\n"
> +        "CID=%x\n"
> +        "parentCID=%x\n"
> +        "createType=\"%s\"\n"
> +        "%s"
> +        "\n"
> +        "# Extent description\n"
> +        "%s"
> +        "\n"
> +        "# The Disk Data Base\n"
> +        "#DDB\n"
> +        "\n"
> +        "ddb.virtualHWVersion = \"%d\"\n"
> +        "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
> +        "ddb.geometry.heads = \"16\"\n"
> +        "ddb.geometry.sectors = \"63\"\n"
> +        "ddb.adapterType = \"ide\"\n";
> +
> +    if (filename_decompose(filename, path, prefix, postfix, 1024)) {

Please don't hardcode buffer sizes, if one of path, prefix, postfix
ever needs to be changed then this code also needs to be updated.  I
suggest defining a constant and using it everywhere.

> +        return -EINVAL;
> +    }
> +    /* Read out options */
> +    while (options && options->name) {
> +        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> +            total_size = options->value.n;
> +        } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> +            backing_file = options->value.s;
> +        } else if (!strcmp(options->name, BLOCK_OPT_COMPAT6)) {
> +            flags |= options->value.n ? BLOCK_FLAG_COMPAT6 : 0;
> +        } else if (!strcmp(options->name, BLOCK_OPT_FMT)) {
> +            fmt = options->value.s;
> +        }
> +        options++;
> +    }
> +    if (!fmt) {
> +        fmt = "monolithicSparse";
> +    }
> +    if (!strcmp(fmt, "monolithicFlat") || !strcmp(fmt, 
> "twoGbMaxExtentFlat")) {
> +        bool split = strcmp(fmt, "monolithicFlat");
> +        const char desc_line_templ[] = "RW %lld FLAT \"%s\" 0\n";
> +        int64_t filesize = total_size;
> +        int idx = 1;
> +
> +        if (backing_file) {
> +            /* not supporting backing file for flat image */
> +            return -ENOTSUP;
> +        }
> +        while (filesize > 0) {
> +            char desc_line[1024];
> +            char ext_filename[1024];
> +            char desc_filename[1024];

Buffer sizes again.

> +            int64_t size = filesize;
> +
> +            if (split && size > split_size) {
> +                size = split_size;
> +            }
> +            if (split) {
> +                sprintf(desc_filename, "%s-f%03d%s",
> +                        prefix, idx++, postfix);

snprintf?

> +                sprintf(ext_filename, "%s%s",
> +                        path, desc_filename);
> +            } else {
> +                sprintf(desc_filename, "%s-flat%s",
> +                        prefix, postfix);
> +                sprintf(ext_filename, "%s%s",
> +                        path, desc_filename);
> +            }
> +            if (vmdk_create_flat(ext_filename, size)) {
> +                return -EINVAL;
> +            }
> +            filesize -= size;
> +
> +            /* Format description line */
> +            snprintf(desc_line, 1024,
> +                        desc_line_templ, size / 512, desc_filename);

Here sizeof(desc_line) should be used as the buffer size to avoid
duplicating it.  Same thing below in the code.

> +            pstrcat(ext_desc_lines, sizeof(ext_desc_lines), desc_line);
> +        }
> +
> +        /* generate descriptor file */
> +        snprintf(desc, sizeof(desc), desc_template,
> +                (unsigned int)time(NULL),
> +                0xffffffff,
> +                fmt,
> +                "",
> +                ext_desc_lines,
> +                (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> +                total_size / (int64_t)(63 * 16 * 512));
> +        fd = open(
> +                filename,
> +                O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +                0644);
> +        if (fd < 0) {
> +            return -errno;
> +        }
> +        ret = qemu_write_full(fd, desc, strlen(desc));
> +        if (ret != strlen(desc)) {
> +            ret = -errno;
> +            goto exit;
> +        }
> +        ret = 0;
> +    } else if (!strcmp(fmt, "monolithicSparse")
> +                || !strcmp(fmt, "twoGbMaxExtentSparse")) {
> +        int ret;
> +        int fd = 0;
> +        int idx = 1;
> +        int64_t filesize = total_size;
> +        const char desc_line_templ[] = "RW %lld SPARSE \"%s\"\n";
> +        char desc_line[1024];
> +        char desc_filename[1024];
> +        char ext_filename[1024];
> +        bool split = strcmp(fmt, "monolithicSparse");
> +        char parent_desc_line[1024] = "";
> +        uint32_t parent_cid = 0xffffffff;
> +
> +        if (backing_file) {
> +            char parent_filename[1024];
> +            BlockDriverState *bs = bdrv_new("");
> +            ret = bdrv_open(bs, backing_file, 0, NULL);
> +            if (ret != 0) {
> +                bdrv_delete(bs);
> +                return ret;
> +            }
> +            filesize = bdrv_getlength(bs);
> +            parent_cid = vmdk_read_cid(bs, 0);

This assumes that the backing file is vmdk.  Where was that checked?

Stefan