From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qc6W5-0001xM-5e for qemu-devel@nongnu.org; Wed, 29 Jun 2011 21:58:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qc6W3-0007ns-II for qemu-devel@nongnu.org; Wed, 29 Jun 2011 21:58:05 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:61271) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qc6W3-0007no-5L for qemu-devel@nongnu.org; Wed, 29 Jun 2011 21:58:03 -0400 Received: by pzk30 with SMTP id 30so1586387pzk.4 for ; Wed, 29 Jun 2011 18:58:02 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1309224777-31024-1-git-send-email-famcool@gmail.com> <1309224777-31024-10-git-send-email-famcool@gmail.com> From: Fam Zheng Date: Thu, 30 Jun 2011 09:57:22 +0800 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Stefan Hajnoczi Cc: kwolf@redhat.com, qemu-devel@nongnu.org, hch@lst.de On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi wrot= e: > 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_nam= e, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0char *buf, int buf_size) >> +{ >> + =C2=A0 =C2=A0char *opt_pos, *opt_end; >> + =C2=A0 =C2=A0const char *end =3D desc + strlen(desc); >> + >> + =C2=A0 =C2=A0opt_pos =3D strstr(desc, opt_name); >> + =C2=A0 =C2=A0if (!opt_pos) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0/* Skip "=3D\"" following opt_name */ >> + =C2=A0 =C2=A0opt_pos +=3D strlen(opt_name) + 2; >> + =C2=A0 =C2=A0if (opt_pos >=3D end) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; >> + =C2=A0 =C2=A0} > > This can produce false positives because strstr(desc, opt_name) will > match anything that contains the opt_name string. =C2=A0Also we don't > verify that "=3D\"" follow the opt_name. =C2=A0Luckily we only use this f= or > "createType" which will hopefully never cause false positives. > >> + =C2=A0 =C2=A0opt_end =3D opt_pos; >> + =C2=A0 =C2=A0while (opt_end < end && *opt_end !=3D '"') { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0opt_end++; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0if (opt_end =3D=3D end || buf_size < opt_end - opt_pos + = 1) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0pstrcpy(buf, opt_end - opt_pos + 1, opt_pos); >> + =C2=A0 =C2=A0return 0; >> +} >> + >> +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0const char *desc_file_path) >> +{ >> + =C2=A0 =C2=A0int ret =3D 0; >> + =C2=A0 =C2=A0int r; >> + =C2=A0 =C2=A0char access[11]; >> + =C2=A0 =C2=A0char type[11]; >> + =C2=A0 =C2=A0char fname[512]; >> + =C2=A0 =C2=A0const char *p =3D desc; >> + =C2=A0 =C2=A0int64_t sectors =3D 0; >> + =C2=A0 =C2=A0int64_t flat_offset; >> + >> + =C2=A0 =C2=A0while (*p) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strncmp(p, "RW", strlen("RW"))) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto next_line; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > > This check is duplicated below and can be removed. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* parse extent line: >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * RW [size in sectors] FLAT "file-name.vmd= k" OFFSET >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * or >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * RW [size in sectors] SPARSE "file-name.v= mdk" >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0flat_offset =3D -1; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0sscanf(p, "%10s %lld %10s %512s", >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0access, §or= s, type, fname); > > Please check the sscanf(3) return value to ensure the format string > matched. =C2=A0The 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. =C2=A0The > format string needs to be 511 to leave space for the NUL terminator. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!strcmp(type, "FLAT")) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sscanf(p, "%10s %lld %10s %51= 2s %lld", >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0access, §or= s, type, fname, &flat_offset); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (flat_offset =3D=3D -1) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* trim the quotation marks around */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fname[0] =3D=3D '"') { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove(fname, fname + 1, str= len(fname) + 1); > > This copies 1 byte too many, just strlen(fname) will do. I meant to copy the NULL terminator too. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fname[strlen(fname) - 1] = =3D=3D '"') { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fname[strlen(fn= ame) - 1] =3D '\0'; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > > If starts as fname[] =3D {'"', '\0'} then this if statement checks > fname[-1] =3D=3D '"'! > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!(strlen(access) && sectors && strlen(t= ype) && strlen(fname))) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto next_line; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > > These can be replaced by checking the sscanf(3) return value above. > Validating sectors is still a good idea though. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strcmp(type, "FLAT") && strcmp(type, "S= PARSE")) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto next_line; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strcmp(access, "RW")) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto next_line; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0ret++; > > This variable is unused. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* save to extents array */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!strcmp(type, "FLAT")) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* FLAT extent */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0char extent_path[PATH_MAX]; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BlockDriverState *extent_file= ; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BlockDriver *drv; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0VmdkExtent *extent; >> + >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0extent_file =3D bdrv_new(""); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drv =3D bdrv_find_format("fil= e"); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!drv) { > > bdrv_delete(extent_file); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0path_combine(extent_path, siz= eof(extent_path), >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0d= esc_file_path, fname); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r =3D bdrv_open(extent_file, = extent_path, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0B= DRV_O_RDWR | BDRV_O_NO_BACKING, drv); > > We should honor the bs->open_flags. =C2=A0Otherwise > cache=3Dnone|writeback|writethrough|unsafe is ignored on the actual > extent files. > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r) { > > bdrv_delete(extent_file); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0extent =3D vmdk_add_extent(bs= , extent_file, true, sectors, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A00, 0, 0, 0, sectors); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0extent->flat_start_offset =3D= flat_offset; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* SPARSE extent, not support= ed for now */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"VMDK: Not supp= orted extent type \"%s\""".\n", type); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOTSUP; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> +next_line: >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* move to next line */ >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0while (*p && *p !=3D '\n') { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p++; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0p++; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0return 0; >> +} >> + >> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags) >> +{ >> + =C2=A0 =C2=A0int ret; >> + =C2=A0 =C2=A0char buf[2048]; >> + =C2=A0 =C2=A0char ct[128]; >> + =C2=A0 =C2=A0BDRVVmdkState *s =3D bs->opaque; >> + >> + =C2=A0 =C2=A0ret =3D bdrv_pread(bs->file, 0, buf, sizeof(buf)); >> + =C2=A0 =C2=A0if (ret < 0) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0buf[2047] =3D '\0'; >> + =C2=A0 =C2=A0if (vmdk_parse_description(buf, "createType", ct, sizeof(= ct))) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0if (strcmp(ct, "monolithicFlat")) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"VMDK: Not supp= orted image type \"%s\""".\n", ct); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOTSUP; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0s->desc_offset =3D 0; >> + =C2=A0 =C2=A0ret =3D vmdk_parse_extents(buf, bs, bs->file->filename); >> + =C2=A0 =C2=A0if (ret) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret; >> + =C2=A0 =C2=A0} >> + >> + =C2=A0 =C2=A0/* try to open parent images, if exist */ >> + =C2=A0 =C2=A0if (vmdk_parent_open(bs)) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(s->extents); > > This duplicates extent freeing code from vmdk_close(). =C2=A0Please reuse > that (maybe by moving it into a vmdk_free_extents() function), it also > frees l1_table, l2_cache, and l1_backup_table. > > Stefan > --=20 Best regards! Fam Zheng