[Qemu-devel] [PATCH v4 0/6] block: Drop BDS.filename

[Qemu-devel] [PATCH v4 0/6] block: Drop BDS.filename

[Max Reitz]

The BDS filename field is generally only used when opening disk images
or emitting error or warning messages, the only exception to this rule
is the map command of qemu-img. However, using exact_filename there
instead should not be a problem. Therefore, we can drop the filename
field from the BlockDriverState and use a function instead which builds
the filename from scratch when called.

This is slower than reading a static char array but the problem of that
static array is that it may become obsolete due to changes in any
BlockDriverState or in the BDS graph. Using a function which rebuilds
the filename every time it is called resolves this problem.

The disadvantage of worse performance is negligible, on the other hand.
After patch 2 of this series, which replaces some queries of
BDS.filename by reads from somewhere else (mostly BDS.exact_filename),
the filename field is only used when a disk image is opened or some
message should be emitted, both of which cases do not suffer from the
performance hit.


http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg07025.html
gives an example of how to achieve an outdated filename, so we do need
this series. We can technically fix it differently (by appropriately
invoking bdrv_refresh_filename()), but that is pretty difficult™. I find
this series simpler and it it's something we wanted to do anyway.

Regarding the fear that this might change current behavior, especially
for libvirt which used filenames to identify nodes in the BDS graph:
Since bdrv_open() already calls bdrv_refresh_filename() today, and
apparently everything works fine, this series will most likely not break
anything. The only thing that will change is if the BDS graph is
dynamically reconfigured at runtime, and in that case it's most likely a
bug fix. Also, I don't think libvirt supports such cases already.


tl;dr: This series is a bug fix and I don't think it'll break legacy
management applications relying on the filename to identify a BDS; as
long as they are not trying to continue that practice with more advanced
configurations (e.g. with Quorum).


v4:
- Patch 2:
  - Do not touch qemu-img map [Kevin]
  - s/bs->filename/filename/ in raw_open_common() [Kevin]
  - s/bs->filename/bs->exact_filename/ in hdev_is_sg() [Kevin]
- Patch 4: Added [Kevin]
- Patch 5:
  - Do not touch raw_open_common() (now in patch 2)
  - Do not touch hdev_is_sg() (now in patch 2)


git-backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[----] [--] 'block: Change bdrv_get_encrypted_filename()'
002/6:[0006] [FC] 'block: Avoid BlockDriverState.filename'
003/6:[----] [--] 'block: Add bdrv_filename()'
004/6:[down] 'qemu-img: Use bdrv_filename_alloc() for map'
005/6:[0012] [FC] 'block: Drop BlockDriverState.filename'
006/6:[----] [--] 'iotests: Test changed Quorum filename'


Max Reitz (6):
  block: Change bdrv_get_encrypted_filename()
  block: Avoid BlockDriverState.filename
  block: Add bdrv_filename()
  qemu-img: Use bdrv_filename_alloc() for map
  block: Drop BlockDriverState.filename
  iotests: Test changed Quorum filename

 block.c                   | 108 +++++++++++++++++++++++++++++++++-------------
 block/commit.c            |   4 +-
 block/gluster.c           |   2 +-
 block/mirror.c            |  16 +++++--
 block/qapi.c              |   4 +-
 block/raw-posix.c         |  12 +++---
 block/raw-win32.c         |   4 +-
 block/raw_bsd.c           |   4 +-
 block/vhdx-log.c          |   5 ++-
 block/vmdk.c              |  22 +++++++---
 block/vpc.c               |   7 ++-
 blockdev.c                |  22 +++++++---
 include/block/block.h     |   4 +-
 include/block/block_int.h |   1 -
 monitor.c                 |   6 ++-
 qemu-img.c                |  14 +++++-
 tests/qemu-iotests/041    |  17 ++++++--
 17 files changed, 184 insertions(+), 68 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename()

[Max Reitz]

Instead of returning a pointer to the filename, copy it into a buffer
specified by the caller.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 25 ++++++++++++++++++-------
 include/block/block.h |  2 +-
 monitor.c             |  6 +++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index d088ee0..41b0f85 100644
--- a/block.c
+++ b/block.c
@@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
         }
     } else {
         if (bdrv_key_required(bs)) {
+            char *enc_filename = g_malloc(PATH_MAX);
+            bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
             error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
                       "'%s' (%s) is encrypted",
                       bdrv_get_device_or_node_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+                      enc_filename);
+            g_free(enc_filename);
         }
     }
 }
@@ -2980,14 +2983,22 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
     return false;
 }
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
+char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
 {
-    if (bs->backing_hd && bs->backing_hd->encrypted)
-        return bs->backing_file;
-    else if (bs->encrypted)
-        return bs->filename;
-    else
+    if (sz > INT_MAX) {
+        sz = INT_MAX;
+    }
+
+    if (bs->backing_hd && bs->backing_hd->encrypted) {
+        pstrcpy(dest, sz, bs->backing_file);
+        return dest;
+    } else if (bs->encrypted) {
+        pstrcpy(dest, sz, bs->filename);
+        return dest;
+    } else {
+        dest[0] = '\0';
         return NULL;
+    }
 }
 
 void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index 37916f7..a78e4f1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -430,7 +430,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t *cluster_sector_num,
                             int *cluster_nb_sectors);
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
+char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
diff --git a/monitor.c b/monitor.c
index aeea2b5..a7d1650 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockCompletionFunc *completion_cb,
                                 void *opaque)
 {
+    char *enc_filename;
     int err;
 
+    enc_filename = g_malloc(PATH_MAX);
+    bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-                   bdrv_get_encrypted_filename(bs));
+                   enc_filename);
+    g_free(enc_filename);
 
     mon->password_completion_cb = completion_cb;
     mon->password_opaque = opaque;
