diff mbox series

[2/7] vmdk: Use bdrv_dirname() for relative extent paths

Message ID 20190725155735.11872-3-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series vmdk: Misc fixes | expand

Commit Message

Max Reitz July 25, 2019, 3:57 p.m. UTC 
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>
---
 block/vmdk.c               | 54 ++++++++++++++++++++++++--------------
 tests/qemu-iotests/059     |  7 +++--
 tests/qemu-iotests/059.out |  4 ++-
 3 files changed, 42 insertions(+), 23 deletions(-)

Comments

John Snow Aug. 12, 2019, 8:17 p.m. UTC | #1 
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
>
diff mbox series

Patch

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