From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39818) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vnfju-0008Jh-Rb for qemu-devel@nongnu.org; Mon, 02 Dec 2013 21:29:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vnfjo-00048f-J5 for qemu-devel@nongnu.org; Mon, 02 Dec 2013 21:29:30 -0500 Message-ID: <529D41FA.2060705@redhat.com> Date: Tue, 03 Dec 2013 10:29:14 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1385953280-7390-1-git-send-email-famz@redhat.com> <20131202132617.GA28132@stefanha-thinkpad.redhat.com> In-Reply-To: <20131202132617.GA28132@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] vmdk: Fix creating big description file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org On 2013=E5=B9=B412=E6=9C=8802=E6=97=A5 21:26, Stefan Hajnoczi wrote: > On Mon, Dec 02, 2013 at 11:01:20AM +0800, Fam Zheng wrote: >> The buffer for description file was 4096 which only covers a few >> hundred of extents. This changes the buffer to dynamic allocated with >> g_strdup_printf in order to support bigger cases. >> >> Signed-off-by: Fam Zheng >> --- >> block/vmdk.c | 65 +- >> tests/qemu-iotests/059 | 5 + >> tests/qemu-iotests/059.out | 2012 ++++++++++++++++++++++++++++++++++= ++++++++++ >> 3 files changed, 2058 insertions(+), 24 deletions(-) > > Does VMware have a hard limit? For example, does it open the 1000 GB > twoGbMaxExtentFlat file from your test case? > Yes it does. The limit is far above this, there is a statement in VMDK=20 spec: "Maximum VMDK file size is 2TB." It opens this file from the test=20 case, however I can't generate such a twoGbMaxExtentFlat from=20 Workstation, because it will be automatically forced to monolithicFlat=20 (if the size is above certain threshold). > Minor comments below but feel free to keep the code as-is if you wish: > >> @@ -1625,7 +1625,8 @@ static int vmdk_create(const char *filename, QEM= UOptionParameter *options, >> "ddb.adapterType =3D \"%s\"\n"; >> >> if (filename_decompose(filename, path, prefix, postfix, PATH_MAX= , errp)) { >> - return -EINVAL; >> + ret =3D -EINVAL; >> + goto exit; >> } >> /* Read out options */ >> while (options && options->name) { >> @@ -1651,7 +1652,8 @@ static int vmdk_create(const char *filename, QEM= UOptionParameter *options, >> strcmp(adapter_type, "lsilogic") && >> strcmp(adapter_type, "legacyESX")) { >> error_setg(errp, "Unknown adapter type: '%s'", adapter_type)= ; >> - return -EINVAL; >> + ret =3D -EINVAL; >> + goto exit; >> } >> if (strcmp(adapter_type, "ide") !=3D 0) { >> /* that's the number of heads with which vmware operates whe= n >> @@ -1667,7 +1669,8 @@ static int vmdk_create(const char *filename, QEM= UOptionParameter *options, >> strcmp(fmt, "twoGbMaxExtentFlat") && >> strcmp(fmt, "streamOptimized")) { >> error_setg(errp, "Unknown subformat: '%s'", fmt); >> - return -EINVAL; >> + ret =3D -EINVAL; >> + goto exit; >> } >> split =3D !(strcmp(fmt, "twoGbMaxExtentFlat") && >> strcmp(fmt, "twoGbMaxExtentSparse")); >> @@ -1681,22 +1684,25 @@ static int vmdk_create(const char *filename, Q= EMUOptionParameter *options, >> } >> if (flat && backing_file) { >> error_setg(errp, "Flat image can't have backing file"); >> - return -ENOTSUP; >> + ret =3D -ENOTSUP; >> + goto exit; >> } >> if (flat && zeroed_grain) { >> error_setg(errp, "Flat image can't enable zeroed grain"); >> - return -ENOTSUP; >> + ret =3D -ENOTSUP; >> + goto exit; >> } >> if (backing_file) { >> BlockDriverState *bs =3D bdrv_new(""); >> ret =3D bdrv_open(bs, backing_file, NULL, 0, NULL, errp); >> if (ret !=3D 0) { >> bdrv_unref(bs); >> - return ret; >> + goto exit; >> } >> if (strcmp(bs->drv->format_name, "vmdk")) { >> bdrv_unref(bs); >> - return -EINVAL; >> + ret =3D -EINVAL; >> + goto exit; >> } >> parent_cid =3D vmdk_read_cid(bs, 0); >> bdrv_unref(bs); > > Changing these to goto isn't really necessary. > Just that I'd like to keep it consistent that exit point is either=20 direct return statement or goto jump, in one function, so it will be=20 easy to reason the code path. >> @@ -1730,25 +1737,31 @@ static int vmdk_create(const char *filename, Q= EMUOptionParameter *options, >> >> if (vmdk_create_extent(ext_filename, size, >> flat, compress, zeroed_grain)) { >> - return -EINVAL; >> + ret =3D -EINVAL; >> + goto exit; >> } >> filesize -=3D size; >> >> /* Format description line */ >> snprintf(desc_line, sizeof(desc_line), >> desc_extent_line, size / 512, desc_filename); >> - pstrcat(ext_desc_lines, sizeof(ext_desc_lines), desc_line); >> + new_desc_lines =3D g_strdup_printf("%s%s", >> + ext_desc_lines ? : "", >> + desc_line); >> + g_free(ext_desc_lines); >> + ext_desc_lines =3D new_desc_lines; > > These lines can be eliminated if you use GString: > > /* Format description line */ > g_string_append_printf(ext_desc_lines, > desc_extent_line, > size / 512, > desc_filename); > OK, good idea. Thanks Fam