-- 
2.4.3

[Qemu-devel] [PATCH v4 2/6] block: Avoid BlockDriverState.filename

[Max Reitz]

In places which directly pass a filename to the OS, we should not use
the filename field at all but exact_filename instead (although the
former currently equals the latter if that is set).

In raw_open_common(), we do not need to access BDS.filename because we
already have a local variable pointing to the filename.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c           |  5 +++--
 block/gluster.c   |  2 +-
 block/raw-posix.c | 12 ++++++------
 block/raw-win32.c |  4 ++--
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 41b0f85..1572785 100644
--- a/block.c
+++ b/block.c
@@ -918,8 +918,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (ret < 0) {
         if (local_err) {
             error_propagate(errp, local_err);
-        } else if (bs->filename[0]) {
-            error_setg_errno(errp, -ret, "Could not open '%s'", bs->filename);
+        } else if (bs->exact_filename[0]) {
+            error_setg_errno(errp, -ret, "Could not open '%s'",
+                             bs->exact_filename);
         } else {
             error_setg_errno(errp, -ret, "Could not open image");
         }
diff --git a/block/gluster.c b/block/gluster.c
index 1eb3a8c..176682b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -358,7 +358,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
 
     gconf = g_new0(GlusterConf, 1);
 
-    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
+    reop_s->glfs = qemu_gluster_init(gconf, state->bs->exact_filename, errp);
     if (reop_s->glfs == NULL) {
         ret = -errno;
         goto exit;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 30df8ad..976dcd7 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -517,7 +517,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
                      "specified. Falling back to aio=threads.\n"
                      "         This will become an error condition in "
                      "future QEMU versions.\n",
-                     bs->filename);
+                     filename);
     }
 #endif
 
@@ -670,7 +670,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
     if (raw_s->fd == -1) {
-        const char *normalized_filename = state->bs->filename;
+        const char *normalized_filename = state->bs->exact_filename;
         ret = raw_normalize_devicepath(&normalized_filename);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not normalize device path");
@@ -2102,7 +2102,7 @@ static bool hdev_is_sg(BlockDriverState *bs)
     struct sg_scsi_id scsiid;
     int sg_version;
 
-    if (stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode) &&
+    if (stat(bs->exact_filename, &st) >= 0 && S_ISCHR(st.st_mode) &&
         !bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
         !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid)) {
         DPRINTF("SG device found: type=%d, version=%d\n",
@@ -2201,7 +2201,7 @@ static int fd_open(BlockDriverState *bs)
             DPRINTF("No floppy (open delayed)\n");
             return -EIO;
         }
-        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = qemu_open(bs->exact_filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
             s->fd_error_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
             s->fd_got_error = 1;
@@ -2498,7 +2498,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag)
         qemu_close(s->fd);
         s->fd = -1;
     }
-    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
+    fd = qemu_open(bs->exact_filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -2722,7 +2722,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         qemu_close(s->fd);
-    fd = qemu_open(bs->filename, s->open_flags, 0644);
+    fd = qemu_open(bs->exact_filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..5c8a894 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -417,7 +417,7 @@ static void raw_close(BlockDriverState *bs)
 
     CloseHandle(s->hfile);
     if (bs->open_flags & BDRV_O_TEMPORARY) {
-        unlink(bs->filename);
+        unlink(bs->exact_filename);
     }
 }
 
@@ -485,7 +485,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
                                               DWORD * high);
     get_compressed_t get_compressed;
     struct _stati64 st;
-    const char *filename = bs->filename;
+    const char *filename = bs->exact_filename;
     /* WinNT support GetCompressedFileSize to determine allocate size */
     get_compressed =
         (get_compressed_t) GetProcAddress(GetModuleHandle("kernel32"),
-- 
2.4.3

[Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename()

[Max Reitz]

Split the part which actually refreshes the BlockDriverState.filename
field off of bdrv_refresh_filename() into a more generic function
bdrv_filename(), which first calls bdrv_refresh_filename() and then
stores a qemu-usable filename into the given buffer instead of
BlockDriverState.filename.

Since bdrv_refresh_filename() therefore no longer refreshes that field,
some calls to that function have to be replaced by calls to
bdrv_filename() "manually" refreshing the BDS filename field (this is
only temporary).

Additionally, a wrapper function bdrv_filename_alloc() is added which
allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer
and returns it, since needing a temporary buffer for the filename is a
rather common pattern.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 39 ++++++++++++++++++++++++++++++++-------
 block/blkverify.c     |  3 ++-
 block/quorum.c        |  2 +-
 include/block/block.h |  2 ++
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 1572785..d2a3e8d 100644
--- a/block.c
+++ b/block.c
@@ -1559,7 +1559,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    bdrv_refresh_filename(bs);
+    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
 
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
@@ -4153,9 +4153,6 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
  *  - full_open_options: Options which, when given when opening a block device
  *                       (without a filename), result in a BDS (mostly)
  *                       equalling the given one
- *  - filename: If exact_filename is set, it is copied here. Otherwise,
- *              full_open_options is converted to a JSON object, prefixed with
- *              "json:" (for use through the JSON pseudo protocol) and put here.
  */
 void bdrv_refresh_filename(BlockDriverState *bs)
 {
@@ -4242,15 +4239,43 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 
         bs->full_open_options = opts;
     }
+}
+
+/* First refreshes exact_filename and full_open_options by calling
+ * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
+ * the target buffer. Otherwise, full_open_options is converted to a JSON
+ * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
+ * put there.
+ *
+ * If sz > 0, the string put into the buffer will always be null-terminated.
+ *
+ * Returns @dest.
+ */
+char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)
+{
+    bdrv_refresh_filename(bs);
+
+    if (sz > INT_MAX) {
+        sz = INT_MAX;
+    }
 
     if (bs->exact_filename[0]) {
-        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
+        pstrcpy(dest, sz, bs->exact_filename);
     } else if (bs->full_open_options) {
         QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
-        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
-                 qstring_get_str(json));
+        snprintf(dest, sz, "json:%s", qstring_get_str(json));
         QDECREF(json);
     }
