If the backing file is overridden, this most probably does change the
guest-visible data of a BDS.  Therefore, we will need to consider this
in bdrv_refresh_filename().

To see whether it has been overridden, we might want to compare
bs->backing_file and bs->backing->bs->filename.  However,
bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
to change the backing child at runtime, without modifying the image
header), so bs->backing_file most of the time simply contains a copy of
bs->backing->bs->filename anyway, so it is useless for such a
comparison.

This patch adds an auto_backing_file BDS field which contains the
backing file path as indicated by the image header, which is not changed
by bdrv_set_backing_hd().

Because of bdrv_refresh_filename() magic, however, a BDS's filename may
differ from what has been specified during bdrv_open().  Then, the
comparison between bs->auto_backing_file and bs->backing->bs->filename
may fail even though bs->backing was opened from bs->auto_backing_file.
To mitigate this, we can copy the real BDS's filename (after the whole
bdrv_open() and bdrv_refresh_filename() process) into
bs->auto_backing_file, if we know the former has been opened based on
the latter.  This is only possible if no options modifying the backing
file's behavior have been specified, though.  To simplify things, this
patch only copies the filename from the backing file if no options have
been specified for it at all.

Furthermore, there are cases where an overlay is created by qemu which
already contains a BDS's filename (e.g. in blockdev-snapshot-sync).  We
do not need to worry about updating the overlay's bs->auto_backing_file
there, because we actually wrote a post-bdrv_refresh_filename() filename
into the image header.

So all in all, there will be false negatives where (as of a future
patch) bdrv_refresh_filename() will assume that the backing file differs
from what was specified in the image header, even though it really does
not.  However, these cases should be limited to where (1) the user
actually did override something in the backing chain (e.g. by specifying
options for the backing file), or (2) the user executed a QMP command to
change some node's backing file (e.g. change-backing-file or
block-commit with @backing-file given) where the given filename does not
happen to coincide with qemu's idea of the backing BDS's filename.

Then again, (1) really is limited to -drive.  With -blockdev or
blockdev-add, you have to adhere to the schema, so a user cannot give
partial "unimportant" options (e.g. by just setting backing.node-name
and leaving the rest to the image header).  Therefore, trying to fix
this would mean trying to fix something for -drive only.

To improve on (2), we would need a full infrastructure to "canonicalize"
an arbitrary filename (+ options), so it can be compared against
another.  That seems a bit over the top, considering that filenames
nowadays are there mostly for the user's entertainment.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h |  4 ++++
 block.c                   | 19 +++++++++++++++++++
 block/qcow.c              |  7 +++++--
 block/qcow2.c             | 10 +++++++---
 block/qed.c               |  7 +++++--
 block/vmdk.c              |  6 ++++--
 6 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 903b9c1034..5ebe3917e5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -696,6 +696,10 @@ struct BlockDriverState {
     char filename[PATH_MAX];
     char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
                                     this file image */
+    /* The backing filename indicated by the image header; if we ever
+     * open this file, then this is replaced by the resulting BDS's
+     * filename (i.e. after a bdrv_refresh_filename() run). */
+    char auto_backing_file[PATH_MAX];
     char backing_format[16]; /* if non-zero and backing_file exists */
 
     QDict *full_open_options;
diff --git a/block.c b/block.c
index feabe46061..655f662997 100644
--- a/block.c
+++ b/block.c
@@ -2279,6 +2279,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
     char *bdref_key_dot;
     const char *reference = NULL;
     int ret = 0;
+    bool implicit_backing = false;
     BlockDriverState *backing_hd;
     QDict *options;
     QDict *tmp_parent_options = NULL;
@@ -2314,6 +2315,16 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
         qobject_unref(options);
         goto free_exit;
     } else {
+        if (qdict_size(options) == 0) {
+            /* If the user specifies options that do not modify the
+             * backing file's behavior, we might still consider it the
+             * implicit backing file.  But it's easier this way, and
+             * just specifying some of the backing BDS's options is
+             * only possible with -drive anyway (otherwise the QAPI
+             * schema forces the user to specify everything). */
+            implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file);
+        }
+
         bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX,
                                        &local_err);
         if (local_err) {
@@ -2347,6 +2358,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
     }
     bdrv_set_aio_context(backing_hd, bdrv_get_aio_context(bs));
 
+    if (implicit_backing) {
+        bdrv_refresh_filename(backing_hd);
+        pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+                backing_hd->filename);
+    }
+
     /* Hook up the backing file link; drop our reference, bs owns the
      * backing_hd reference now */
     bdrv_set_backing_hd(bs, backing_hd, &local_err);
@@ -3660,6 +3677,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     if (ret == 0) {
         pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
         pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
+        pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+                backing_file ?: "");
     }
     return ret;
 }
