From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] vmdk: Use bdrv_dirname() for relative extent paths
Date: Mon, 12 Aug 2019 16:17:24 -0400 [thread overview]
Message-ID: <7d00fbea-5762-9e0f-199f-c3886727d937@redhat.com> (raw)
In-Reply-To: <20190725155735.11872-3-mreitz@redhat.com>
On 7/25/19 11:57 AM, Max Reitz wrote:
> This makes iotest 033 pass with e.g. subformat=monolithicFlat. It also
> turns a former error in 059 into success.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Seems roughly correct, but I only really gave it a cursory look; my
trust in you knowing the exact semantics of filename and path variables
because of those lengthy series is doing the heavy lifting here:
Reviewed-by: John Snow <jsnow@redhat.com>
(And if it breaks, it's for 4.2, and it's just vmdk, we'll figure it out.)
> ---
> block/vmdk.c | 54 ++++++++++++++++++++++++--------------
> tests/qemu-iotests/059 | 7 +++--
> tests/qemu-iotests/059.out | 4 ++-
> 3 files changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index bd36ece125..db6acfc31e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1076,8 +1076,7 @@ static const char *next_line(const char *s)
> }
>
> static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> - const char *desc_file_path, QDict *options,
> - Error **errp)
> + QDict *options, Error **errp)
> {
> int ret;
> int matches;
> @@ -1087,6 +1086,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> const char *p, *np;
> int64_t sectors = 0;
> int64_t flat_offset;
> + char *desc_file_dir = NULL;
> char *extent_path;
> BdrvChild *extent_file;
> BDRVVmdkState *s = bs->opaque;
> @@ -1130,16 +1130,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> continue;
> }
>
> - if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
> - !desc_file_path[0])
> - {
> - bdrv_refresh_filename(bs->file->bs);
> - error_setg(errp, "Cannot use relative extent paths with VMDK "
> - "descriptor file '%s'", bs->file->bs->filename);
> - return -EINVAL;
> - }
> + if (path_is_absolute(fname) || path_has_protocol(fname)) {
> + extent_path = g_strdup(fname);
> + } else {
> + if (!desc_file_dir) {
> + desc_file_dir = bdrv_dirname(bs->file->bs, errp);
> + if (!desc_file_dir) {
> + bdrv_refresh_filename(bs->file->bs);
> + error_prepend(errp, "Cannot use relative paths with VMDK "
> + "descriptor file '%s': ",
> + bs->file->bs->filename);
> + ret = -EINVAL;
> + goto out;
> + }
> + }
>
> - extent_path = path_combine(desc_file_path, fname);
> + extent_path = g_strconcat(desc_file_dir, fname, NULL);
> + }
>
> ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
> assert(ret < 32);
> @@ -1149,7 +1156,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> g_free(extent_path);
> if (local_err) {
> error_propagate(errp, local_err);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> /* save to extents array */
> @@ -1160,7 +1168,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> 0, 0, 0, 0, 0, &extent, errp);
> if (ret < 0) {
> bdrv_unref_child(bs, extent_file);
> - return ret;
> + goto out;
> }
> extent->flat_start_offset = flat_offset << 9;
> } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
> @@ -1175,24 +1183,27 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> g_free(buf);
> if (ret) {
> bdrv_unref_child(bs, extent_file);
> - return ret;
> + goto out;
> }
> extent = &s->extents[s->num_extents - 1];
> } else if (!strcmp(type, "SESPARSE")) {
> ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
> if (ret) {
> bdrv_unref_child(bs, extent_file);
> - return ret;
> + goto out;
> }
> extent = &s->extents[s->num_extents - 1];
> } else {
> error_setg(errp, "Unsupported extent type '%s'", type);
> bdrv_unref_child(bs, extent_file);
> - return -ENOTSUP;
> + ret = -ENOTSUP;
> + goto out;
> }
> extent->type = g_strdup(type);
> }
> - return 0;
> +
> + ret = 0;
> + goto out;
>
> invalid:
> np = next_line(p);
> @@ -1201,7 +1212,11 @@ invalid:
> np--;
> }
> error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
> - return -EINVAL;
> + ret = -EINVAL;
> +
> +out:
> + g_free(desc_file_dir);
> + return ret;
> }
>
> static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> @@ -1228,8 +1243,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->bs->exact_filename, options,
> - errp);
> + ret = vmdk_parse_extents(buf, bs, options, errp);
> exit:
> return ret;
> }
> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> index 279aee6815..fbed5f9483 100755
> --- a/tests/qemu-iotests/059
> +++ b/tests/qemu-iotests/059
> @@ -114,9 +114,12 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o subformat=streamOptimized "$TEST_IMG.qcow2
>
> echo
> echo "=== Testing monolithicFlat with internally generated JSON file name ==="
> +# Should work, because bdrv_dirname() works fine with blkdebug
> IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
> -$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" 2>&1 \
> - | _filter_testdir | _filter_imgfmt
> +$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio" \
> + -c info \
> + 2>&1 \
> + | _filter_testdir | _filter_imgfmt | _filter_img_info
> _cleanup_test_img
>
> echo
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 77d8984428..120cddd207 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2050,7 +2050,9 @@ wrote 512/512 bytes at offset 10240
>
> === Testing monolithicFlat with internally generated JSON file name ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> -qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
> +format name: IMGFMT
> +cluster size: 0 bytes
> +vm state offset: 0 bytes
>
> === Testing version 3 ===
> image: TEST_DIR/iotest-version3.IMGFMT
>
next prev parent reply other threads:[~2019-08-12 20:18 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-25 15:57 [Qemu-devel] [PATCH 0/7] vmdk: Misc fixes Max Reitz
2019-07-25 15:57 ` [Qemu-devel] [PATCH 1/7] iotests: Fix _filter_img_create() Max Reitz
2019-08-12 19:32 ` [Qemu-devel] [Qemu-block] " John Snow
2019-07-25 15:57 ` [Qemu-devel] [PATCH 2/7] vmdk: Use bdrv_dirname() for relative extent paths Max Reitz
2019-08-12 20:17 ` John Snow [this message]
2019-07-25 15:57 ` [Qemu-devel] [PATCH 3/7] iotests: Keep testing broken " Max Reitz
2019-08-12 20:21 ` [Qemu-devel] [Qemu-block] " John Snow
2019-07-25 15:57 ` [Qemu-devel] [PATCH 4/7] vmdk: Reject invalid compressed writes Max Reitz
2019-08-12 20:26 ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-12 21:03 ` Max Reitz
2019-08-12 21:16 ` John Snow
2019-08-13 12:58 ` Max Reitz
2019-07-25 15:57 ` [Qemu-devel] [PATCH 5/7] iotests: Disable broken streamOptimized tests Max Reitz
2019-07-25 15:57 ` [Qemu-devel] [PATCH 6/7] iotests: Disable 110 for vmdk.twoGbMaxExtentSparse Max Reitz
2019-08-12 21:26 ` [Qemu-devel] [Qemu-block] " John Snow
2019-07-25 15:57 ` [Qemu-devel] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats Max Reitz
2019-07-25 17:00 ` Eric Blake
2019-07-26 7:52 ` Max Reitz
2019-08-12 21:33 ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-13 14:00 ` Max Reitz
2019-08-13 22:26 ` John Snow
2019-08-14 14:01 ` Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7d00fbea-5762-9e0f-199f-c3886727d937@redhat.com \
--to=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).