+
+    return dest;
+}
+
+/* Wrapper around bdrv_filename() using g_malloc(PATH_MAX) for the destination
+ * buffer.
+ */
+char *bdrv_filename_alloc(BlockDriverState *bs)
+{
+    return bdrv_filename(bs, g_malloc(PATH_MAX), PATH_MAX);
 }
 
 /* This accessor function purpose is to allow the device models to access the
diff --git a/block/blkverify.c b/block/blkverify.c
index d277e63..cb9ce02 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -309,7 +309,8 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file has already been refreshed */
-    bdrv_refresh_filename(s->test_file);
+    bdrv_filename(s->test_file, s->test_file->filename,
+                  sizeof(s->test_file->filename));
 
     if (bs->file->full_open_options && s->test_file->full_open_options) {
         QDict *opts = qdict_new();
diff --git a/block/quorum.c b/block/quorum.c
index 2f6c45f..00d1fb0 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1003,7 +1003,7 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_refresh_filename(s->bs[i]);
+        bdrv_filename(s->bs[i], s->bs[i]->filename, sizeof(s->bs[i]->filename));
         if (!s->bs[i]->full_open_options) {
             return;
         }
diff --git a/include/block/block.h b/include/block/block.h
index a78e4f1..4be6c18 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -267,6 +267,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
+char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz);
+char *bdrv_filename_alloc(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
-- 
2.4.3

[Qemu-devel] [PATCH v4 4/6] qemu-img: Use bdrv_filename_alloc() for map

[Max Reitz]

Replaces bs->filename by the result of bdrv_filename_alloc() in the
qemu-img map subcommand. Since that value is queried relatively often,
however, it should be cached.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 75f4ee4..b2ea059 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2141,6 +2141,7 @@ typedef struct MapEntry {
     int64_t length;
     int64_t offset;
     BlockDriverState *bs;
+    char *bs_filename;
 } MapEntry;
 
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
@@ -2155,7 +2156,7 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
             printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
-                   e->start, e->length, e->offset, e->bs->filename);
+                   e->start, e->length, e->offset, e->bs_filename);
         }
         /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
          * Modify the flags here to allow more coalescing.
@@ -2305,6 +2306,12 @@ static int img_map(int argc, char **argv)
             goto out;
         }
 
+        if (next.bs == curr.bs) {
+            next.bs_filename = curr.bs_filename;
+        } else {
+            next.bs_filename = bdrv_filename_alloc(next.bs);
+        }
+
         if (curr.length != 0 && curr.flags == next.flags &&
             curr.depth == next.depth &&
             ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
@@ -2316,10 +2323,15 @@ static int img_map(int argc, char **argv)
         if (curr.length > 0) {
             dump_map_entry(output_format, &curr, &next);
         }
+
+        if (curr.bs != next.bs) {
+            g_free(curr.bs_filename);
+        }
         curr = next;
     }
 
     dump_map_entry(output_format, &curr, NULL);
+    g_free(curr.bs_filename);
 
 out:
     blk_unref(blk);
-- 
2.4.3

[Qemu-devel] [PATCH v4 5/6] block: Drop BlockDriverState.filename

[Max Reitz]

That field is now only used during initialization of BlockDriverStates
(opening images) and for error or warning messages. Performance is not
that much of an issue here, so we can drop the field and replace its use
by a call to bdrv_filename() or bdrv_filename_alloc(). By doing so we
can ensure the result always to be recent, whereas the contents of
BlockDriverState.filename may have been obsoleted by manipulations of
single BlockDriverStates or of the BDS graph.

The users of the BDS filename field were changed as follows:
- copying the filename into another buffer is trivially replaced by
  using bdrv_filename() instead of the copy function
- strdup() on the filename is replaced by a call to
  bdrv_filename_alloc()
- bdrv_filename(bs, bs->filename, sizeof(bs->filename)) is replaced by
  bdrv_refresh_filename(bs)
  (these were introduced by the previous patch)
- anywhere else bdrv_filename_alloc() is used, any access to
  BlockDriverState.filename is then replaced by the buffer returned, and
  it is freed when it is no longer needed

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 47 ++++++++++++++++++++++++++++++-----------------
 block/blkverify.c         |  3 +--
 block/commit.c            |  4 +++-
 block/mirror.c            | 16 ++++++++++++----
 block/qapi.c              |  4 ++--
 block/quorum.c            |  2 +-
 block/raw_bsd.c           |  4 +++-
 block/vhdx-log.c          |  5 ++++-
 block/vmdk.c              | 22 ++++++++++++++++------
 block/vpc.c               |  7 +++++--
 blockdev.c                | 22 +++++++++++++++++-----
 include/block/block_int.h |  1 -
 12 files changed, 94 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index d2a3e8d..c53d1d6 100644
--- a/block.c
+++ b/block.c
@@ -227,10 +227,10 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,
 void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
                                     Error **errp)
 {
-    char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-
+    char *backed = bdrv_filename_alloc(bs);
     bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
                                                  dest, sz, errp);
