From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qbx97-00031i-Cn for qemu-devel@nongnu.org; Wed, 29 Jun 2011 11:57:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qbx95-0003hu-Is for qemu-devel@nongnu.org; Wed, 29 Jun 2011 11:57:45 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:52055) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qbx95-0003hp-46 for qemu-devel@nongnu.org; Wed, 29 Jun 2011 11:57:43 -0400 Received: by gyg10 with SMTP id 10so621207gyg.4 for ; Wed, 29 Jun 2011 08:57:42 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1309224777-31024-10-git-send-email-famcool@gmail.com> References: <1309224777-31024-1-git-send-email-famcool@gmail.com> <1309224777-31024-10-git-send-email-famcool@gmail.com> Date: Wed, 29 Jun 2011 16:57:41 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, qemu-devel@nongnu.org, hch@lst.de On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng wrote: > +/* find an option value out of descriptor file */ > +static int vmdk_parse_description(const char *desc, const char *opt_name= , > + =A0 =A0 =A0 =A0char *buf, int buf_size) > +{ > + =A0 =A0char *opt_pos, *opt_end; > + =A0 =A0const char *end =3D desc + strlen(desc); > + > + =A0 =A0opt_pos =3D strstr(desc, opt_name); > + =A0 =A0if (!opt_pos) { > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} > + =A0 =A0/* Skip "=3D\"" following opt_name */ > + =A0 =A0opt_pos +=3D strlen(opt_name) + 2; > + =A0 =A0if (opt_pos >=3D end) { > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} 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 "=3D\"" follow the opt_name. Luckily we only use this for "createType" which will hopefully never cause false positives. > + =A0 =A0opt_end =3D opt_pos; > + =A0 =A0while (opt_end < end && *opt_end !=3D '"') { > + =A0 =A0 =A0 =A0opt_end++; > + =A0 =A0} > + =A0 =A0if (opt_end =3D=3D end || buf_size < opt_end - opt_pos + 1) { > + =A0 =A0 =A0 =A0return -1; > + =A0 =A0} > + =A0 =A0pstrcpy(buf, opt_end - opt_pos + 1, opt_pos); > + =A0 =A0return 0; > +} > + > +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, > + =A0 =A0 =A0 =A0const char *desc_file_path) > +{ > + =A0 =A0int ret =3D 0; > + =A0 =A0int r; > + =A0 =A0char access[11]; > + =A0 =A0char type[11]; > + =A0 =A0char fname[512]; > + =A0 =A0const char *p =3D desc; > + =A0 =A0int64_t sectors =3D 0; > + =A0 =A0int64_t flat_offset; > + > + =A0 =A0while (*p) { > + =A0 =A0 =A0 =A0if (strncmp(p, "RW", strlen("RW"))) { > + =A0 =A0 =A0 =A0 =A0 =A0goto next_line; > + =A0 =A0 =A0 =A0} This check is duplicated below and can be removed. > + =A0 =A0 =A0 =A0/* parse extent line: > + =A0 =A0 =A0 =A0 * RW [size in sectors] FLAT "file-name.vmdk" OFFSET > + =A0 =A0 =A0 =A0 * or > + =A0 =A0 =A0 =A0 * RW [size in sectors] SPARSE "file-name.vmdk" > + =A0 =A0 =A0 =A0 */ > + =A0 =A0 =A0 =A0flat_offset =3D -1; > + =A0 =A0 =A0 =A0sscanf(p, "%10s %lld %10s %512s", > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0access, §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. > + =A0 =A0 =A0 =A0if (!strcmp(type, "FLAT")) { > + =A0 =A0 =A0 =A0 =A0 =A0sscanf(p, "%10s %lld %10s %512s %lld", > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0access, §ors, type, fname, &flat_off= set); > + =A0 =A0 =A0 =A0 =A0 =A0if (flat_offset =3D=3D -1) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0} > + > + =A0 =A0 =A0 =A0/* trim the quotation marks around */ > + =A0 =A0 =A0 =A0if (fname[0] =3D=3D '"') { > + =A0 =A0 =A0 =A0 =A0 =A0memmove(fname, fname + 1, strlen(fname) + 1); This copies 1 byte too many, just strlen(fname) will do. > + =A0 =A0 =A0 =A0 =A0 =A0if (fname[strlen(fname) - 1] =3D=3D '"') { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fname[strlen(fname) - 1] =3D '\0'; > + =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0} If starts as fname[] =3D {'"', '\0'} then this if statement checks fname[-1] =3D=3D '"'! > + =A0 =A0 =A0 =A0if (!(strlen(access) && sectors && strlen(type) && strle= n(fname))) { > + =A0 =A0 =A0 =A0 =A0 =A0goto next_line; > + =A0 =A0 =A0 =A0} These can be replaced by checking the sscanf(3) return value above. Validating sectors is still a good idea though. > + =A0 =A0 =A0 =A0if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) { > + =A0 =A0 =A0 =A0 =A0 =A0goto next_line; > + =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0if (strcmp(access, "RW")) { > + =A0 =A0 =A0 =A0 =A0 =A0goto next_line; > + =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0ret++; This variable is unused. > + =A0 =A0 =A0 =A0/* save to extents array */ > + =A0 =A0 =A0 =A0if (!strcmp(type, "FLAT")) { > + =A0 =A0 =A0 =A0 =A0 =A0/* FLAT extent */ > + =A0 =A0 =A0 =A0 =A0 =A0char extent_path[PATH_MAX]; > + =A0 =A0 =A0 =A0 =A0 =A0BlockDriverState *extent_file; > + =A0 =A0 =A0 =A0 =A0 =A0BlockDriver *drv; > + =A0 =A0 =A0 =A0 =A0 =A0VmdkExtent *extent; > + > + =A0 =A0 =A0 =A0 =A0 =A0extent_file =3D bdrv_new(""); > + =A0 =A0 =A0 =A0 =A0 =A0drv =3D bdrv_find_format("file"); > + =A0 =A0 =A0 =A0 =A0 =A0if (!drv) { bdrv_delete(extent_file); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0 =A0 =A0path_combine(extent_path, sizeof(extent_path), > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0desc_file_path, fname); > + =A0 =A0 =A0 =A0 =A0 =A0r =3D bdrv_open(extent_file, extent_path, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BDRV_O_RDWR | BDRV_O_NO_BACKING,= drv); We should honor the bs->open_flags. Otherwise cache=3Dnone|writeback|writethrough|unsafe is ignored on the actual extent files. > + =A0 =A0 =A0 =A0 =A0 =A0if (r) { bdrv_delete(extent_file); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0 =A0 =A0extent =3D vmdk_add_extent(bs, extent_file, true= , sectors, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00, 0, 0, 0, sect= ors); > + =A0 =A0 =A0 =A0 =A0 =A0extent->flat_start_offset =3D flat_offset; > + =A0 =A0 =A0 =A0} else { > + =A0 =A0 =A0 =A0 =A0 =A0/* SPARSE extent, not supported for now */ > + =A0 =A0 =A0 =A0 =A0 =A0fprintf(stderr, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"VMDK: Not supported extent type \"%s\""= ".\n", type); > + =A0 =A0 =A0 =A0 =A0 =A0return -ENOTSUP; > + =A0 =A0 =A0 =A0} > +next_line: > + =A0 =A0 =A0 =A0/* move to next line */ > + =A0 =A0 =A0 =A0while (*p && *p !=3D '\n') { > + =A0 =A0 =A0 =A0 =A0 =A0p++; > + =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0p++; > + =A0 =A0} > + =A0 =A0return 0; > +} > + > +static int vmdk_open_desc_file(BlockDriverState *bs, int flags) > +{ > + =A0 =A0int ret; > + =A0 =A0char buf[2048]; > + =A0 =A0char ct[128]; > + =A0 =A0BDRVVmdkState *s =3D bs->opaque; > + > + =A0 =A0ret =3D bdrv_pread(bs->file, 0, buf, sizeof(buf)); > + =A0 =A0if (ret < 0) { > + =A0 =A0 =A0 =A0return ret; > + =A0 =A0} > + =A0 =A0buf[2047] =3D '\0'; > + =A0 =A0if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { > + =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0} > + =A0 =A0if (strcmp(ct, "monolithicFlat")) { > + =A0 =A0 =A0 =A0fprintf(stderr, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"VMDK: Not supported image type \"%s\"""= .\n", ct); > + =A0 =A0 =A0 =A0return -ENOTSUP; > + =A0 =A0} > + =A0 =A0s->desc_offset =3D 0; > + =A0 =A0ret =3D vmdk_parse_extents(buf, bs, bs->file->filename); > + =A0 =A0if (ret) { > + =A0 =A0 =A0 =A0return ret; > + =A0 =A0} > + > + =A0 =A0/* try to open parent images, if exist */ > + =A0 =A0if (vmdk_parent_open(bs)) { > + =A0 =A0 =A0 =A0qemu_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