Re: [Qemu-devel] [PATCH 11/12] VMDK: vmdk_create and options for mono flat image

[Stefan Hajnoczi]

On Sat, Jun 4, 2011 at 1:44 AM, Fam Zheng <address@hidden> wrote:
> +    if (flat) {

The flat and !flat cases are too big, please split them out into functions.

> +        const char desc_template[] =
> +        "# Disk DescriptorFile\n"
> +        "version=1\n"
> +        "CID=%x\n"
> +        "parentCID=ffffffff\n"
> +        "createType=\"monolithicFlat\"\n"
> +        "\n"
> +        "# Extent description\n"
> +        "RW %" PRId64 " FLAT \"%s\" 0\n"
> +        "\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";

This is almost identical to the desc_template[] below.  Please use
createType=%s and extent type format specifiers to share this template
instead of copy-pasting it.

> +        char ext_filename[1024];
> +        strncpy(ext_filename, filename, 1024);
> +        ext_filename[1023] = '\0';
> +        if (backing_file) {
> +            /* not supporting backing file for flat image */
> +            return -1;
> +        }
> +        if (!strcmp(&ext_filename[strlen(ext_filename) - 5], ".vmdk"))
> +            strcpy(&ext_filename[strlen(ext_filename) - 5], "-flat.vmdk");
> +        else
> +            strcat(ext_filename, "-flat.vmdk");
> +        /* create extent first */
> +        fd = open(
> +                ext_filename,
> +                O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +                0644);
> +        if (fd < 0)
> +            return -errno;
> +        ret = ftruncate(fd, total_size * 512);
> +        if (ret) goto exit;
> +        close(fd);
> +
> +        /* generate descriptor file */
> +        snprintf(desc, sizeof(desc), desc_template, (unsigned int)time(NULL),
> +                 total_size, ext_filename,
> +                 (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> +                 total_size / (int64_t)(63 * 16));
> +        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 {
> +        const char desc_template[] =
> +        "# Disk DescriptorFile\n"
> +        "version=1\n"
> +        "CID=%x\n"
> +        "parentCID=ffffffff\n"
> +        "createType=\"monolithicSparse\"\n"
> +        "\n"
> +        "# Extent description\n"
> +        "RW %" PRId64 " SPARSE \"%s\"\n"
> +        "\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";
> +        /* XXX: add support for backing file */

Is this comment still true?  It seems there is support for a backing
file via vmdk_snapshot_create().

Stefan