+    g_free(backed);
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -821,6 +821,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     QDict *options, int flags, BlockDriver *drv, Error **errp)
 {
     int ret, open_flags;
+    char *filename_buffer = NULL;
     const char *filename;
     const char *node_name = NULL;
     QemuOpts *opts;
@@ -831,7 +832,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     assert(options != NULL && bs->options != options);
 
     if (file != NULL) {
-        filename = file->filename;
+        filename_buffer = bdrv_filename_alloc(file);
+        filename = filename_buffer;
     } else {
         filename = qdict_get_try_str(options, "filename");
     }
@@ -839,7 +841,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (drv->bdrv_needs_filename && !filename) {
         error_setg(errp, "The '%s' block driver requires a file name",
                    drv->format_name);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail_filename;
     }
 
     trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
@@ -888,11 +891,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     }
 
     if (filename != NULL) {
-        pstrcpy(bs->filename, sizeof(bs->filename), filename);
+        pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), filename);
     } else {
-        bs->filename[0] = '\0';
+        bs->exact_filename[0] = '\0';
     }
-    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
 
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
@@ -952,6 +954,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     assert((bs->request_alignment != 0) || bdrv_is_sg(bs));
 
     qemu_opts_del(opts);
+    g_free(filename_buffer);
     return 0;
 
 free_and_fail:
@@ -961,6 +964,8 @@ free_and_fail:
     bs->drv = NULL;
 fail_opts:
     qemu_opts_del(opts);
+fail_filename:
+    g_free(filename_buffer);
     return ret;
 }
 
@@ -1158,7 +1163,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     }
     bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing);
     bs->open_flags &= ~BDRV_O_NO_BACKING;
-    pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
+    bdrv_filename(backing_hd, bs->backing_file, sizeof(bs->backing_file));
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
             backing_hd->drv ? backing_hd->drv->format_name : "");
 
@@ -1559,7 +1564,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
+    bdrv_refresh_filename(bs);
 
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
@@ -1823,8 +1828,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             if (local_err != NULL) {
                 error_propagate(errp, local_err);
             } else {
+                char *filename = bdrv_filename_alloc(reopen_state->bs);
                 error_setg(errp, "failed while preparing to reopen image '%s'",
-                           reopen_state->bs->filename);
+                           filename);
+                g_free(filename);
             }
             goto error;
         }
@@ -2441,6 +2448,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     BlockDriverState *base_bs = NULL;
     BlockDriverState *new_top_bs = NULL;
     BlkIntermediateStates *intermediate_state, *next;
+    char *base_filename = NULL;
     int ret = -EIO;
 
     QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
@@ -2487,7 +2495,10 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    backing_file_str = backing_file_str ? backing_file_str : base_bs->filename;
+    if (!backing_file_str) {
+        base_filename = bdrv_filename_alloc(base_bs);
+        backing_file_str = base_filename;
+    }
     ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
                                    base_bs->drv ? base_bs->drv->format_name : "");
     if (ret) {
@@ -2506,6 +2517,7 @@ exit:
     QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
         g_free(intermediate_state);
     }
+    g_free(base_filename);
     return ret;
 }
 
@@ -2994,8 +3006,7 @@ char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
         pstrcpy(dest, sz, bs->backing_file);
         return dest;
     } else if (bs->encrypted) {
-        pstrcpy(dest, sz, bs->filename);
-        return dest;
+        return bdrv_filename(bs, dest, sz);
     } else {
         dest[0] = '\0';
         return NULL;
@@ -3096,7 +3107,7 @@ int bdrv_is_snapshot(BlockDriverState *bs)
 }
 
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
- * relative, it must be relative to the chain.  So, passing in bs->filename
+ * relative, it must be relative to the chain.  So, passing in the filename
  * from a BDS as backing_file should not be done, as that may be relative to
  * the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
@@ -3131,18 +3142,20 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         } else {
             /* If not an absolute filename path, make it relative to the current
              * image's filename path */
-            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
-                         backing_file);
+            char *curr_filename = bdrv_filename_alloc(curr_bs);
+            path_combine(filename_tmp, PATH_MAX, curr_filename, backing_file);
 
             /* We are going to compare absolute pathnames */
             if (!realpath(filename_tmp, filename_full)) {
+                g_free(curr_filename);
                 continue;
             }
 
             /* We need to make sure the backing filename we are comparing against
              * is relative to the current image filename (or absolute) */
-            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
+            path_combine(filename_tmp, PATH_MAX, curr_filename,
                          curr_bs->backing_file);
+            g_free(curr_filename);
 
             if (!realpath(filename_tmp, backing_file_full)) {
                 continue;
diff --git a/block/blkverify.c b/block/blkverify.c
index cb9ce02..d277e63 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -309,8 +309,7 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file has already been refreshed */
-    bdrv_filename(s->test_file, s->test_file->filename,
-                  sizeof(s->test_file->filename));
+    bdrv_refresh_filename(s->test_file);
 
     if (bs->file->full_open_options && s->test_file->full_open_options) {
         QDict *opts = qdict_new();
diff --git a/block/commit.c b/block/commit.c
index 7312a5b..524cb2e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -227,7 +227,9 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     overlay_bs = bdrv_find_overlay(bs, top);
 
     if (overlay_bs == NULL) {
-        error_setg(errp, "Could not find overlay image for %s:", top->filename);
+        char *top_filename = bdrv_filename_alloc(top);
+        error_setg(errp, "Could not find overlay image for %s:", top_filename);
+        g_free(top_filename);
         return;
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index b2fb4b9..5e46650 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -786,25 +786,33 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
 
     length = bdrv_getlength(bs);
     if (length < 0) {
-        error_setg_errno(errp, -length,
-                         "Unable to determine length of %s", bs->filename);
+        char *filename = bdrv_filename_alloc(bs);
+        error_setg_errno(errp, -length, "Unable to determine length of %s",
+                         filename);
+        g_free(filename);
         goto error_restore_flags;
     }
 
     base_length = bdrv_getlength(base);
     if (base_length < 0) {
+        char *base_filename = bdrv_filename_alloc(base);
         error_setg_errno(errp, -base_length,
-                         "Unable to determine length of %s", base->filename);
+                         "Unable to determine length of %s", base_filename);
+        g_free(base_filename);
         goto error_restore_flags;
     }
 
     if (length > base_length) {
         ret = bdrv_truncate(base, length);
         if (ret < 0) {
+            char *filename = bdrv_filename_alloc(bs);
+            char *base_filename = bdrv_filename_alloc(base);
             error_setg_errno(errp, -ret,
                             "Top image %s is larger than base image %s, and "
                              "resize of base image failed",
-                             bs->filename, base->filename);
+                             filename, base_filename);
+            g_free(filename);
+            g_free(base_filename);
             goto error_restore_flags;
         }
     }
diff --git a/block/qapi.c b/block/qapi.c
index 2ce5097..1078d7e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -38,7 +38,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
     BlockDriverState *bs0;
     BlockDeviceInfo *info = g_malloc0(sizeof(*info));
 
-    info->file                   = g_strdup(bs->filename);
+    info->file                   = bdrv_filename_alloc(bs);
     info->ro                     = bs->read_only;
     info->drv                    = g_strdup(bs->drv->format_name);
     info->encrypted              = bs->encrypted;
@@ -218,7 +218,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
     }
 
     info = g_new0(ImageInfo, 1);