diff --git a/block/qcow.c b/block/qcow.c
index 385d935258..20f610ec74 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -31,6 +31,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/bswap.h"
+#include "qemu/cutils.h"
 #include <zlib.h>
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
@@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
         ret = bdrv_pread(bs->file, header.backing_file_offset,
-                   bs->backing_file, len);
+                   bs->auto_backing_file, len);
         if (ret < 0) {
             goto fail;
         }
-        bs->backing_file[len] = '\0';
+        bs->auto_backing_file[len] = '\0';
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                bs->auto_backing_file);
     }
 
     /* Disable migration when qcow images are used */
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..6ca57b7d21 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1469,13 +1469,15 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
             goto fail;
         }
         ret = bdrv_pread(bs->file, header.backing_file_offset,
-                         bs->backing_file, len);
+                         bs->auto_backing_file, len);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not read backing file name");
             goto fail;
         }
-        bs->backing_file[len] = '\0';
-        s->image_backing_file = g_strdup(bs->backing_file);
+        bs->auto_backing_file[len] = '\0';
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                bs->auto_backing_file);
+        s->image_backing_file = g_strdup(bs->auto_backing_file);
     }
 
     /* Internal snapshots */
@@ -2469,6 +2471,8 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
         return -EINVAL;
     }
 
+    pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+            backing_file ?: "");
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
     pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
 
diff --git a/block/qed.c b/block/qed.c
index 689ea9d4d5..1419cbe3c9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -454,11 +454,14 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
         }
 
         ret = qed_read_string(bs->file, s->header.backing_filename_offset,
-                              s->header.backing_filename_size, bs->backing_file,
-                              sizeof(bs->backing_file));
+                              s->header.backing_filename_size,
+                              bs->auto_backing_file,
+                              sizeof(bs->auto_backing_file));
         if (ret < 0) {
             return ret;
         }
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                bs->auto_backing_file);
 
         if (s->header.features & QED_F_BACKING_FORMAT_NO_PROBE) {
             pstrcpy(bs->backing_format, sizeof(bs->backing_format), "raw");
diff --git a/block/vmdk.c b/block/vmdk.c
index 87fdd647df..09157da1df 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -386,12 +386,14 @@ static int vmdk_parent_open(BlockDriverState *bs)
             ret = -EINVAL;
             goto out;
         }
-        if ((end_name - p_name) > sizeof(bs->backing_file) - 1) {
+        if ((end_name - p_name) > sizeof(bs->auto_backing_file) - 1) {
             ret = -EINVAL;
             goto out;
         }
 
-        pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
+        pstrcpy(bs->auto_backing_file, end_name - p_name + 1, p_name);
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                bs->auto_backing_file);
     }
 
 out:
-- 
2.17.1

On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
> If the backing file is overridden, this most probably does change the
> guest-visible data of a BDS.  Therefore, we will need to consider this
> in bdrv_refresh_filename().
>
> To see whether it has been overridden, we might want to compare
> bs->backing_file and bs->backing->bs->filename.  However,
> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
> to change the backing child at runtime, without modifying the image
> header), so bs->backing_file most of the time simply contains a copy of
> bs->backing->bs->filename anyway, so it is useless for such a
> comparison.

What's the point of bs->backing_file then? In what cases is it different
from backing->bs->filename?

Berto

On 2018-09-05 16:22, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>> If the backing file is overridden, this most probably does change the
>> guest-visible data of a BDS.  Therefore, we will need to consider this
>> in bdrv_refresh_filename().
>>
>> To see whether it has been overridden, we might want to compare
>> bs->backing_file and bs->backing->bs->filename.  However,
>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>> to change the backing child at runtime, without modifying the image
>> header), so bs->backing_file most of the time simply contains a copy of
>> bs->backing->bs->filename anyway, so it is useless for such a
>> comparison.
> 
> What's the point of bs->backing_file then? In what cases is it different
> from backing->bs->filename?

Good question!  Yes, why?

One thing is when you have detached the backing file, bs->backing_file
will stay the same, even though backing is now NULL.  But that is pretty
much useless, I couldn't find any part in the block layer which relies
on this.

The other is... Fun.  When you create the BDS, it is different, because
the format driver will just put the filename it got from the image
header there.  So imagine you have this configuration:

$ mkdir foo
$ qemu-img create -f qcow2 foo/base.qcow2 64M
$ qemu-img create -f qcow2 -b base.qcow2 foo/top.qcow2

Now when you open foo/top.qcow2, backing_file will contain "base.qcow2"
at first.  This is what bdrv_open_backing_file() expects (because that's
what bdrv_get_full_backing_filename() is for).

