On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famc...@gmail.com> wrote:
> +/* find an option value out of descriptor file */
> +static int vmdk_parse_description(const char *desc, const char *opt_name,
> + char *buf, int buf_size)
> +{
> + char *opt_pos, *opt_end;
> + const char *end = desc + strlen(desc);
> +
> + opt_pos = strstr(desc, opt_name);
> + if (!opt_pos) {
> + return -1;
> + }
> + /* Skip "=\"" following opt_name */
> + opt_pos += strlen(opt_name) + 2;
> + if (opt_pos >= end) {
> + return -1;
> + }
This can produce false positives because strstr(desc, opt_name) will
match anything that contains the opt_name string. Also we don't
verify that "=\"" follow the opt_name. Luckily we only use this for
"createType" which will hopefully never cause false positives.
> + opt_end = opt_pos;
> + while (opt_end < end && *opt_end != '"') {
> + opt_end++;
> + }
> + if (opt_end == end || buf_size < opt_end - opt_pos + 1) {
> + return -1;
> + }
> + pstrcpy(buf, opt_end - opt_pos + 1, opt_pos);
> + return 0;
> +}
> +
> +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> + const char *desc_file_path)
> +{
> + int ret = 0;
> + int r;
> + char access[11];
> + char type[11];
> + char fname[512];
> + const char *p = desc;
> + int64_t sectors = 0;
> + int64_t flat_offset;
> +
> + while (*p) {
> + if (strncmp(p, "RW", strlen("RW"))) {
> + goto next_line;
> + }
This check is duplicated below and can be removed.
> + /* parse extent line:
> + * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
> + * or
> + * RW [size in sectors] SPARSE "file-name.vmdk"
> + */
> + flat_offset = -1;
> + sscanf(p, "%10s %lld %10s %512s",
> + access, §ors, type, fname);
Please check the sscanf(3) return value to ensure the format string
matched. The value of access[], type[], fname[], sectors will be
unchanged at the point where sscanf(3) fails so your checks will not
work.
Declared as char fname[512] but using "%512s" format string. The
format string needs to be 511 to leave space for the NUL terminator.
> + if (!strcmp(type, "FLAT")) {
> + sscanf(p, "%10s %lld %10s %512s %lld",
> + access, §ors, type, fname, &flat_offset);
> + if (flat_offset == -1) {
> + return -EINVAL;
> + }
> + }
> +
> + /* trim the quotation marks around */
> + if (fname[0] == '"') {
> + memmove(fname, fname + 1, strlen(fname) + 1);
This copies 1 byte too many, just strlen(fname) will do.
> + if (fname[strlen(fname) - 1] == '"') {
> + fname[strlen(fname) - 1] = '\0';
> + }
> + }
If starts as fname[] = {'"', '\0'} then this if statement checks
fname[-1] == '"'!
> + if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) {
> + goto next_line;
> + }
These can be replaced by checking the sscanf(3) return value above.
Validating sectors is still a good idea though.
> + if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) {
> + goto next_line;
> + }
> + if (strcmp(access, "RW")) {
> + goto next_line;
> + }
> + ret++;
This variable is unused.
> + /* save to extents array */
> + if (!strcmp(type, "FLAT")) {
> + /* FLAT extent */
> + char extent_path[PATH_MAX];
> + BlockDriverState *extent_file;
> + BlockDriver *drv;
> + VmdkExtent *extent;
> +
> + extent_file = bdrv_new("");
> + drv = bdrv_find_format("file");
> + if (!drv) {
bdrv_delete(extent_file);
> + return -EINVAL;
> + }
> + path_combine(extent_path, sizeof(extent_path),
> + desc_file_path, fname);
> + r = bdrv_open(extent_file, extent_path,
> + BDRV_O_RDWR | BDRV_O_NO_BACKING, drv);
We should honor the bs->open_flags. Otherwise
cache=none|writeback|writethrough|unsafe is ignored on the actual
extent files.
> + if (r) {
bdrv_delete(extent_file);
> + return -EINVAL;
> + }
> + extent = vmdk_add_extent(bs, extent_file, true, sectors,
> + 0, 0, 0, 0, sectors);
> + extent->flat_start_offset = flat_offset;
> + } else {
> + /* SPARSE extent, not supported for now */
> + fprintf(stderr,
> + "VMDK: Not supported extent type \"%s\""".\n", type);
> + return -ENOTSUP;
> + }
> +next_line:
> + /* move to next line */
> + while (*p && *p != '\n') {
> + p++;
> + }
> + p++;
> + }
> + return 0;
> +}
> +
> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
> +{
> + int ret;
> + char buf[2048];
> + char ct[128];
> + BDRVVmdkState *s = bs->opaque;
> +
> + ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));
> + if (ret < 0) {
> + return ret;
> + }
> + buf[2047] = '\0';
> + if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> + return -EINVAL;
> + }
> + if (strcmp(ct, "monolithicFlat")) {
> + fprintf(stderr,
> + "VMDK: Not supported image type \"%s\""".\n", ct);
> + return -ENOTSUP;
> + }
> + s->desc_offset = 0;
> + ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> + if (ret) {
> + return ret;
> + }
> +
> + /* try to open parent images, if exist */
> + if (vmdk_parent_open(bs)) {
> + qemu_free(s->extents);
This duplicates extent freeing code from vmdk_close(). Please reuse
that (maybe by moving it into a vmdk_free_extents() function), it also
frees l1_table, l2_cache, and l1_backup_table.
Stefan