-    info->filename        = g_strdup(bs->filename);
+    info->filename        = bdrv_filename_alloc(bs);
     info->format          = g_strdup(bdrv_get_format_name(bs));
     info->virtual_size    = size;
     info->actual_size     = bdrv_get_allocated_file_size(bs);
diff --git a/block/quorum.c b/block/quorum.c
index 00d1fb0..2f6c45f 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1003,7 +1003,7 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_filename(s->bs[i], s->bs[i]->filename, sizeof(s->bs[i]->filename));
+        bdrv_refresh_filename(s->bs[i]);
         if (!s->bs[i]->full_open_options) {
             return;
         }
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index e3d2d04..30df852 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -210,6 +210,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     bs->sg = bs->file->sg;
 
     if (bs->probed && !bdrv_is_read_only(bs)) {
+        char *filename = bdrv_filename_alloc(bs->file);
         fprintf(stderr,
                 "WARNING: Image format was not specified for '%s' and probing "
                 "guessed raw.\n"
@@ -217,7 +218,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                 "raw images, write operations on block 0 will be restricted.\n"
                 "         Specify the 'raw' format explicitly to remove the "
                 "restrictions.\n",
-                bs->file->filename);
+                filename);
+        g_free(filename);
     }
 
     return 0;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47fec63..629c1ed 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -782,13 +782,16 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
 
     if (logs.valid) {
         if (bs->read_only) {
+            char *filename = bdrv_filename_alloc(bs);
+
             ret = -EPERM;
             error_setg_errno(errp, EPERM,
                              "VHDX image file '%s' opened read-only, but "
                              "contains a log that needs to be replayed.  To "
                              "replay the log, execute:\n qemu-img check -r "
                              "all '%s'",
-                             bs->filename, bs->filename);
+                             filename, filename);
+            g_free(filename);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vmdk.c b/block/vmdk.c
index fbaab67..1981281 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -444,9 +444,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                      extent->l1_table,
                      l1_size);
     if (ret < 0) {
+        char *extent_filename = bdrv_filename_alloc(extent->file);
         error_setg_errno(errp, -ret,
                          "Could not read l1 table from extent '%s'",
-                         extent->file->filename);
+                         extent_filename);
+        g_free(extent_filename);
         goto fail_l1;
     }
     for (i = 0; i < extent->l1_size; i++) {
@@ -464,9 +466,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                          extent->l1_backup_table,
                          l1_size);
         if (ret < 0) {
+            char *extent_filename = bdrv_filename_alloc(extent->file);
             error_setg_errno(errp, -ret,
                              "Could not read l1 backup table from extent '%s'",
-                             extent->file->filename);
+                             extent_filename);
+            g_free(extent_filename);
             goto fail_l1b;
         }
         for (i = 0; i < extent->l1_size; i++) {
@@ -495,9 +499,11 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char *filename = bdrv_filename_alloc(file);
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->filename);
+                         filename);
+        g_free(filename);
         return ret;
     }
     ret = vmdk_add_extent(bs, file, false,
@@ -572,9 +578,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char *filename = bdrv_filename_alloc(file);
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->filename);
+                         filename);
+        g_free(filename);
         return -EINVAL;
     }
     if (header.capacity == 0) {
@@ -818,8 +826,10 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
             !desc_file_path[0])
         {
+            char *filename = bdrv_filename_alloc(bs->file);
             error_setg(errp, "Cannot use relative extent paths with VMDK "
-                       "descriptor file '%s'", bs->file->filename);
+                       "descriptor file '%s'", filename);
+            g_free(filename);
             return -EINVAL;
         }
 
@@ -2086,7 +2096,7 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
     ImageInfo *info = g_new0(ImageInfo, 1);
 
     *info = (ImageInfo){
-        .filename         = g_strdup(extent->file->filename),
+        .filename         = bdrv_filename_alloc(extent->file),
         .format           = g_strdup(extent->type),
         .virtual_size     = extent->sectors * BDRV_SECTOR_SIZE,
         .compressed       = extent->compressed,
diff --git a/block/vpc.c b/block/vpc.c
index 3e385d9..064b2f8 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -204,9 +204,12 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     checksum = be32_to_cpu(footer->checksum);
     footer->checksum = 0;
-    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
+    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) {
+        char *filename = bdrv_filename_alloc(bs);
         fprintf(stderr, "block-vpc: The header checksum of '%s' is "
-            "incorrect.\n", bs->filename);
+                "incorrect.\n", filename);
+        g_free(filename);
+    }
 
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = cpu_to_be32(checksum);
diff --git a/blockdev.c b/blockdev.c
index 62a4586..54a1c24 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -227,10 +227,12 @@ bool drive_check_orphaned(void)
         /* Unless this is a default drive, this may be an oversight. */
         if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
             dinfo->type != IF_NONE) {
+            char *filename = bdrv_filename_alloc(blk_bs(blk));
             fprintf(stderr, "Warning: Orphaned drive without device: "
                     "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
+                    blk_name(blk), filename, if_name[dinfo->type],
                     dinfo->bus, dinfo->unit);
+            g_free(filename);
             rs = true;
         }
     }