But when the backing BDS is attached, bdrv_set_backing_hd() (through
bdrv_backing_attach()) will update backing_file to "foo/base.qcow2" (or
probably the absolute equivalent, I can't remember).

That seems really pointless to me, and after more investigation, I
couldn't find any reason for why bdrv_backing_attach() should update
backing_file.  Especially considering this behavior breaks things, like
bdrv_find_backing_image(), which again would expect "base.qcow2" (i.e. a
filename relative to the overlay).

So in short, functions in the block layer that query backing_file
generally expect it to be what's in the image header; while on the other
hand functions that set backing_file ignore that expectation half of the
time.


So consequentially, there is a "block: Leave BDS.backing_file constant"
patch in my "block: Deal with filters" series.  It makes backing_file
always report what the image header says.


Now you will probably ask: OK, nice, but why do you still need
auto_backing_file then?  Wouldn't that change be enough to just use
backing_file?

The issue here is that the image header may contain a filename that
bdrv_refresh_filename() will transform.  Say your image header says
"nbd:localhost:10809" for the backing file.  So that's what backing_file
will report, too.  bdrv_refresh_filename() however will make
backing->bs->filename report "nbd://localhost:10809".  So if you'd
compare backing_file against backing->bs->filename, you'd see a
difference and you'd have to assume the backing file was overridden,
when in fact it wasn't.

That's what we still need auto_backing_file for: It does not always
reflect the image header, instead it is supposed to reflect the
bdrv_refresh_filename() result for the BDS that is opened when you open
the backing filename given in the image header.

(At least that would be desirable; we cannot always conform to that
specification, but we'll try at least.)

Max

On Fri 07 Sep 2018 01:32:58 PM CEST, Max Reitz wrote:
> On 2018-09-05 16:22, Alberto Garcia wrote:
>> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>>> If the backing file is overridden, this most probably does change the
>>> guest-visible data of a BDS.  Therefore, we will need to consider this
>>> in bdrv_refresh_filename().
>>>
>>> To see whether it has been overridden, we might want to compare
>>> bs->backing_file and bs->backing->bs->filename.  However,
>>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>>> to change the backing child at runtime, without modifying the image
>>> header), so bs->backing_file most of the time simply contains a copy of
>>> bs->backing->bs->filename anyway, so it is useless for such a
>>> comparison.
>> 
>> What's the point of bs->backing_file then? In what cases is it different
>> from backing->bs->filename?
>
> Good question!  Yes, why?
>
> One thing is when you have detached the backing file, bs->backing_file
> will stay the same, even though backing is now NULL.  But that is
> pretty much useless, I couldn't find any part in the block layer which
> relies on this.
  [...]
> But when the backing BDS is attached, bdrv_set_backing_hd() (through
> bdrv_backing_attach()) will update backing_file to "foo/base.qcow2"
> (or probably the absolute equivalent, I can't remember).

But then why do you need bs->backing_file at all? What are the cases
where you need bs->backing_file but you can't use directly the value in
bs->backing->bs->filename because it's wrong or missing?

> So consequentially, there is a "block: Leave BDS.backing_file
> constant" patch in my "block: Deal with filters" series.  It makes
> backing_file always report what the image header says.

Ok, so after tha patch bs->backing_file is guaranteed to be exactly as
it's written on the image header.

And bs->auto_backing_file is the backing filename after having called
bdrv_refresh_filename(). And that's supposed to be exactly the same as
bs->backing->bs->filename. So I have the same question again, why do you
need bs->auto_backing_filename and in what cases is it different from
bs->auto_backing_file different from bs->backing->bs->filename?

Berto

On 2018-09-07 14:28, Alberto Garcia wrote:
> On Fri 07 Sep 2018 01:32:58 PM CEST, Max Reitz wrote:
>> On 2018-09-05 16:22, Alberto Garcia wrote:
>>> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>>>> If the backing file is overridden, this most probably does change the
>>>> guest-visible data of a BDS.  Therefore, we will need to consider this
>>>> in bdrv_refresh_filename().
>>>>
>>>> To see whether it has been overridden, we might want to compare
>>>> bs->backing_file and bs->backing->bs->filename.  However,
>>>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>>>> to change the backing child at runtime, without modifying the image
>>>> header), so bs->backing_file most of the time simply contains a copy of
>>>> bs->backing->bs->filename anyway, so it is useless for such a
>>>> comparison.
>>>
>>> What's the point of bs->backing_file then? In what cases is it different
>>> from backing->bs->filename?
>>
>> Good question!  Yes, why?
>>
>> One thing is when you have detached the backing file, bs->backing_file
>> will stay the same, even though backing is now NULL.  But that is
>> pretty much useless, I couldn't find any part in the block layer which
>> relies on this.
>   [...]
>> But when the backing BDS is attached, bdrv_set_backing_hd() (through
>> bdrv_backing_attach()) will update backing_file to "foo/base.qcow2"
>> (or probably the absolute equivalent, I can't remember).
> 
> But then why do you need bs->backing_file at all? What are the cases
> where you need bs->backing_file but you can't use directly the value in
> bs->backing->bs->filename because it's wrong or missing?

The main case is when bs->backing has not been opened yet.  Then
bdrv_open_backing_file() needs some place that tells it what image file
to open.

>> So consequentially, there is a "block: Leave BDS.backing_file
>> constant" patch in my "block: Deal with filters" series.  It makes
>> backing_file always report what the image header says.
> 
> Ok, so after tha patch bs->backing_file is guaranteed to be exactly as
> it's written on the image header.

Yes.

> And bs->auto_backing_file is the backing filename after having called
> bdrv_refresh_filename(). And that's supposed to be exactly the same as
> bs->backing->bs->filename.

No.

It's supposed to be exactly the same, *if* bs->backing->bs has been
opened only based on bs->backing_file.

The user can override the backing file (either as a whole through the
@backing node reference, or at least some of its options as a dict under
@backing), and then bs->auto_backing_filename is not supposed to be the
same as bs->backing->bs->filename.

> So I have the same question again, why do you
> need bs->auto_backing_filename and in what cases is it different from
> bs->auto_backing_file different from bs->backing->bs->filename?

The whole purpose of bs->auto_backing_file is so you can compare it with
bs->backing->bs->filename, see when it differs, and then you know that
the user has changed the backing file from what it would be based on the
image header alone (that is, if the user hadn't specified any @backing
option at all).

This is exactly what the commit message says.


It's just that for this comparison to really make sense, we would need
the refreshed filename of the BDS that is opened when you just take the
image header's backing filename.
If we know that the user has not specified any options that override it,
we copy bs->backing->bs->filename to bs->auto_backing_filename, because
we know it to be exactly that value that we want.
Otherwise, we just have to hope that refreshing the filename does not
change it from what the image header said.

(Now you could ask yourself, if we know the user hasn't specified any
options to override the backing file -- why don't we just store a flag?
I tried that, Kevin wasn't really happy about it because it's hard to
catch all corner cases.  There are many ways to change a backing file,
and if you want to find out when a backing file has been changed to
exactly what the image header says (this is the ideal outcome of the
commit block job, for instance), you basically get back to the same
problem of comparing filenames.)


Let me try to summarize what bs->auto_backing_filename is again.

We want to compare it against bs->backing->bs->filename, which is
basically bdrv_filename(bdrv_open(X)).  Question is, is X the same as
bs->backing_file?  (assuming bs->backing_file is what the image header says)

So ideally, bs->auto_backing_filename needs to be the result of
bdrv_filename(bdrv_open(bs->backing_file)).  But that is rather stupid
to do, so we don't do that.

The next best thing is just using bs->backing_file and hoping that
bdrv_refresh_filename() didn't change anything.  Sometimes, that hope is
futile, however.

So I try to get bdrv_refresh_filename() squeezed in there sometimes.
When at any point we know that bs->backing->bs is exactly
bdrv_open(bs->backing_file), we know that bs->backing->bs->filename is
bdrv_filename(bdrv_open(bs->backing_file)).  So then we can copy it into
bs->auto_backing_file -- *but only if we know that bs->backing->bs ==
bdrv_open(bs->backing_file)!*

Max

On Fri 07 Sep 2018 02:42:53 PM CEST, Max Reitz wrote:
> The whole purpose of bs->auto_backing_file is so you can compare it
> with bs->backing->bs->filename, see when it differs, and then you know
> that the user has changed the backing file from what it would be based
> on the image header alone (that is, if the user hadn't specified any
> @backing option at all).
  [...]
> (Now you could ask yourself, if we know the user hasn't specified any
> options to override the backing file -- why don't we just store a
> flag?  I tried that, Kevin wasn't really happy about it because it's
> hard to catch all corner cases.  There are many ways to change a
> backing file, and if you want to find out when a backing file has been
> changed to exactly what the image header says (this is the ideal
> outcome of the commit block job, for instance), you basically get back
> to the same problem of comparing filenames.)

Ok, I think this clarifies a lot. Thanks!

Berto

On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
> @@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>              goto fail;
>          }
>          ret = bdrv_pread(bs->file, header.backing_file_offset,
> -                   bs->backing_file, len);
> +                   bs->auto_backing_file, len);
>          if (ret < 0) {
>              goto fail;
>          }
> -        bs->backing_file[len] = '\0';
> +        bs->auto_backing_file[len] = '\0';
> +        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                bs->auto_backing_file);
>      }

The patch looks fine, I just wonder why you don't simply copy
bs->backing_file into bs->auto_backing_file instead of the other way
around, it would make the patch smaller.

Either way,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto