[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats |
Date: |
Fri, 8 Jul 2011 16:29:45 +0100 |
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
- [Qemu-devel] [PATCH v8 02/12] VMDK: bugfix, align offset to cluster in get_whole_cluster, (continued)
- [Qemu-devel] [PATCH v8 02/12] VMDK: bugfix, align offset to cluster in get_whole_cluster, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 03/12] VMDK: probe for monolithicFlat images, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 04/12] VMDK: separate vmdk_open by format version, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 05/12] VMDK: add field BDRVVmdkState.desc_offset, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 06/12] VMDK: flush multiple extents, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 07/12] VMDK: move 'static' cid_update flag to bs field, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 08/12] VMDK: change get_cluster_offset return type, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 09/12] VMDK: open/read/write for monolithicFlat image, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats, Fam Zheng, 2011/07/05
- Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats,
Stefan Hajnoczi <=
- [Qemu-devel] [PATCH v8 11/12] VMDK: fix coding style, Fam Zheng, 2011/07/05
- [Qemu-devel] [PATCH v8 12/12] block: add bdrv_get_allocated_file_size() operation, Fam Zheng, 2011/07/05
- Re: [Qemu-devel] [PATCH v8 00/12] Adding VMDK monolithic flat support, Stefan Hajnoczi, 2011/07/08