On Tue, Jul 5, 2011 at 12:31 PM, Fam Zheng <famc...@gmail.com> 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