From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47692) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwKuJ-0004pB-Fo for qemu-devel@nongnu.org; Wed, 03 Dec 2014 20:08:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XwKuD-0007cK-9y for qemu-devel@nongnu.org; Wed, 03 Dec 2014 20:08:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwKuD-0007cE-1z for qemu-devel@nongnu.org; Wed, 03 Dec 2014 20:08:29 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB418SCj010102 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 3 Dec 2014 20:08:28 -0500 Date: Thu, 4 Dec 2014 09:08:25 +0800 From: Fam Zheng Message-ID: <20141204010825.GA21315@ad.nay.redhat.com> References: <1417615043-26174-1-git-send-email-mreitz@redhat.com> <1417615043-26174-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1417615043-26174-2-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/2] vmdk: Fix error for JSON descriptor file names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On Wed, 12/03 14:57, Max Reitz wrote: > If vmdk blindly tries to use path_combine() using bs->file->filename as > the base file name, this will result in a bad error message for JSON > file names when calling bdrv_open(). It is better to only try > bs->file->exact_filename; if that is empty, bs->file->filename will be > useless for path_combine() and an error should be emitted (containing > bs->file->filename because desc_file_path (which is > bs->file->exact_filename) is empty). > > Signed-off-by: Max Reitz > --- > block.c | 2 +- > block/vmdk.c | 10 +++++++++- > include/block/block.h | 1 + > 3 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 591fbe4..239ae5f 100644 > --- a/block.c > +++ b/block.c > @@ -229,7 +229,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs) > } > > /* check if the path starts with ":" */ > -static int path_has_protocol(const char *path) > +int path_has_protocol(const char *path) > { > const char *p; > > diff --git a/block/vmdk.c b/block/vmdk.c > index 2cbfd3e..65413a1 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -818,6 +818,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, > goto next_line; > } > > + if (!path_is_absolute(fname) && !path_has_protocol(fname) && If I read the VMDK spec correctly, fname can't have protocol (as a format spec it hasn't a single word on protocol :). But it's not wrong to check. I continue reading thinking it as "true". fname, on the other hand, is in most cases a relative path. In this case, this condition falls to desc_file_path, which is bs->file->exact_filename. This will be non-empty for local files, so the path_combine below will work. If fname is absolute, path_combine will also work. So it is correct. > + !desc_file_path[0]) > + { > + error_setg(errp, "Cannot use relative extent paths with VMDK " > + "descriptor file '%s'", bs->file->filename); > + return -EINVAL; > + } > + > path_combine(extent_path, sizeof(extent_path), > desc_file_path, fname); > extent_file = NULL; > @@ -894,7 +902,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf, > } > s->create_type = g_strdup(ct); > s->desc_offset = 0; > - ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp); > + ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp); > exit: > return ret; > } > diff --git a/include/block/block.h b/include/block/block.h > index 610be9f..919c8f5 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -401,6 +401,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, > char *dest, size_t sz); > int bdrv_is_snapshot(BlockDriverState *bs); > > +int path_has_protocol(const char *path); Exporting path_has_protocol could be a separate patch but I don't have a strong objection as this patch is still short. Reviewed-by: Fam Zheng > int path_is_absolute(const char *path); > void path_combine(char *dest, int dest_size, > const char *base_path, > -- > 1.9.3 >