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

[Fam Zheng]

On Fri, Jul 8, 2011 at 11:29 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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?
>
I think so. This function is not used to create snapshot, actually
it's called for backing file. It copies header and L1 table, allocate
L2 tables and leave them zero. This is equivalent to creating a new
extent. The only difference is to set descriptor options parentCID and
parantFileNameHint.

>> @@ -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
>



-- 
Best regards!
Fam Zheng