@@ -1507,10 +1509,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
+        char *old_filename = bdrv_filename_alloc(state->old_bs);
         bdrv_img_create(new_image_file, format,
-                        state->old_bs->filename,
+                        old_filename,
                         state->old_bs->drv->format_name,
                         NULL, -1, flags, &local_err, false);
+        g_free(old_filename);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -2388,9 +2392,11 @@ void qmp_block_commit(const char *device,
     top_bs = bs;
 
     if (has_top && top) {
-        if (strcmp(bs->filename, top) != 0) {
+        char *filename = bdrv_filename_alloc(bs);
+        if (strcmp(filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
+        g_free(filename);
     }
 
     if (top_bs == NULL) {
@@ -2536,9 +2542,11 @@ void qmp_drive_backup(const char *device, const char *target,
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         assert(format && drv);
         if (source) {
-            bdrv_img_create(target, format, source->filename,
+            char *source_filename = bdrv_filename_alloc(source);
+            bdrv_img_create(target, format, source_filename,
                             source->drv->format_name, NULL,
                             size, flags, &local_err, false);
+            g_free(source_filename);
         } else {
             bdrv_img_create(target, format, NULL, NULL, NULL,
                             size, flags, &local_err, false);
@@ -2781,15 +2789,19 @@ void qmp_drive_mirror(const char *device, const char *target,
         bdrv_img_create(target, format,
                         NULL, NULL, NULL, size, flags, &local_err, false);
     } else {
+        char *source_filename;
+
         switch (mode) {
         case NEW_IMAGE_MODE_EXISTING:
             break;
         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
             /* create new image with backing file */
+            source_filename = bdrv_filename_alloc(source);
             bdrv_img_create(target, format,
-                            source->filename,
+                            source_filename,
                             source->drv->format_name,
                             NULL, size, flags, &local_err, false);
+            g_free(source_filename);
             break;
         default:
             abort();
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 14ad4c3..677c05a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -370,7 +370,6 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    char filename[PATH_MAX];
     char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
                                     this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
-- 
2.4.3

[Qemu-devel] [PATCH v4 6/6] iotests: Test changed Quorum filename

[Max Reitz]

After drive-mirror replacing a Quorum child, the filename of the Quorum
BDS should reflect the change. This patch replaces the existing test for
whether the operation did actually exchange the BDS (which simply tested
whether the new BDS existed) by a test which examines the children list
contained in the Quorum BDS's filename as returned by query-block.

As a nice side effect of confirming that the new BDS actually belongs to
the Quorum BDS, this checks whether the filename was properly updated.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/041 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index de8ea15..ad8ce83 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -18,6 +18,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import json
 import time
 import os
 import iotests
@@ -914,10 +915,18 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait(drive="quorum0")
-        result = self.vm.qmp('query-named-block-nodes')
-        self.assert_qmp(result, 'return[0]/file', quorum_repair_img)
-        # TODO: a better test requiring some QEMU infrastructure will be added
-        #       to check that this file is really driven by quorum
+
+        result = self.vm.qmp('query-block')
+        for blockdev in result['return']:
+            if blockdev['device'] == 'quorum0':
+                filename = blockdev['inserted']['image']['filename']
+                self.assertEquals(filename[:5], 'json:')
+                children = json.loads(filename[5:])['children']
+                self.assertEquals(children[0]['file']['filename'], quorum_img1)
+                self.assertEquals(children[1]['file']['filename'],
+                                  quorum_repair_img)
+                self.assertEquals(children[2]['file']['filename'], quorum_img3)
+
         self.vm.shutdown()
 
 if __name__ == '__main__':
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3148 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> Instead of returning a pointer to the filename, copy it into a buffer
> specified by the caller.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 25 ++++++++++++++++++-------
>  include/block/block.h |  2 +-
>  monitor.c             |  6 +++++-
>  3 files changed, 24 insertions(+), 9 deletions(-)

Would it be better to just have bdrv_get_encrypted_filename() return a
g_malloc'd buffer, instead of making the caller do it?  Then the buffer
can be g_strdup'd to the correct size, instead of over-allocating
PATH_MAX bytes when a smaller size will usually do.

> 
> diff --git a/block.c b/block.c
> index d088ee0..41b0f85 100644
> --- a/block.c
> +++ b/block.c
> @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
>          }
>      } else {
>          if (bdrv_key_required(bs)) {
> +            char *enc_filename = g_malloc(PATH_MAX);
> +            bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
>              error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
>                        "'%s' (%s) is encrypted",
>                        bdrv_get_device_or_node_name(bs),
> -                      bdrv_get_encrypted_filename(bs));
> +                      enc_filename);

Should you assert(enc_filename) to prove we know we aren't calling
error_set("%s", NULL)?  After all, bdrv_get_encrypted_filename() can
return NULL, but only if no encrypted name is available; while
bdrv_key_required() implies that an encrypted name is available.


> -const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
> +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
>  {
> -    if (bs->backing_hd && bs->backing_hd->encrypted)
> -        return bs->backing_file;
> -    else if (bs->encrypted)
> -        return bs->filename;
> -    else
> +    if (sz > INT_MAX) {
> +        sz = INT_MAX;
> +    }
> +
> +    if (bs->backing_hd && bs->backing_hd->encrypted) {
> +        pstrcpy(dest, sz, bs->backing_file);
> +        return dest;

Again, using g_strdup() here instead of making the caller pass in a
buffer might be nicer semantics (certainly fewer places that have to
pre-allocate g_malloc0(PATH_MAX) bytes).


> +++ b/monitor.c
> @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
>                                  BlockCompletionFunc *completion_cb,
>                                  void *opaque)
>  {
> +    char *enc_filename;
>      int err;
>  
> +    enc_filename = g_malloc(PATH_MAX);
> +    bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
>      monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
> -                   bdrv_get_encrypted_filename(bs));
> +                   enc_filename);
> +    g_free(enc_filename);

And once again, an assert(enc_filename) might be nice to be sure we
aren't dealing with a NULL return.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/6] block: Avoid BlockDriverState.filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> In places which directly pass a filename to the OS, we should not use
> the filename field at all but exact_filename instead (although the
> former currently equals the latter if that is set).
> 
> In raw_open_common(), we do not need to access BDS.filename because we
> already have a local variable pointing to the filename.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c           |  5 +++--
>  block/gluster.c   |  2 +-
>  block/raw-posix.c | 12 ++++++------
>  block/raw-win32.c |  4 ++--
>  4 files changed, 12 insertions(+), 11 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> Split the part which actually refreshes the BlockDriverState.filename
> field off of bdrv_refresh_filename() into a more generic function
> bdrv_filename(), which first calls bdrv_refresh_filename() and then
> stores a qemu-usable filename into the given buffer instead of
> BlockDriverState.filename.
> 
> Since bdrv_refresh_filename() therefore no longer refreshes that field,
> some calls to that function have to be replaced by calls to
> bdrv_filename() "manually" refreshing the BDS filename field (this is
> only temporary).
> 
> Additionally, a wrapper function bdrv_filename_alloc() is added which
> allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer
> and returns it, since needing a temporary buffer for the filename is a
> rather common pattern.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 39 ++++++++++++++++++++++++++++++++-------
>  block/blkverify.c     |  3 ++-
>  block/quorum.c        |  2 +-
>  include/block/block.h |  2 ++
>  4 files changed, 37 insertions(+), 9 deletions(-)
> 

> +/* First refreshes exact_filename and full_open_options by calling
> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
> + * the target buffer. Otherwise, full_open_options is converted to a JSON
> + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
> + * put there.
> + *
> + * If sz > 0, the string put into the buffer will always be null-terminated.
> + *
> + * Returns @dest.
> + */
> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)
> +{

How does one tell if 'sz' was large enough, vs. too short and therefore
the snprintf() truncated the resulting string?  Would the code be any
simpler if this always returned a freshly g_malloc'd string of the right
length, rather than making the caller have to pre-allocate and guess
whether the allocation was big enough?

> +    bdrv_refresh_filename(bs);
> +
> +    if (sz > INT_MAX) {
> +        sz = INT_MAX;
> +    }
>  
>      if (bs->exact_filename[0]) {
> -        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
> +        pstrcpy(dest, sz, bs->exact_filename);
>      } else if (bs->full_open_options) {
>          QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
> -        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
> -                 qstring_get_str(json));
> +        snprintf(dest, sz, "json:%s", qstring_get_str(json));
>          QDECREF(json);
>      }
> +
> +    return dest;

In other words, I think it's very dangerous to use snprintf() without
checking whether the result fit.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 4/6] qemu-img: Use bdrv_filename_alloc() for map

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> Replaces bs->filename by the result of bdrv_filename_alloc() in the
> qemu-img map subcommand. Since that value is queried relatively often,
> however, it should be cached.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 5/6] block: Drop BlockDriverState.filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> That field is now only used during initialization of BlockDriverStates
> (opening images) and for error or warning messages. Performance is not
> that much of an issue here, so we can drop the field and replace its use
> by a call to bdrv_filename() or bdrv_filename_alloc(). By doing so we
> can ensure the result always to be recent, whereas the contents of
> BlockDriverState.filename may have been obsoleted by manipulations of
> single BlockDriverStates or of the BDS graph.
> 
> The users of the BDS filename field were changed as follows:
> - copying the filename into another buffer is trivially replaced by
>   using bdrv_filename() instead of the copy function
> - strdup() on the filename is replaced by a call to
>   bdrv_filename_alloc()
> - bdrv_filename(bs, bs->filename, sizeof(bs->filename)) is replaced by
>   bdrv_refresh_filename(bs)
>   (these were introduced by the previous patch)
> - anywhere else bdrv_filename_alloc() is used, any access to
>   BlockDriverState.filename is then replaced by the buffer returned, and
>   it is freed when it is no longer needed
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

Looks like clean conversions.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 6/6] iotests: Test changed Quorum filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On 08/18/2015 05:10 PM, Max Reitz wrote:
> After drive-mirror replacing a Quorum child, the filename of the Quorum
> BDS should reflect the change. This patch replaces the existing test for
> whether the operation did actually exchange the BDS (which simply tested
> whether the new BDS existed) by a test which examines the children list
> contained in the Quorum BDS's filename as returned by query-block.
> 
> As a nice side effect of confirming that the new BDS actually belongs to
> the Quorum BDS, this checks whether the filename was properly updated.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/041 | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 1/6] block: Change bdrv_get_encrypted_filename()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3803 bytes --]

On 31.08.2015 22:50, Eric Blake wrote:
> On 08/18/2015 05:10 PM, Max Reitz wrote:
>> Instead of returning a pointer to the filename, copy it into a buffer
>> specified by the caller.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c               | 25 ++++++++++++++++++-------
>>  include/block/block.h |  2 +-
>>  monitor.c             |  6 +++++-
>>  3 files changed, 24 insertions(+), 9 deletions(-)
> 
> Would it be better to just have bdrv_get_encrypted_filename() return a
> g_malloc'd buffer, instead of making the caller do it?  Then the buffer
> can be g_strdup'd to the correct size, instead of over-allocating
> PATH_MAX bytes when a smaller size will usually do.

Probably, yes. I wanted the interface to be similar to the new
bdrv_filename(), but since all the callers of
bdrv_get_encrypted_filename() would have to allocate the buffer anyway,
I probably should pass through bdrv_filename_alloc() (and more on that
in my reply to your review of patch 3).

>>
>> diff --git a/block.c b/block.c
>> index d088ee0..41b0f85 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
>>          }
>>      } else {
>>          if (bdrv_key_required(bs)) {
>> +            char *enc_filename = g_malloc(PATH_MAX);
>> +            bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
>>              error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
>>                        "'%s' (%s) is encrypted",
>>                        bdrv_get_device_or_node_name(bs),
>> -                      bdrv_get_encrypted_filename(bs));
>> +                      enc_filename);
> 
> Should you assert(enc_filename) to prove we know we aren't calling
> error_set("%s", NULL)?  After all, bdrv_get_encrypted_filename() can
> return NULL, but only if no encrypted name is available; while
> bdrv_key_required() implies that an encrypted name is available.

Well, in this version, enc_filename will never be NULL because it has
been returned by g_malloc(). But it may become necessary with the
interface change you suggested (bdrv_get_encrypted_filename() allocating
the buffer and returning it), so I'll keep it in mind.

Thanks for your review and your suggestions! :-)

Max

>> -const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
>> +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
>>  {
>> -    if (bs->backing_hd && bs->backing_hd->encrypted)
>> -        return bs->backing_file;
>> -    else if (bs->encrypted)
>> -        return bs->filename;
>> -    else
>> +    if (sz > INT_MAX) {
>> +        sz = INT_MAX;
>> +    }
>> +
>> +    if (bs->backing_hd && bs->backing_hd->encrypted) {
>> +        pstrcpy(dest, sz, bs->backing_file);
>> +        return dest;
> 
> Again, using g_strdup() here instead of making the caller pass in a
> buffer might be nicer semantics (certainly fewer places that have to
> pre-allocate g_malloc0(PATH_MAX) bytes).
> 
> 
>> +++ b/monitor.c
>> @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
>>                                  BlockCompletionFunc *completion_cb,
>>                                  void *opaque)
>>  {
>> +    char *enc_filename;
>>      int err;
>>  
>> +    enc_filename = g_malloc(PATH_MAX);
>> +    bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX);
>>      monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
>> -                   bdrv_get_encrypted_filename(bs));
>> +                   enc_filename);
>> +    g_free(enc_filename);
> 
> And once again, an assert(enc_filename) might be nice to be sure we
> aren't dealing with a NULL return.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 3/6] block: Add bdrv_filename()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 31.08.2015 23:00, Eric Blake wrote:
> On 08/18/2015 05:10 PM, Max Reitz wrote:
>> Split the part which actually refreshes the BlockDriverState.filename
>> field off of bdrv_refresh_filename() into a more generic function
>> bdrv_filename(), which first calls bdrv_refresh_filename() and then
>> stores a qemu-usable filename into the given buffer instead of
>> BlockDriverState.filename.
>>
>> Since bdrv_refresh_filename() therefore no longer refreshes that field,
>> some calls to that function have to be replaced by calls to
>> bdrv_filename() "manually" refreshing the BDS filename field (this is
>> only temporary).
>>
>> Additionally, a wrapper function bdrv_filename_alloc() is added which
>> allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer
>> and returns it, since needing a temporary buffer for the filename is a
>> rather common pattern.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c               | 39 ++++++++++++++++++++++++++++++++-------
>>  block/blkverify.c     |  3 ++-
>>  block/quorum.c        |  2 +-
>>  include/block/block.h |  2 ++
>>  4 files changed, 37 insertions(+), 9 deletions(-)
>>
> 
>> +/* First refreshes exact_filename and full_open_options by calling
>> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
>> + * the target buffer. Otherwise, full_open_options is converted to a JSON
>> + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
>> + * put there.
>> + *
>> + * If sz > 0, the string put into the buffer will always be null-terminated.
>> + *
>> + * Returns @dest.
>> + */
>> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)
>> +{
> 
> How does one tell if 'sz' was large enough, vs. too short and therefore
> the snprintf() truncated the resulting string?  Would the code be any
> simpler if this always returned a freshly g_malloc'd string of the right
> length, rather than making the caller have to pre-allocate and guess
> whether the allocation was big enough?
> 
>> +    bdrv_refresh_filename(bs);
>> +
>> +    if (sz > INT_MAX) {
>> +        sz = INT_MAX;
>> +    }
>>  
>>      if (bs->exact_filename[0]) {
>> -        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
>> +        pstrcpy(dest, sz, bs->exact_filename);
>>      } else if (bs->full_open_options) {
>>          QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
>> -        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
>> -                 qstring_get_str(json));
>> +        snprintf(dest, sz, "json:%s", qstring_get_str(json));
>>          QDECREF(json);
>>      }
>> +
>> +    return dest;
> 
> In other words, I think it's very dangerous to use snprintf() without
> checking whether the result fit.

There are a couple of places in qemu which copy BDS.filename into a
pre-existing buffer, so I'd rather not just drop bdrv_filename() as it is.

I guess I'll just make it so that calling bdrv_filename() with a NULL
dest will result in the buffer being allocated.

Note however that there are a couple of places in qemu which rely on
filenames not exceeding PATH_MAX (by using PATH_MAX sized buffers for
holding them). Maybe we'll eventually get around to fix them, but right
now it's a limitation not introduced by this series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]