Re: [Qemu-block] [PULL 08/23] file-posix: Forbid trying to change unsupported options during reopen

[Peter Maydell]

On 1 October 2018 at 18:18, Kevin Wolf  wrote:
> From: Alberto Garcia 
>
> The file-posix code is used for the "file", "host_device" and
> "host_cdrom" drivers, and it allows reopening images. However the only
> option that is actually processed is "x-check-cache-dropped", and
> changes in all other options (e.g. "filename") are silently ignored:
>
>(qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"
>
> While we could allow changing some of the other options, let's keep
> things as they are for now but return an error if the user tries to
> change any of them.
>
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Max Reitz 
> Signed-off-by: Kevin Wolf 
> ---
>  block/file-posix.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index bc5e54560a..2da3a76355 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -849,8 +849,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>  goto out;
>  }
>
> -rs->check_cache_dropped = qemu_opt_get_bool(opts, 
> "x-check-cache-dropped",
> -false);
> +rs->check_cache_dropped =
> +qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
> +
> +/* This driver's reopen function doesn't currently allow changing
> + * other options, so let's put them back in the original QDict and
> + * bdrv_reopen_prepare() will detect changes and complain. */
> +qemu_opts_to_qdict(opts, state->options);

Hi. Coverity is suspicious about this call to qemu_opts_to_qdict()
because it returns a value which this callsite is ignoring but
almost all others don't ignore (CID 1395991). Is it correct?

(It also doesn't like the call in block.c: CID 1395989.)

thanks
-- PMM

Re: [Qemu-block] [PULL 08/23] file-posix: Forbid trying to change unsupported options during reopen

[Kevin Wolf]

Am 05.10.2018 um 14:55 hat Peter Maydell geschrieben:
> On 1 October 2018 at 18:18, Kevin Wolf  wrote:
> > From: Alberto Garcia 
> >
> > The file-posix code is used for the "file", "host_device" and
> > "host_cdrom" drivers, and it allows reopening images. However the only
> > option that is actually processed is "x-check-cache-dropped", and
> > changes in all other options (e.g. "filename") are silently ignored:
> >
> >(qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"
> >
> > While we could allow changing some of the other options, let's keep
> > things as they are for now but return an error if the user tries to
> > change any of them.
> >
> > Signed-off-by: Alberto Garcia 
> > Reviewed-by: Max Reitz 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/file-posix.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index bc5e54560a..2da3a76355 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -849,8 +849,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> >  goto out;
> >  }
> >
> > -rs->check_cache_dropped = qemu_opt_get_bool(opts, 
> > "x-check-cache-dropped",
> > -false);
> > +rs->check_cache_dropped =
> > +qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
> > +
> > +/* This driver's reopen function doesn't currently allow changing
> > + * other options, so let's put them back in the original QDict and
> > + * bdrv_reopen_prepare() will detect changes and complain. */
> > +qemu_opts_to_qdict(opts, state->options);
> 
> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict()
> because it returns a value which this callsite is ignoring but
> almost all others don't ignore (CID 1395991). Is it correct?
> 
> (It also doesn't like the call in block.c: CID 1395989.)

I think this is a false positive. qemu_opts_to_qdict() returns the dict
it was given, except if NULL was given, then it returns a newly
allocated one.

Kevin

[Qemu-block] bdrv_replace_child crashes

[Vladimir Sementsov-Ogievskiy]

Hi Kevin!

In commit 33a610c3986 "block: Involve block drivers in permission 
granting" you write:
     if (old_bs) {
     /* Update permissions for old node. This is guaranteed to succeed
  * because we're just taking a parent away, so we're loosening
  * restrictions. */
     bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
     bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, 
&error_abort);
     bdrv_set_perm(old_bs, perm, shared_perm);
     }

we have some crashes on this error_abort, because F_SETLK or F_OFD_SETLK 
return an error on NFS (for example when remote server not responding).

What to do with this? Is it safe just to ignore the error here, at least 
in case of bdrv_detach_child?

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PULL 08/23] file-posix: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote:
>> +rs->check_cache_dropped =
>> +qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
>> +
>> +/* This driver's reopen function doesn't currently allow changing
>> + * other options, so let's put them back in the original QDict and
>> + * bdrv_reopen_prepare() will detect changes and complain. */
>> +qemu_opts_to_qdict(opts, state->options);
>
> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict()
> because it returns a value which this callsite is ignoring but almost
> all others don't ignore (CID 1395991). Is it correct?

It's a false positive because qemu_opts_to_qdict() returns the same
value that is passed (unless that value is NULL).

We can fix the warning by doing

   state->options = qemu_opts_to_qdict(opts, state->options);

or by modifying qemu_opts_to_qdict() to get a QDict ** and return void.

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] qemu-img.c: comment typo fix

[Peter Maydell]

On 4 October 2018 at 17:48, Eric Blake  wrote:
> On 10/4/18 11:18 AM, Cleber Rosa wrote:
>>
>> A trivial comment typo fix.
>>
>> Signed-off-by: Cleber Rosa 

>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1085,7 +1085,7 @@ static int64_t find_nonzero(const uint8_t *buf,
>> int64_t n)
>>   }
>> /*
>> - * Returns true iff the first sector pointed to by 'buf' contains at
>> least
>> + * Returns true if the first sector pointed to by 'buf' contains at least
>
>
> NACK.  You're not the first person to propose this change.  However, "iff"
> is an English word (albeit archaic) which is shorthand for "if and only if",
> which has a distinct logical meaning separate from the weaker "if".
> Spelling it out in longhand instead of calling it a typo is probably
> acceptable, though.

Yes; if you happen to have a mathematical background then it's
a familiar abbreviation; but otherwise it isn't, and it's more
confusing than helpful I think. (I don't think it's archaic; it's
just commonly used in university-and-higher-level maths and not
elsewhere.)

 git grep '\Wiff\W'
shows a surprisingly large number of uses. (NB that some of those
are in 3rd-party code, notably the libdecnumber stuff, and should
probably not be changed.)

thanks
-- PMM

Re: [Qemu-block] [PULL 08/23] file-posix: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

On Fri 05 Oct 2018 03:40:09 PM CEST, Alberto Garcia wrote:
> On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote:
>>> +rs->check_cache_dropped =
>>> +qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
>>> +
>>> +/* This driver's reopen function doesn't currently allow changing
>>> + * other options, so let's put them back in the original QDict and
>>> + * bdrv_reopen_prepare() will detect changes and complain. */
>>> +qemu_opts_to_qdict(opts, state->options);
>>
>> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict()
>> because it returns a value which this callsite is ignoring but almost
>> all others don't ignore (CID 1395991). Is it correct?
>
> It's a false positive because qemu_opts_to_qdict() returns the same
> value that is passed (unless that value is NULL).

and I forgot to add: in this case it's guaranteed to be non-NULL, since
it's initialized in bdrv_reopen_queue_child().

Berto

Re: [Qemu-block] [PULL 08/23] file-posix: Forbid trying to change unsupported options during reopen

[Peter Maydell]

On 5 October 2018 at 14:40, Alberto Garcia  wrote:
> On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote:
>>> +rs->check_cache_dropped =
>>> +qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
>>> +
>>> +/* This driver's reopen function doesn't currently allow changing
>>> + * other options, so let's put them back in the original QDict and
>>> + * bdrv_reopen_prepare() will detect changes and complain. */
>>> +qemu_opts_to_qdict(opts, state->options);
>>
>> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict()
>> because it returns a value which this callsite is ignoring but almost
>> all others don't ignore (CID 1395991). Is it correct?
>
> It's a false positive because qemu_opts_to_qdict() returns the same
> value that is passed (unless that value is NULL).
>
> We can fix the warning by doing
>
>state->options = qemu_opts_to_qdict(opts, state->options);
>
> or by modifying qemu_opts_to_qdict() to get a QDict ** and return void.

I think just marking the issues as false-positives in the coverity
UI is sufficient here.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] tests/tcg/README: fix location for lm32 tests

[Alex Bennée]

Cleber Rosa  writes:

> Point to the right and obvious location for lm32 tests.
>
> Signed-off-by: Cleber Rosa 
> ---
>  tests/tcg/README | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/tcg/README b/tests/tcg/README
> index a5643d33e7..2a58f9a058 100644
> --- a/tests/tcg/README
> +++ b/tests/tcg/README
> @@ -10,6 +10,6 @@ with "make test-cris".
>
>  LM32
>  
> -The testsuite for LM32 is in tests/tcg/cris.  You can run it
> +The testsuite for LM32 is in tests/tcg/lm32.  You can run it
>  with "make test-lm32".

Acked-by: Alex Bennée 

It's a correct fix, although I note running "make test-lm32" doesn't do
much either in the top of the build dir or in that directory. It's
probably best to drop the second line until I fix it.

--
Alex Bennée

Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] qemu-img.c: comment typo fix

[Philippe Mathieu-Daudé]

On 04/10/2018 18:53, Cleber Rosa wrote:> On 10/4/18 12:48 PM, Eric Blake
wrote:
>> On 10/4/18 11:18 AM, Cleber Rosa wrote:
>>> A trivial comment typo fix.
>>>
>>> Signed-off-by: Cleber Rosa 
>>> ---
>>>   qemu-img.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index b12f4cd19b..a96b76c187 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1085,7 +1085,7 @@ static int64_t find_nonzero(const uint8_t *buf,
>>> int64_t n)
>>>   }
>>>     /*
>>> - * Returns true iff the first sector pointed to by 'buf' contains at
>>> least
>>> + * Returns true if the first sector pointed to by 'buf' contains at
>>> least
>>
>> NACK.  You're not the first person to propose this change.  However,
>> "iff" is an English word (albeit archaic) which is shorthand for "if and
>> only if", which has a distinct logical meaning separate from the weaker
>> "if".  Spelling it out in longhand instead of calling it a typo is
>> probably acceptable, though.
>>
> 
> Wow, thanks for the explanation.  I agree that spelling it out is a good
> option, as it'll probably prevent other people from falling into the
> same "language trap" as I did.

https://lists.gnu.org/archive/html/qemu-devel/2011-06/msg01269.html

https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04200.html

;)

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qemu-iotests: make 218 executable

[Philippe Mathieu-Daudé]

On 04/10/2018 18:18, Cleber Rosa wrote:
> Commit 990dc39c made all tests executable at the time, but 218 came in
> later, and missing those permissions.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  tests/qemu-iotests/218 | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 tests/qemu-iotests/218
> 
> diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
> old mode 100644
> new mode 100755

We could add a git pre-commit hook grep'ing for "^#!\
*(/usr/bin/env|/bin/(b?a)?sh)" in the 1st line and then checking the
file perms.

Re: [Qemu-block] [Qemu-devel] [PATCH 10/10] scripts/qemu.py: remove trailing quotes on docstring

[Philippe Mathieu-Daudé]

On 04/10/2018 18:18, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  scripts/qemu.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7abe26de69..676eb9709a 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -88,7 +88,7 @@ class QEMUMachine(object):
>  @param name: prefix for socket and log file names (default: qemu-PID)
>  @param test_dir: where to create socket and log file
>  @param monitor_address: address for QMP monitor
> -@param socket_scm_helper: helper program, required for send_fd_scm()"
> +@param socket_scm_helper: helper program, required for send_fd_scm()
>  @note: Qemu process is not started until launch() is used.
>  '''
>  if args is None:
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] qemu-iotests: fix filename containing checks

[Philippe Mathieu-Daudé]

On 04/10/2018 18:18, Cleber Rosa wrote:
> Commit cce293a2945 moved some functions from common.config to
> common.rc, but the error messages still reference the old file
> location.
> 
> Signed-off-by: Cleber Rosa 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tests/qemu-iotests/common.rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 44bee16a5e..70ca65b49b 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -170,7 +170,7 @@ if [ ! -e "$TEST_DIR" ]; then
>  fi
>  
>  if [ ! -d "$TEST_DIR" ]; then
> -echo "common.config: Error: \$TEST_DIR ($TEST_DIR) is not a directory"
> +echo "common.rc: Error: \$TEST_DIR ($TEST_DIR) is not a directory"
>  exit 1
>  fi
>  
> @@ -179,7 +179,7 @@ if [ -z "$REMOTE_TEST_DIR" ]; then
>  fi
>  
>  if [ ! -d "$SAMPLE_IMG_DIR" ]; then
> -echo "common.config: Error: \$SAMPLE_IMG_DIR ($SAMPLE_IMG_DIR) is not a 
> directory"
> +echo "common.rc: Error: \$SAMPLE_IMG_DIR ($SAMPLE_IMG_DIR) is not a 
> directory"
>  exit 1
>  fi
>  
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 08/10] scripts/decodetree.py: fix reference to attributes

[Philippe Mathieu-Daudé]

On 04/10/2018 18:18, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  scripts/decodetree.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> index 457cffea90..37c76b5507 100755
> --- a/scripts/decodetree.py
> +++ b/scripts/decodetree.py
> @@ -298,7 +298,7 @@ class Field:
>  s = 's'
>  else:
>  s = ''
> -return str(pos) + ':' + s + str(len)
> +return str(self.pos) + ':' + s + str(self.len)
>  
>  def str_extract(self):
>  if self.sign:
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 07/10] scripts/decodetree.py: remove unused imports

[Philippe Mathieu-Daudé]

On 04/10/2018 18:18, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  scripts/decodetree.py | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> index 277f9a9bba..457cffea90 100755
> --- a/scripts/decodetree.py
> +++ b/scripts/decodetree.py
> @@ -149,12 +149,10 @@
>  #   trans_addl_i(ctx, &arg_opi, insn)
>  #
>  
> -import io
>  import os
>  import re
>  import sys
>  import getopt
> -import pdb
>  
>  insnwidth = 32
>  insnmask = 0x
> 

[Qemu-block] [PATCH] tests: Disable test-bdrv-drain

[Peter Maydell]

The test-bdrv-drain test fails at least 50% of the time
on my OS build system. Disable the test until we can figure
out what's going on, as this makes pull request processing
very difficult.

Signed-off-by: Peter Maydell 
---
 tests/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 175d013f4af..4aae761e769 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -91,7 +91,7 @@ gcov-files-test-thread-pool-y = thread-pool.c
 gcov-files-test-hbitmap-y = util/hbitmap.c
 check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
-check-unit-y += tests/test-bdrv-drain$(EXESUF)
+#check-unit-y += tests/test-bdrv-drain$(EXESUF)
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
 check-unit-y += tests/test-block-backend$(EXESUF)
-- 
2.19.0

Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain

[Peter Maydell]

On 5 October 2018 at 15:38, Peter Maydell  wrote:
> The test-bdrv-drain test fails at least 50% of the time
> on my OS build system. Disable the test until we can figure

This is a typo: I meant "OSX build system".

> out what's going on, as this makes pull request processing
> very difficult.
>
> Signed-off-by: Peter Maydell 

thanks
-- PMM

[Qemu-block] [PATCH] crypto: Fix defaults in QCryptoBlockCreateOptionsLUKS

[Alberto Garcia]

The values specified in the documentation don't match the actual
defaults set in qcrypto_block_luks_create().

Signed-off-by: Alberto Garcia 
---
 qapi/crypto.json | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index a51b434412..b2a4cff683 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -181,11 +181,11 @@
 # The options that apply to LUKS encryption format initialization
 #
 # @cipher-alg: the cipher algorithm for data encryption
-#  Currently defaults to 'aes'.
+#  Currently defaults to 'aes-256'.
 # @cipher-mode: the cipher mode for data encryption
-#   Currently defaults to 'cbc'
+#   Currently defaults to 'xts'
 # @ivgen-alg: the initialization vector generator
-# Currently defaults to 'essiv'
+# Currently defaults to 'plain64'
 # @ivgen-hash-alg: the initialization vector generator hash
 #  Currently defaults to 'sha256'
 # @hash-alg: the master key hash algorithm
-- 
2.11.0

Re: [Qemu-block] [PATCH] crypto: Fix defaults in QCryptoBlockCreateOptionsLUKS

[Daniel P . Berrangé]

On Fri, Oct 05, 2018 at 04:52:02PM +0200, Alberto Garcia wrote:
> The values specified in the documentation don't match the actual
> defaults set in qcrypto_block_luks_create().
> 
> Signed-off-by: Alberto Garcia 
> ---
>  qapi/crypto.json | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

05.10.2018 00:19, Vladimir Sementsov-Ogievskiy wrote:
On 10/04/2018 05:52 PM, Kevin Wolf wrote:
Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
04.10.2018 15:44, Kevin Wolf wrote:
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Fleecing-hook filter does copy-before-write operation. It should be
inserted above active disk and has a target node for CBW, like the
following:

  +---+
  | Guest |
  +---+---+
  |r,w
  v
  +---+---+  target   +---+
  | Fleecing hook |-->| target(qcow2) |
  +---+---+   CBW +---+---+
  |   |
backing |r,w|
  v   |
  +---+-+  backing|
  | Active disk |<+
  +-+r

Target's backing may point to active disk (should be set up
separately), which gives fleecing-scheme.

Signed-off-by: Vladimir Sementsov-Ogievskiy 

This lacks an explanation why we need a specialised fleecing hook driver
rather than just a generic bdrv_backup_top block driver in analogy to
what commit and mirror are already doing.

In fact, if I'm reading the last patch of the series right, backup
doesn't even restrict the use of the fleecing-hook driver to actual
fleecing scenarios.

Maybe what doesn't feel right to me is just that it's a misnomer, and if
you rename it into bdrv_backup_top (and make it internal to the block
job), it is very close to what I actually have in mind?

Kevin
Hm.
1. assume we move to internal bdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of the
bdrv_backup_top node
I think you mean sync=none?

Yes, this is true. There is no actual background job taking place there,
so the job infrastructure doesn't add much. As you say, it's just
inserting the node at the start and dropping it again at the end.

3. looks interesting to get rid of empty (doing nothing) job and use
bdrv_backup_top directly.
We could directly make the filter node available for the user, like this
series does. Should we do that? I'm not sure, but I'm not necessarily
opposed either.

But looking at the big picture, I have some more thoughts on this:

1. Is backup with sync=none only useful for fleecing? My understanding
was that "fleecing" specifically means a setup where the target of
the backup node is an overlay of the active layer of the guest
device.

I can imagine other use cases that would use sync=none (e.g. if you
don't access arbitrary blocks like from the NBD server in the
fleecing setup, but directly write to a backup file that can be
commited back later to revert things).

So I think 'fleecing-hook' is too narrow as a name. Maybe just
'backup' would be better?

may be copy-before-write?


2. mirror has a sync=none mode, too. And like backup, it doesn't
actually have any background job running then (at least in active
mirror mode), but only changes the graph at the end of the job.
Some consistency would be nice there, so is the goal to eventually
let the user create filter nodes for all jobs that don't have a
real background job?

3. We have been thinking about unifying backup, commit and mirror
into a single copy block job because they are doing quite similar
things. Of course, there are differences whether the old data or the
new data should be copied on a write, and which graph changes to make
at the end of the job, but many of the other differences are actually
features that would make sense in all of them, but are only
implemented in one job driver.

Maybe having a single 'copy' filter driver that provides options to
select backup-like behaviour or mirror-like behaviour, and that can
then internally be used by all three block jobs would be an
interesting first step towards this?


Isn't it a question about having several simple things against one complicated?)
All these jobs are similar only in the fact that they are copying blocks from 
one point to another.. So, instead of creating one big job with a lot of 
options, we can separate copying code to some kind of internal copying api, to 
then share it between jobs (and qemi-img convert). Didn't you considered this 
way? Intuitively, I'm not a fan of idea to create one job, but I don't insist. 
Of course, we can create one job carefully split the code to different objects 
files, with (again) separate copying api, shared with qemu-img, so, difference 
between one-job vs several-jobs will be mostly in qapi/ , not in the real 
code...


We can start with supporting only what backup needs, but design
everything with the idea that mirror and commit could use it, too.

I honestly feel that at first this wouldn't be very different from what
you have, so with a few renames and cleanups we might be good. But it
would give us a desig

Re: [Qemu-block] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Kevin Wolf]

Hi Vladimir,

can you please check your mailer settings? The plain text version of the
emails is hardly legible because it mixes quotes text and replies. I
had to manually open the HTML part to figure out what you really wrote.

Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hmm, how to share children?
> 
> backup job has two source BdrvChild'ren - child_job and child_root of
> job blk and two target BdrvChild'ren - again, child_job and
> child_root.
> 
> backup_top has source child - child_backing and second - child_file
> (named "target")..

Right, these are six BdrvChild instances in total. I think we can ignore
the child_job ones, they are internal to the block job infrastructure,
so we have four of them left.

> Which BdrvChild'ren you suggest to remove? They are all different.

Now that you introduced backup_top, I think we don't need any
BlockBackends any more. So I suggest to remove the child_root ones and
to do all I/O through the child_backing and child_file ones of
backup_top.

> I don't know, why job needs both unnamed blk's and child_job's, and I
> don't know is it necessary for backup to use blk's not BdrvChild'ren..

I think we had a case recently where it turned out that it is strictly
speaking even wrong for jobs to use BlockBackends in a function that
intercepts a request on the BDS level (like the copy-before-write of
backup).

So getting rid of the BlockBackends isn't only okay, but actually a good
thing by itself.

> And with internal way in none-mode we'll have two unused blk's  and
> four unused BdrvChild'ren.. Or we want to rewrite backup to use
> BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So
> I'm lost. What did you mean?

child_job isn't actually unused, even though you never use them to make
requests. The child_job BdrvChild is important because of the
BdrvChildRole callbacks it provides to the block job infrastructure.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain

[Kevin Wolf]

Am 05.10.2018 um 16:41 hat Peter Maydell geschrieben:
> On 5 October 2018 at 15:38, Peter Maydell  wrote:
> > The test-bdrv-drain test fails at least 50% of the time
> > on my OS build system. Disable the test until we can figure
> 
> This is a typo: I meant "OSX build system".
> 
> > out what's going on, as this makes pull request processing
> > very difficult.
> >
> > Signed-off-by: Peter Maydell 

Can we disable it conditionally only on OS X instead?

I'd hate to lose this test case, and without OS X I can't really do
anything to fix the problem (or to even find out if it's a test case
problem or a bug that the test case reveals).

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain

[Peter Maydell]

On 5 October 2018 at 17:17, Kevin Wolf  wrote:
> Am 05.10.2018 um 16:41 hat Peter Maydell geschrieben:
>> On 5 October 2018 at 15:38, Peter Maydell  wrote:
>> > The test-bdrv-drain test fails at least 50% of the time
>> > on my OS build system. Disable the test until we can figure
>>
>> This is a typo: I meant "OSX build system".
>>
>> > out what's going on, as this makes pull request processing
>> > very difficult.
>> >
>> > Signed-off-by: Peter Maydell 
>
> Can we disable it conditionally only on OS X instead?
>
> I'd hate to lose this test case, and without OS X I can't really do
> anything to fix the problem (or to even find out if it's a test case
> problem or a bug that the test case reveals).

If we disable it for OSX only then nobody has any incentive
to investigate and fix it...

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Eric Blake]

On 10/5/18 11:40 AM, Vladimir Sementsov-Ogievskiy wrote:

05.10.2018 18:52, Kevin Wolf wrote:

Hi Vladimir,

can you please check your mailer settings? The plain text version of the
emails is hardly legible because it mixes quotes text and replies. I
had to manually open the HTML part to figure out what you really wrote.


I've sent it from other thunderbird instance from home, I hope
thunderbird at work (where I'm composing now) is ok..


Comparing the two:

Home:
Message-ID: <46e224bd-8c1f-4565-944e-52440e85e...@virtuozzo.com>
...
Content-Type: multipart/alternative;
boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"

Work:
Message-ID: <05adf79a-4ae1-0ba1-aa7f-7696aa043...@virtuozzo.com>
...
Content-Type: text/plain; charset="utf-8"
Content-ID: 
Content-Transfer-Encoding: base64

So, the difference is that at home, you haven't told thunderbird to send 
plain-text only emails to specific recipients (setting up the list as 
one of those recipients that wants plain-text only), and something else 
in your local configurations then results in a multipart email where the 
html portion looks fine but the plain-text portion has horrendous 
quoting. But at work, you are configured for plain-text-only output, 
html is not even available, and the quoting is decent from the start.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

05.10.2018 18:52, Kevin Wolf wrote:
> Hi Vladimir,
>
> can you please check your mailer settings? The plain text version of the
> emails is hardly legible because it mixes quotes text and replies. I
> had to manually open the HTML part to figure out what you really wrote.

I've sent it from other thunderbird instance from home, I hope 
thunderbird at work (where I'm composing now) is ok..

>
> Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hmm, how to share children?
>>
>> backup job has two source BdrvChild'ren - child_job and child_root of
>> job blk and two target BdrvChild'ren - again, child_job and
>> child_root.
>>
>> backup_top has source child - child_backing and second - child_file
>> (named "target")..
> Right, these are six BdrvChild instances in total. I think we can ignore
> the child_job ones, they are internal to the block job infrastructure,
> so we have four of them left.
>
>> Which BdrvChild'ren you suggest to remove? They are all different.
> Now that you introduced backup_top, I think we don't need any
> BlockBackends any more. So I suggest to remove the child_root ones and
> to do all I/O through the child_backing and child_file ones of
> backup_top.
>
>> I don't know, why job needs both unnamed blk's and child_job's, and I
>> don't know is it necessary for backup to use blk's not BdrvChild'ren..
> I think we had a case recently where it turned out that it is strictly
> speaking even wrong for jobs to use BlockBackends in a function that
> intercepts a request on the BDS level (like the copy-before-write of
> backup).
>
> So getting rid of the BlockBackends isn't only okay, but actually a good
> thing by itself.
>
>> And with internal way in none-mode we'll have two unused blk's  and
>> four unused BdrvChild'ren.. Or we want to rewrite backup to use
>> BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So
>> I'm lost. What did you mean?
> child_job isn't actually unused, even though you never use them to make
> requests. The child_job BdrvChild is important because of the
> BdrvChildRole callbacks it provides to the block job infrastructure.
>
> Kevin

Ok, understand, thank you for the explanation!

-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qemu-iotests: make 218 executable

[Cleber Rosa]

On 10/5/18 9:36 AM, Philippe Mathieu-Daudé wrote:
> On 04/10/2018 18:18, Cleber Rosa wrote:
>> Commit 990dc39c made all tests executable at the time, but 218 came in
>> later, and missing those permissions.
>>
>> Signed-off-by: Cleber Rosa 
>> ---
>>  tests/qemu-iotests/218 | 0
>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>  mode change 100644 => 100755 tests/qemu-iotests/218
>>
>> diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
>> old mode 100644
>> new mode 100755
> 
> We could add a git pre-commit hook grep'ing for "^#!\
> *(/usr/bin/env|/bin/(b?a)?sh)" in the 1st line and then checking the
> file perms.
> 

Good idea.  Probably better to send that as a different patch, right?

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qemu-iotests: make 218 executable

[Eric Blake]

On 10/5/18 12:49 PM, Cleber Rosa wrote:



We could add a git pre-commit hook grep'ing for "^#!\
*(/usr/bin/env|/bin/(b?a)?sh)" in the 1st line and then checking the
file perms.



Good idea.  Probably better to send that as a different patch, right?


Yes, separate patch. For that matter, "^#!/" is probably a sufficient 
pattern to catch any file that intends to be run via an interpreter, and 
therefore wants to be executable, rather than limiting to just env and 
specific shells.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain

[Kevin Wolf]

Am 05.10.2018 um 18:54 hat Peter Maydell geschrieben:
> On 5 October 2018 at 17:17, Kevin Wolf  wrote:
> > Am 05.10.2018 um 16:41 hat Peter Maydell geschrieben:
> >> On 5 October 2018 at 15:38, Peter Maydell  wrote:
> >> > The test-bdrv-drain test fails at least 50% of the time
> >> > on my OS build system. Disable the test until we can figure
> >>
> >> This is a typo: I meant "OSX build system".
> >>
> >> > out what's going on, as this makes pull request processing
> >> > very difficult.
> >> >
> >> > Signed-off-by: Peter Maydell 
> >
> > Can we disable it conditionally only on OS X instead?
> >
> > I'd hate to lose this test case, and without OS X I can't really do
> > anything to fix the problem (or to even find out if it's a test case
> > problem or a bug that the test case reveals).
> 
> If we disable it for OSX only then nobody has any incentive
> to investigate and fix it...

And if we disable it wholesale, then nobody has any incentive to fix any
bug that the test case could have uncovered.

Look, if this were on BSD or something, I'd even setup a BSD VM and try
to investigate. With OS X, that's not an option. If OS X users care
about the bug, they need to fix it. If you want to give them an
incentive, then the test case needs to stay enabled. If they don't care,
we can disable the test case for OS X (and leave QEMU broken if it's a
real bug, but eventually someone will certainly report a bug in a real
life scenario in that case).

Anyway, killing tests for Linux users because they can't fix OS X bugs
doesn't sound like a very useful policy to me.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qemu-iotests: make 218 executable

[Eric Blake]

On 10/5/18 1:02 PM, Eric Blake wrote:

On 10/5/18 12:49 PM, Cleber Rosa wrote:



We could add a git pre-commit hook grep'ing for "^#!\
*(/usr/bin/env|/bin/(b?a)?sh)" in the 1st line and then checking the
file perms.



Good idea.  Probably better to send that as a different patch, right?


Yes, separate patch. For that matter, "^#!/" is probably a sufficient 
pattern to catch any file that intends to be run via an interpreter, and 
therefore wants to be executable, rather than limiting to just env and 
specific shells.


Also, many maintainers already have scripts/checkpatch.pl as a git 
pre-commit hook, and putting the check in checkpatch.pl means that 
patchew would also flag the issue.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

Thank you, hope that's fixed now)

On 10/05/2018 07:47 PM, Eric Blake wrote:
> On 10/5/18 11:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 05.10.2018 18:52, Kevin Wolf wrote:
>>> Hi Vladimir,
>>>
>>> can you please check your mailer settings? The plain text version of the
>>> emails is hardly legible because it mixes quotes text and replies. I
>>> had to manually open the HTML part to figure out what you really wrote.
>>
>> I've sent it from other thunderbird instance from home, I hope
>> thunderbird at work (where I'm composing now) is ok..
> 
> Comparing the two:
> 
> Home:
> Message-ID: <46e224bd-8c1f-4565-944e-52440e85e...@virtuozzo.com>
> ...
> Content-Type: multipart/alternative;
>  boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"
> 
> Work:
> Message-ID: <05adf79a-4ae1-0ba1-aa7f-7696aa043...@virtuozzo.com>
> ...
> Content-Type: text/plain; charset="utf-8"
> Content-ID: 
> Content-Transfer-Encoding: base64
> 
> So, the difference is that at home, you haven't told thunderbird to send 
> plain-text only emails to specific recipients (setting up the list as 
> one of those recipients that wants plain-text only), and something else 
> in your local configurations then results in a multipart email where the 
> html portion looks fine but the plain-text portion has horrendous 
> quoting. But at work, you are configured for plain-text-only output, 
> html is not even available, and the quoting is decent from the start.
> 

Re: [Qemu-block] [PATCH v3 1/3] qapi: add x-debug-query-block-graph

[Max Reitz]

On 02.10.18 15:01, Vladimir Sementsov-Ogievskiy wrote:
> 28.09.2018 19:31, Max Reitz wrote:
>> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote:
>>> Add a new command, returning block nodes (and their users) graph.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   qapi/block-core.json   |  91 +++
>>>   include/block/block.h  |   1 +
>>>   include/sysemu/block-backend.h |   2 +
>>>   block.c    | 129 +
>>>   block/block-backend.c  |   5 ++
>>>   blockdev.c |   5 ++
>>>   6 files changed, 233 insertions(+)
>> The design looks better (that is to say, good) to me, so I mostly have
>> technical remarks.  (Only a bit of bike-shedding this time.)
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4c7a37afdc..34cdc595d7 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1629,6 +1629,97 @@
>>>   ##
>>>   { 'command': 'query-named-block-nodes', 'returns': [
>>> 'BlockDeviceInfo' ] }
>>>   +##
>>> +# @BlockGraphNodeType:
>>> +#
>>> +# Since: 3.1
>>> +##
>>> +{ 'enum': 'BlockGraphNodeType',
>>> +  'data': [ 'blk', 'job', 'bds' ] }
>> I wouldn't use abbreviations here, but the full names.  At least they
>> should be described.
> 
> Hmm do you have a suggestion? Do you mean something like
>   block-backend
>   block-job
>   block-driver-state
> ?

That's what I meant, yes; though I'm not sure I was right.

"block-backend" sounds good; "job" is up to you since it isn't
necessarily an abbreviation.

About BDSs...  I guess I'll leave it up to you, too.  block-driver-state
doesn't tell people more than just "bds", so it isn't much more useful.
 Usually I'd call it "node" because that's what we like to call it in
docs.  Unfortunately, that won't quite work here, because in this case,
all of these things are nodes.

"block-node" would be the best I can come up with.  But I'm not sure
whether it's any better than "bds", so I'll just leave it up to you.

Documentation would still be nice, but I don't even know how you'd
describe @bds.  "BlockDriverState"?  Technically it'd be "Node of the
block graph", but, er, well.  That's a bad description for an element of
the "BlockGraphNodeType" enum.

So what I'm saying is "block-backend" would be nice, the rest is up to you.

>> (Though with x-debug-, it doesn't matter too much.)
>>
>>> +
>>> +##
>>> +# @BlockGraphNode:
>>> +#
>> I think a description on at least what the name means for each type
>> would be useful; and that @id is generated just for this request and not
>> some significant value in the block layer.
> 
> let me compose here, before sending next version:
> 
> @id: Block graph node identifier. This @id is generated only for
> x-debug-query-block-graph and don't relate to any other identifiers in

s/don't/does not/

> Qemu.
> @type: Type of graph node. Can be one of block-backend, block-job or
> block-driver-state.
> @name: Human readable name of the node. Corresponds to node-name for
> block-driver-state nodes, and not guaranteed to be unique in the whole

I'd prefer s/, and/; is/ (or s/, and/. Is/)

> graph (with block-jobs and block-backends)

Sounds good to me overall.

[...]

>>> +}
>>> +
>>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
>>> +{
>>> +    uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
>> I'd prefer a cast to uintptr_t.  Otherwise the compiler may warn that
>> you cast a pointer to an integer of different size (with -m32).
>>
>> Storing it in a uint64_t (with an implicit cast then) is OK, though.
>> But maybe you do want to store it in a uintptr_t.  The only reason not
>> to is because it's a uint64_t in the QAPI schema, but I think it'd be a
>> bit cleaner to work with uintptr_t internally and then emit it as
>> uint64_t (because that's definitely safe).  It's just a bit more honest
>> because this way it's clear that with -m32, we cannot even represent IDs
>> larger than 32 bit (which doesn't matter).
> 
> ok. then, why not use just "void *" ?

Because you want to store integer IDs and not pointers.

>>> +
>>> +    if (ret > 0) {
>> Just for style I'd prefer != over >.  That makes it more clear that this
>> is a NULL check and not a check for errors (represented as negative
>> integers, even though @ret is unsigned).
> 
> hm and it will be more clear with pointer type...
> 
>>
>>> +    return ret;
>>> +    }
>>> +
>>> +    ret = g_hash_table_size(gr->hash) + 1;
>> Maybe add a comment that you add 1 because 0 (NULL) is reserved for
>> non-entries?  (Yes, it's clear, or I wouldn't have figured it out, but
>> I'd still find it nice.)
> 
> hmm, I don't remember why is it reserved, looks like it doesn't matter..
> But it may be more native to count graph nodes from 1, not from 0.

Because you only have a single function for adding entries to and
querying entries from gr->hash.  So it needs to distinguish whether a
node already has

[Qemu-block] [PATCH v11 03/31] block: Skip implicit nodes for filename info

[Max Reitz]

bdrv_refresh_filename() should simply skip all implicit nodes.  They are
supposed to be invisible to the user, so they should not appear in
filename information.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block.c b/block.c
index fd5e18081a..4bbb278659 100644
--- a/block.c
+++ b/block.c
@@ -5237,6 +5237,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 bdrv_refresh_filename(child->bs);
 }
 
+if (bs->implicit) {
+/* For implicit nodes, just copy everything from the single child */
+child = QLIST_FIRST(&bs->children);
+assert(QLIST_NEXT(child, next) == NULL);
+
+pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+child->bs->exact_filename);
+pstrcpy(bs->filename, sizeof(bs->filename), child->bs->filename);
+
+bs->full_open_options = qobject_ref(child->bs->full_open_options);
+
+return;
+}
+
 if (drv->bdrv_refresh_filename) {
 /* Obsolete information is of no use here, so drop the old file name
  * information before refreshing it */
-- 
2.17.1

[Qemu-block] [PATCH v11 02/31] block: Use children list in bdrv_refresh_filename

[Max Reitz]

bdrv_refresh_filename() should invoke itself recursively on all
children, not just on file.

With that change, we can remove the manual invocations in blkverify,
quorum, commit, mirror, and blklogwrites.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block.c  | 9 +
 block/blklogwrites.c | 3 ---
 block/blkverify.c| 3 ---
 block/commit.c   | 1 -
 block/mirror.c   | 1 -
 block/quorum.c   | 1 -
 6 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 8a7bc9308a..fd5e18081a 100644
--- a/block.c
+++ b/block.c
@@ -5224,16 +5224,17 @@ static bool append_open_options(QDict *d, 
BlockDriverState *bs)
 void bdrv_refresh_filename(BlockDriverState *bs)
 {
 BlockDriver *drv = bs->drv;
+BdrvChild *child;
 QDict *opts;
 
 if (!drv) {
 return;
 }
 
-/* This BDS's file name will most probably depend on its file's name, so
- * refresh that first */
-if (bs->file) {
-bdrv_refresh_filename(bs->file->bs);
+/* This BDS's file name may depend on any of its children's file names, so
+ * refresh those first */
+QLIST_FOREACH(child, &bs->children, next) {
+bdrv_refresh_filename(child->bs);
 }
 
 if (drv->bdrv_refresh_filename) {
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index ff98cd5533..8f1b589d54 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -285,9 +285,6 @@ static void 
blk_log_writes_refresh_filename(BlockDriverState *bs,
 {
 BDRVBlkLogWritesState *s = bs->opaque;
 
-/* bs->file->bs has already been refreshed */
-bdrv_refresh_filename(s->log_file->bs);
-
 if (bs->file->bs->full_open_options
 && s->log_file->bs->full_open_options)
 {
diff --git a/block/blkverify.c b/block/blkverify.c
index 89bf4386e3..035d77b64a 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -285,9 +285,6 @@ static void blkverify_refresh_filename(BlockDriverState 
*bs, QDict *options)
 {
 BDRVBlkverifyState *s = bs->opaque;
 
-/* bs->file->bs has already been refreshed */
-bdrv_refresh_filename(s->test_file->bs);
-
 if (bs->file->bs->full_open_options
 && s->test_file->bs->full_open_options)
 {
diff --git a/block/commit.c b/block/commit.c
index a2da5740b0..dec1e89916 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -232,7 +232,6 @@ static int coroutine_fn 
bdrv_commit_top_preadv(BlockDriverState *bs,
 
 static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
 {
-bdrv_refresh_filename(bs->backing->bs);
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
 bs->backing->bs->filename);
 }
diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..9c9bea276d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1438,7 +1438,6 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
  * bdrv_set_backing_hd */
 return;
 }
-bdrv_refresh_filename(bs->backing->bs);
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
 bs->backing->bs->filename);
 }
diff --git a/block/quorum.c b/block/quorum.c
index eb526cc0f1..b14e9c6ec6 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1080,7 +1080,6 @@ static void quorum_refresh_filename(BlockDriverState *bs, 
QDict *options)
 int i;
 
 for (i = 0; i < s->num_children; i++) {
-bdrv_refresh_filename(s->children[i]->bs);
 if (!s->children[i]->bs->full_open_options) {
 return;
 }
-- 
2.17.1

[Qemu-block] [PATCH v11 01/31] block: Use bdrv_refresh_filename() to pull

[Max Reitz]

Before this patch, bdrv_refresh_filename() is used in a pushing manner:
Whenever the BDS graph is modified, the parents of the modified edges
are supposed to be updated (recursively upwards).  However, that is
nonviable, considering that we want child changes not to concern
parents.

Also, in the long run we want a pull model anyway: Here, we would have a
bdrv_filename() function which returns a BDS's filename, freshly
constructed.

This patch is an intermediate step.  It adds bdrv_refresh_filename()
calls before every place a BDS.filename value is used.  The only
exceptions are protocol drivers that use their own filename, which
clearly would not profit from refreshing that filename before.

Also, bdrv_get_encrypted_filename() is removed along the way (as a user
of BDS.filename), since it is completely unused.

In turn, all of the calls to bdrv_refresh_filename() before this patch
are removed, because we no longer have to call this function on graph
changes.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 include/block/block.h |  1 -
 block.c   | 31 +++
 block/qapi.c  |  4 
 block/raw-format.c|  1 +
 block/replication.c   |  2 --
 block/vhdx-log.c  |  1 +
 block/vmdk.c  |  6 ++
 block/vpc.c   |  4 +++-
 blockdev.c|  8 
 9 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..47a9448d44 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -480,7 +480,6 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
 int64_t *cluster_offset,
 int64_t *cluster_bytes);
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
diff --git a/block.c b/block.c
index 0d6e5f1a76..8a7bc9308a 100644
--- a/block.c
+++ b/block.c
@@ -304,8 +304,11 @@ 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_refresh_filename(bs);
+
+backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
 bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
  dest, sz, errp);
 }
@@ -984,6 +987,8 @@ static void bdrv_backing_attach(BdrvChild *c)
"node is used as backing hd of '%s'",
bdrv_get_device_or_node_name(parent));
 
+bdrv_refresh_filename(backing_hd);
+
 parent->open_flags &= ~BDRV_O_NO_BACKING;
 pstrcpy(parent->backing_file, sizeof(parent->backing_file),
 backing_hd->filename);
@@ -1385,6 +1390,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 }
 
 if (file != NULL) {
+bdrv_refresh_filename(blk_bs(file));
 filename = blk_bs(file)->filename;
 } else {
 /*
@@ -2254,8 +2260,6 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 bdrv_unref(backing_hd);
 }
 
-bdrv_refresh_filename(bs);
-
 out:
 bdrv_refresh_limits(bs, NULL);
 }
@@ -2782,8 +2786,6 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 g_free(child_key_dot);
 }
 
-bdrv_refresh_filename(bs);
-
 /* Check if any unknown options were used */
 if (qdict_size(options) != 0) {
 const QDictEntry *entry = qdict_first(options);
@@ -3234,6 +3236,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 if (local_err != NULL) {
 error_propagate(errp, local_err);
 } else {
+bdrv_refresh_filename(reopen_state->bs);
 error_setg(errp, "failed while preparing to reopen image '%s'",
reopen_state->bs->filename);
 }
@@ -3806,7 +3809,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
BlockDriverState *base,
 /* success - we can delete the intermediate states, and link top->base */
 /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once
  * we've figured out how they should work. */
-backing_file_str = backing_file_str ? backing_file_str : base->filename;
+if (!backing_file_str) {
+bdrv_refresh_filename(base);
+backing_file_str = base->filename;
+}
 
 QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
 /* Check whether we are allowed to switch c from top to base */
@@ -4202,16 +4208,6 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState 
*bs)
 return bs->supported_zer

[Qemu-block] [PATCH v11 05/31] block: Respect backing bs in bdrv_refresh_filename

[Max Reitz]

Basically, bdrv_refresh_filename() should respect all children of a
BlockDriverState. However, generally those children are driver-specific,
so this function cannot handle the general case. On the other hand,
there are only few drivers which use other children than @file and
@backing (that being vmdk, quorum, and blkverify).

Most block drivers only use @file and/or @backing (if they use any
children at all). Both can be implemented directly in
bdrv_refresh_filename.

The user overriding the file's filename is already handled, however, the
user overriding the backing file is not. If this is done, opening the
BDS with the plain filename of its file will not be correct, so we may
not set bs->exact_filename in that case.

iotest 051 contains test cases for overriding the backing file, and so
its output changes with this patch applied.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c   | 38 ++-
 tests/qemu-iotests/051.out|  8 
 tests/qemu-iotests/051.pc.out |  8 
 3 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index e158c4b4bb..b9afdab601 100644
--- a/block.c
+++ b/block.c
@@ -5228,6 +5228,21 @@ static bool append_open_options(QDict *d, 
BlockDriverState *bs)
 return found_any;
 }
 
+/* Note: This function may return false positives; it may return true
+ * even if opening the backing file specified by bs's image header
+ * would result in exactly bs->backing. */
+static bool bdrv_backing_overridden(BlockDriverState *bs)
+{
+if (bs->backing) {
+return strcmp(bs->auto_backing_file,
+  bs->backing->bs->filename);
+} else {
+/* No backing BDS, so if the image header reports any backing
+ * file, it must have been suppressed */
+return bs->auto_backing_file[0] != '\0';
+}
+}
+
 /* Updates the following BDS fields:
  *  - exact_filename: A filename which may be used for opening a block device
  *which (mostly) equals the given BDS (even without any
@@ -5245,6 +5260,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 BlockDriver *drv = bs->drv;
 BdrvChild *child;
 QDict *opts;
+bool backing_overridden;
 
 if (!drv) {
 return;
@@ -5270,6 +5286,16 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 return;
 }
 
+backing_overridden = bdrv_backing_overridden(bs);
+
+if (bs->open_flags & BDRV_O_NO_IO) {
+/* Without I/O, the backing file does not change anything.
+ * Therefore, in such a case (primarily qemu-img), we can
+ * pretend the backing file has not been overridden even if
+ * it technically has been. */
+backing_overridden = false;
+}
+
 if (drv->bdrv_refresh_filename) {
 /* Obsolete information is of no use here, so drop the old file name
  * information before refreshing it */
@@ -5295,6 +5321,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 
 opts = qdict_new();
 has_open_options = append_open_options(opts, bs);
+has_open_options |= backing_overridden;
 
 /* If no specific options have been given for this BDS, the filename of
  * the underlying file should suffice for this one as well */
@@ -5306,11 +5333,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
  * file BDS. The full options QDict of that file BDS should somehow
  * contain a representation of the filename, therefore the following
  * suffices without querying the (exact_)filename of this BDS. */
-if (bs->file->bs->full_open_options) {
+if (bs->file->bs->full_open_options &&
+(!bs->backing || bs->backing->bs->full_open_options))
+{
 qdict_put_str(opts, "driver", drv->format_name);
 qdict_put(opts, "file",
   qobject_ref(bs->file->bs->full_open_options));
 
+if (bs->backing) {
+qdict_put(opts, "backing",
+  qobject_ref(bs->backing->bs->full_open_options));
+} else if (backing_overridden) {
+qdict_put_null(opts, "backing");
+}
+
 bs->full_open_options = opts;
 } else {
 qobject_unref(opts);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 793af2ab96..b900935fbc 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -82,7 +82,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive 
file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig,if=none,id=drive0
 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": 
"file", "filename": "TEST_DIR/t.qcow2.orig"}}, "driver": "qcow2", "file": 
{"driver": "file", "filename": "T

[Qemu-block] [PATCH v11 04/31] block: Add BDS.auto_backing_file

[Max Reitz]

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 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 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 92ecbd866e..385070f373 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 4bbb278659..e158c4b4bb 100644
--- a/block.c
+++ b/block.c
@@ -2281,6 +2281,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;
@@ -2316,6 +2317,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
+

[Qemu-block] [PATCH v11 06/31] iotests.py: Add filter_imgfmt()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 tests/qemu-iotests/iotests.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..5c45788dac 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -246,6 +246,9 @@ def filter_img_info(output, filename):
 lines.append(line)
 return '\n'.join(lines)
 
+def filter_imgfmt(msg):
+return msg.replace(imgfmt, 'IMGFMT')
+
 def log(msg, filters=[]):
 for flt in filters:
 msg = flt(msg)
-- 
2.17.1

[Qemu-block] [PATCH v11 00/31] block: Fix some filename generation issues

[Max Reitz]

Once more, I’ll spare both me and you another iteration of the cover
letter, so see here:

http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html

(Although this series no longer includes a @base-directory option.)

In regards to the last version, the biggest change is that I dropped
backing_overridden and instead try to compare the filename from the
image header with the filename of the actual backing BDS to find out
whether the backing file has been overridden.

In order that this doesn’t break whenever the header contains a slightly
unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
“nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
different from what bdrv_refresh_filename() would generate), when the
reference filename in the BDS (auto_backing_file) is used to open the
backing file, it is updated from the backing BDS's resulting filename.


Changes in v10:
- Rebased

- Patch 5: Dropped parentheses in return statement, because the one
   thing I hate even more than syntax I don't like is patchew
   complaining about my syntax
   (kept the R-b, because it's an obvious and simple change)

- Patch 7: Dropped @require_existence [Berto]

- Patch 24: Rebase conflict in code removed; Berto changed bs->options
not to include child options, so append_open_options() no
longer had to actively exclude those.  Now the new code
works the other way around (including specific strong
options instead of excluding anything that seems generic and
concerns child nodes), so the new code is not affected.  But
the old code that's removed is changed, so there's a rebase
conflict here that was simple to resolve.

- Patch 31: Wrongly had some code commented out in the last version.
Sorry.  [Berto]


Backport-diff:

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/31:[] [-C] 'block: Use bdrv_refresh_filename() to pull'
002/31:[] [--] 'block: Use children list in bdrv_refresh_filename'
003/31:[] [--] 'block: Skip implicit nodes for filename info'
004/31:[] [--] 'block: Add BDS.auto_backing_file'
005/31:[0002] [FC] 'block: Respect backing bs in bdrv_refresh_filename'
006/31:[] [--] 'iotests.py: Add filter_imgfmt()'
007/31:[0007] [FC] 'iotests.py: Add node_info()'
008/31:[] [-C] 'iotests: Add test for backing file overrides'
009/31:[] [--] 'block: Make path_combine() return the path'
010/31:[] [--] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.'
011/31:[] [--] 'block: bdrv_get_full_backing_filename's ret. val.'
012/31:[] [--] 'block: Add bdrv_make_absolute_filename()'
013/31:[] [--] 'block: Fix bdrv_find_backing_image()'
014/31:[] [--] 'block: Add bdrv_dirname()'
015/31:[] [--] 'blkverify: Make bdrv_dirname() return NULL'
016/31:[] [--] 'quorum: Make bdrv_dirname() return NULL'
017/31:[] [--] 'block/nbd: Make bdrv_dirname() return NULL'
018/31:[] [--] 'block/nfs: Implement bdrv_dirname()'
019/31:[] [--] 'block: Use bdrv_dirname() for relative filenames'
020/31:[] [--] 'iotests: Add quorum case to test 110'
021/31:[] [--] 'block: Add strong_runtime_opts to BlockDriver'
022/31:[] [--] 'block: Add BlockDriver.bdrv_gather_child_options'
023/31:[] [--] 'block: Generically refresh runtime options'
024/31:[0016] [FC] 'block: Purify .bdrv_refresh_filename()'
025/31:[] [--] 'block: Do not copy exact_filename from format file'
026/31:[] [-C] 'block/nvme: Fix bdrv_refresh_filename()'
027/31:[] [--] 'block/curl: Harmonize option defaults'
028/31:[] [--] 'block/curl: Implement bdrv_refresh_filename()'
029/31:[] [--] 'block/null: Generate filename even with latency-ns'
030/31:[] [--] 'block: BDS options may lack the "driver" option'
031/31:[0030] [FC] 'iotests: Test json:{} filenames of internal BDSs'


Previous history:
v10: http://lists.nongnu.org/archive/html/qemu-block/2018-08/msg00234.html
v9: http://lists.nongnu.org/archive/html/qemu-block/2018-06/msg01291.html
v8: http://lists.nongnu.org/archive/html/qemu-block/2018-02/msg00157.html
v7: http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00831.html
v6: http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
[...]
v1: http://lists.nongnu.org/archive/html/qemu-block/2016-04/msg00740.html


Max Reitz (31):
  block: Use bdrv_refresh_filename() to pull
  block: Use children list in bdrv_refresh_filename
  block: Skip implicit nodes for filename info
  block: Add BDS.auto_backing_file
  block: Respect backing bs in bdrv_refresh_filename
  iotests.py: Add filter_imgfmt()
  iotests.py: Add node_info()
  iotests: Add test for backing file overrides
  block: Make path_combine() return the path
  block: bdrv_get

[Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()

[Max Reitz]

This function queries a node; since we cannot do that right now, it
executes query-named-block-nodes and returns the matching node's object.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5c45788dac..604f200600 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine):
 else:
 iotests.log(ev)
 
+def node_info(self, node_name):
+nodes = self.qmp('query-named-block-nodes')
+for x in nodes['return']:
+if x['node-name'] == node_name:
+return x
+return None
+
 
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 
-- 
2.17.1

[Qemu-block] [PATCH v11 11/31] block: bdrv_get_full_backing_filename's ret. val.

[Max Reitz]

Make bdrv_get_full_backing_filename() return an allocated string instead
of placing the result in a caller-provided buffer.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 include/block/block.h |  3 +--
 block.c   | 48 +++
 block/qapi.c  | 12 ++-
 3 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index bb238488c4..ac149abca4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -482,8 +482,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
 
 void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
-void bdrv_get_full_backing_filename(BlockDriverState *bs,
-char *dest, size_t sz, Error **errp);
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
 char *bdrv_get_full_backing_filename_from_filename(const char *backed,
const char *backing,
Error **errp);
diff --git a/block.c b/block.c
index d31c759091..3b5896083c 100644
--- a/block.c
+++ b/block.c
@@ -319,28 +319,16 @@ char *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 *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
 {
 char *backed;
-char *full_name;
-Error *local_error = NULL;
 
 bdrv_refresh_filename(bs);
 
 backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-
-full_name = bdrv_get_full_backing_filename_from_filename(backed,
- bs->backing_file,
- &local_error);
-if (full_name) {
-pstrcpy(dest, sz, full_name);
-g_free(full_name);
-} else if (local_error) {
-error_propagate(errp, local_error);
-} else if (sz > 0) {
-*dest = '\0';
-}
+return bdrv_get_full_backing_filename_from_filename(backed,
+bs->backing_file,
+errp);
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -2307,7 +2295,7 @@ out:
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp)
 {
-char *backing_filename = g_malloc0(PATH_MAX);
+char *backing_filename = NULL;
 char *bdref_key_dot;
 const char *reference = NULL;
 int ret = 0;
@@ -2342,7 +2330,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*parent_options,
  */
 reference = qdict_get_try_str(parent_options, bdref_key);
 if (reference || qdict_haskey(options, "file.filename")) {
-backing_filename[0] = '\0';
+/* keep backing_filename NULL */
 } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
 qobject_unref(options);
 goto free_exit;
@@ -2357,8 +2345,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*parent_options,
 implicit_backing = !strcmp(bs->auto_backing_file, 
bs->backing_file);
 }
 
-bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX,
-   &local_err);
+backing_filename = bdrv_get_full_backing_filename(bs, &local_err);
 if (local_err) {
 ret = -EINVAL;
 error_propagate(errp, local_err);
@@ -2379,9 +2366,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*parent_options,
 qdict_put_str(options, "driver", bs->backing_format);
 }
 
-backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
-   reference, options, 0, bs, &child_backing,
-   errp);
+backing_hd = bdrv_open_inherit(backing_filename, reference, options, 0, bs,
+   &child_backing, errp);
 if (!backing_hd) {
 bs->open_flags |= BDRV_O_NO_BACKING;
 error_prepend(errp, "Could not open backing file: ");
@@ -4364,7 +4350,6 @@ BlockDriverState 
*bdrv_find_backing_image(BlockDriverState *bs,
 int is_protocol = 0;
 BlockDriverState *curr_bs = NULL;
 BlockDriverState *retval = NULL;
-Error *local_error = NULL;
 
 if (!bs || !bs->drv || !backing_file) {
 return NULL;
@@ -4384,21 +4369,22 @@ BlockDriverState 
*bdrv_find_backing_image(BlockDriverState *bs,
 /* If either of the filename paths is actually a protocol, then
  * compare unmodified paths; otherwise make paths relative */
 if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+char *backing_file_full_ret;
+
   

[Qemu-block] [PATCH v11 13/31] block: Fix bdrv_find_backing_image()

[Max Reitz]

bdrv_find_backing_image() should use bdrv_get_full_backing_filename() or
bdrv_make_absolute_filename() instead of trying to do what those
functions do by itself.

path_combine_deprecated() can now be dropped, so let's do that.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 33 ++---
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index 6a346f6e5e..45acdfa2bb 100644
--- a/block.c
+++ b/block.c
@@ -201,15 +201,6 @@ char *path_combine(const char *base_path, const char 
*filename)
 return result;
 }
 
-static void path_combine_deprecated(char *dest, int dest_size,
-const char *base_path,
-const char *filename)
-{
-char *combined = path_combine(base_path, filename);
-pstrcpy(dest, dest_size, combined);
-g_free(combined);
-}
-
 /*
  * Helper function for bdrv_parse_filename() implementations to remove optional
  * protocol prefixes (especially "file:") from a filename and for putting the
@@ -4370,13 +4361,9 @@ BlockDriverState 
*bdrv_find_backing_image(BlockDriverState *bs,
 
 filename_full = g_malloc(PATH_MAX);
 backing_file_full = g_malloc(PATH_MAX);
-filename_tmp  = g_malloc(PATH_MAX);
 
 is_protocol = path_has_protocol(backing_file);
 
-/* This will recursively refresh everything in the backing chain */
-bdrv_refresh_filename(bs);
-
 for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) {
 
 /* If either of the filename paths is actually a protocol, then
@@ -4402,22 +4389,23 @@ 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_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
-backing_file);
-
-/* We are going to compare absolute pathnames */
-if (!realpath(filename_tmp, filename_full)) {
+filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
+   NULL);
+/* We are going to compare canonicalized absolute pathnames */
+if (!filename_tmp || !realpath(filename_tmp, filename_full)) {
+g_free(filename_tmp);
 continue;
 }
+g_free(filename_tmp);
 
 /* We need to make sure the backing filename we are comparing 
against
  * is relative to the current image filename (or absolute) */
-path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
-curr_bs->backing_file);
-
-if (!realpath(filename_tmp, backing_file_full)) {
+filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
+if (!filename_tmp || !realpath(filename_tmp, backing_file_full)) {
+g_free(filename_tmp);
 continue;
 }
+g_free(filename_tmp);
 
 if (strcmp(backing_file_full, filename_full) == 0) {
 retval = curr_bs->backing->bs;
@@ -4428,7 +4416,6 @@ BlockDriverState 
*bdrv_find_backing_image(BlockDriverState *bs,
 
 g_free(filename_full);
 g_free(backing_file_full);
-g_free(filename_tmp);
 return retval;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v11 08/31] iotests: Add test for backing file overrides

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 tests/qemu-iotests/228 | 235 +
 tests/qemu-iotests/228.out |  84 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 320 insertions(+)
 create mode 100755 tests/qemu-iotests/228
 create mode 100644 tests/qemu-iotests/228.out

diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
new file mode 100755
index 00..a2200efba5
--- /dev/null
+++ b/tests/qemu-iotests/228
@@ -0,0 +1,235 @@
+#!/usr/bin/env python
+#
+# Test for when a backing file is considered overridden (thus, a
+# json:{} filename is generated for the overlay) and when it is not
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# Creator/Owner: Max Reitz 
+
+import iotests
+from iotests import log, qemu_img, filter_testfiles, filter_imgfmt
+
+# Need backing file and change-backing-file support
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+def log_node_info(node):
+log('')
+
+log('bs->filename: ' + node['image']['filename'],
+filters=[filter_testfiles, filter_imgfmt])
+log('bs->backing_file: ' + node['backing_file'],
+filters=[filter_testfiles, filter_imgfmt])
+
+if 'backing-image' in node['image']:
+log('bs->backing->bs->filename: ' +
+node['image']['backing-image']['filename'],
+filters=[filter_testfiles, filter_imgfmt])
+else:
+log('bs->backing: (none)')
+
+log('')
+
+
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+# Choose a funny way to describe the backing filename
+assert qemu_img('create', '-f', iotests.imgfmt, '-b',
+'file:' + base_img_path, top_img_path) == 0
+
+vm.launch()
+
+log('--- Implicit backing file ---')
+log('')
+
+vm.qmp_log('blockdev-add',
+node_name='node0',
+driver=iotests.imgfmt,
+file={
+'driver': 'file',
+'filename': top_img_path
+},
+filters=[filter_testfiles, filter_imgfmt])
+
+# Filename should be plain, and the backing filename should not
+# contain the "file:" prefix
+log_node_info(vm.node_info('node0'))
+
+vm.qmp_log('blockdev-del', node_name='node0')
+
+log('')
+log('--- change-backing-file ---')
+log('')
+
+vm.qmp_log('blockdev-add',
+   node_name='node0',
+   driver=iotests.imgfmt,
+   file={
+   'driver': 'file',
+   'filename': top_img_path
+   },
+   filters=[filter_testfiles, filter_imgfmt])
+
+# Changing the backing file to a qemu-reported filename should
+# result in qemu accepting the corresponding BDS as the implicit
+# backing BDS (and thus not generate a json:{} filename).
+# So, first, query the backing filename.
+
+backing_filename = \
+vm.node_info('node0')['image']['backing-image']['filename']
+
+# Next, change the backing file to something different
+
+vm.qmp_log('change-backing-file',
+   image_node_name='node0',
+   device='node0',
+   backing_file='null-co://')
+
+# Now, verify that we get a json:{} filename
+# (Image header says "null-co://", actual backing file still is
+# base_img_path)
+
+log_node_info(vm.node_info('node0'))
+
+# Change it back
+# (To get header and backing file in sync)
+
+vm.qmp_log('change-backing-file',
+   image_node_name='node0',
+   device='node0',
+   backing_file=backing_filename)
+
+# And verify that we get our original results
+
+log_node_info(vm.node_info('node0'))
+
+# Finally, try a "file:" prefix.  While this is actually what we
+# originally had in the image header, qemu will not reopen the
+# backing file here, so it cannot verify that this filename
+# "resolves" to the actual backing BDS's filename and will thus
+# consider both to be different.
+# (This may be fixed in the future.)
+
+vm.qmp_log('change-backing-file',
+   image_node_n

[Qemu-block] [PATCH v11 12/31] block: Add bdrv_make_absolute_filename()

[Max Reitz]

This is a general function for making a filename that is relative to a
certain BDS absolute.

It calls bdrv_get_full_backing_filename_from_filename() for now, but
that will be changed in a follow-up patch.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 3b5896083c..6a346f6e5e 100644
--- a/block.c
+++ b/block.c
@@ -319,16 +319,29 @@ char *bdrv_get_full_backing_filename_from_filename(const 
char *backed,
 }
 }
 
-char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
+/*
+ * If @filename is empty or NULL, this function returns NULL without
+ * setting @errp.  In all other cases, NULL will only be returned with
+ * @errp set.
+ */
+static char *bdrv_make_absolute_filename(BlockDriverState *relative_to,
+ const char *filename, Error **errp)
 {
-char *backed;
+char *bs_filename;
 
-bdrv_refresh_filename(bs);
+bdrv_refresh_filename(relative_to);
+
+bs_filename = relative_to->exact_filename[0]
+  ? relative_to->exact_filename
+  : relative_to->filename;
 
-backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-return bdrv_get_full_backing_filename_from_filename(backed,
-bs->backing_file,
-errp);
+return bdrv_get_full_backing_filename_from_filename(bs_filename,
+filename ?: "", errp);
+}
+
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
+{
+return bdrv_make_absolute_filename(bs, bs->backing_file, errp);
 }
 
 void bdrv_register(BlockDriver *bdrv)
-- 
2.17.1

[Qemu-block] [PATCH v11 09/31] block: Make path_combine() return the path

[Max Reitz]

Besides being safe for arbitrary path lengths, after some follow-up
patches all callers will want a freshly allocated buffer anyway.

In the meantime, path_combine_deprecated() is added which has the same
interface as path_combine() had before this patch. All callers to that
function will be converted in follow-up patches.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Reviewed-by: Kevin Wolf 
---
 include/block/block.h |  4 +-
 block.c   | 85 ---
 block/vmdk.c  |  3 +-
 3 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 47a9448d44..e5f01ebed0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -491,9 +491,7 @@ void bdrv_get_full_backing_filename_from_filename(const 
char *backed,
 
 int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
-void path_combine(char *dest, int dest_size,
-  const char *base_path,
-  const char *filename);
+char *path_combine(const char *base_path, const char *filename);
 
 int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
diff --git a/block.c b/block.c
index b9afdab601..d083b02b58 100644
--- a/block.c
+++ b/block.c
@@ -152,53 +152,62 @@ int path_is_absolute(const char *path)
 #endif
 }
 
-/* if filename is absolute, just copy it to dest. Otherwise, build a
+/* if filename is absolute, just return its duplicate. Otherwise, build a
path to it by considering it is relative to base_path. URL are
supported. */
-void path_combine(char *dest, int dest_size,
-  const char *base_path,
-  const char *filename)
+char *path_combine(const char *base_path, const char *filename)
 {
+const char *protocol_stripped = NULL;
 const char *p, *p1;
+char *result;
 int len;
 
-if (dest_size <= 0)
-return;
 if (path_is_absolute(filename)) {
-pstrcpy(dest, dest_size, filename);
-} else {
-const char *protocol_stripped = NULL;
+return g_strdup(filename);
+}
 
-if (path_has_protocol(base_path)) {
-protocol_stripped = strchr(base_path, ':');
-if (protocol_stripped) {
-protocol_stripped++;
-}
+if (path_has_protocol(base_path)) {
+protocol_stripped = strchr(base_path, ':');
+if (protocol_stripped) {
+protocol_stripped++;
 }
-p = protocol_stripped ?: base_path;
+}
+p = protocol_stripped ?: base_path;
 
-p1 = strrchr(base_path, '/');
+p1 = strrchr(base_path, '/');
 #ifdef _WIN32
-{
-const char *p2;
-p2 = strrchr(base_path, '\\');
-if (!p1 || p2 > p1)
-p1 = p2;
+{
+const char *p2;
+p2 = strrchr(base_path, '\\');
+if (!p1 || p2 > p1) {
+p1 = p2;
 }
+}
 #endif
-if (p1)
-p1++;
-else
-p1 = base_path;
-if (p1 > p)
-p = p1;
-len = p - base_path;
-if (len > dest_size - 1)
-len = dest_size - 1;
-memcpy(dest, base_path, len);
-dest[len] = '\0';
-pstrcat(dest, dest_size, filename);
+if (p1) {
+p1++;
+} else {
+p1 = base_path;
+}
+if (p1 > p) {
+p = p1;
 }
+len = p - base_path;
+
+result = g_malloc(len + strlen(filename) + 1);
+memcpy(result, base_path, len);
+strcpy(result + len, filename);
+
+return result;
+}
+
+static void path_combine_deprecated(char *dest, int dest_size,
+const char *base_path,
+const char *filename)
+{
+char *combined = path_combine(base_path, filename);
+pstrcpy(dest, dest_size, combined);
+g_free(combined);
 }
 
 /*
@@ -297,7 +306,7 @@ void bdrv_get_full_backing_filename_from_filename(const 
char *backed,
 error_setg(errp, "Cannot use relative backing file names for '%s'",
backed);
 } else {
-path_combine(dest, sz, backed, backing);
+path_combine_deprecated(dest, sz, backed, backing);
 }
 }
 
@@ -4373,8 +4382,8 @@ 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);
+path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
+backing_file);
 
 /* We are going to compare absolute pathnames */
 if (!realpath(filename_tmp, filename_full)) {
@@ -4383,8 +4392,8 @@ BlockDriverState 
*bdrv_find_backing_image(Bl

[Qemu-block] [PATCH v11 20/31] iotests: Add quorum case to test 110

[Max Reitz]

Test 110 tests relative backing filenames for complex BDS trees.  Now
that the originally supposedly failing test passes, let us add a new
failing test: Quorum can never work automatically (without detecting
whether all child nodes have the same base directory, but that would be
rather inconsistent behavior).

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 tests/qemu-iotests/110 | 26 ++
 tests/qemu-iotests/110.out |  7 +++
 2 files changed, 33 insertions(+)

diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index ba1b3c6c7d..08976c8a61 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -30,6 +30,7 @@ status=1  # failure is the default!
 _cleanup()
 {
_cleanup_test_img
+rm -f "$TEST_IMG.copy"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -87,6 +88,31 @@ echo
 # omit the image size; it should work anyway
 _make_test_img -b "$TEST_IMG_REL.base"
 
+echo
+echo '=== Nodes without a common directory ==='
+echo
+
+cp "$TEST_IMG" "$TEST_IMG.copy"
+
+# Should inform us that the actual path of the backing file cannot be 
determined
+TEST_IMG="json:{
+'driver': '$IMGFMT',
+'file': {
+'driver': 'quorum',
+'vote-threshold': 1,
+'children': [
+{
+'driver': 'file',
+'filename': '$TEST_IMG'
+},
+{
+'driver': 'file',
+'filename': '$TEST_IMG.copy'
+}
+]
+}
+}" _img_info | _filter_img_info
+
 
 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 5370bc1d26..1d0b2475cc 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -19,4 +19,11 @@ backing file: t.IMGFMT.base (actual path: 
TEST_DIR/t.IMGFMT.base)
 === Backing name is always relative to the backed image ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=t.IMGFMT.base
+
+=== Nodes without a common directory ===
+
+image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", 
"filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": 
"TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "blkverify": false, 
"rewrite-corrupted": false, "vote-threshold": 1}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (cannot determine actual path)
 *** done
-- 
2.17.1

[Qemu-block] [PATCH v11 14/31] block: Add bdrv_dirname()

[Max Reitz]

This function may be implemented by block drivers to derive a directory
name from a BDS. Concatenating this g_free()-able string with a relative
filename must result in a valid (not necessarily existing) filename, so
this is a function that should generally be not implemented by format
drivers, because this is protocol-specific.

If a BDS's driver does not implement this function, bdrv_dirname() will
fall through to the BDS's file if it exists. If it does not, the
exact_filename field will be used to generate a directory name.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 include/block/block.h |  1 +
 include/block/block_int.h |  7 +++
 block.c   | 27 +++
 3 files changed, 35 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index ac149abca4..c6ac72e778 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -486,6 +486,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, 
Error **errp);
 char *bdrv_get_full_backing_filename_from_filename(const char *backed,
const char *backing,
Error **errp);
+char *bdrv_dirname(BlockDriverState *bs, Error **errp);
 
 int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 385070f373..f448b90ee7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -141,6 +141,13 @@ struct BlockDriver {
 
 void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
 
+/*
+ * Returns an allocated string which is the directory name of this BDS: It
+ * will be used to make relative filenames absolute by prepending this
+ * function's return value to them.
+ */
+char *(*bdrv_dirname)(BlockDriverState *bs, Error **errp);
+
 /* aio */
 BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
diff --git a/block.c b/block.c
index 45acdfa2bb..0e5edccd15 100644
--- a/block.c
+++ b/block.c
@@ -5403,6 +5403,33 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 }
 }
 
+char *bdrv_dirname(BlockDriverState *bs, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg(errp, "Node '%s' is ejected", bs->node_name);
+return NULL;
+}
+
+if (drv->bdrv_dirname) {
+return drv->bdrv_dirname(bs, errp);
+}
+
+if (bs->file) {
+return bdrv_dirname(bs->file->bs, errp);
+}
+
+bdrv_refresh_filename(bs);
+if (bs->exact_filename[0] != '\0') {
+return path_combine(bs->exact_filename, "");
+}
+
+error_setg(errp, "Cannot generate a base directory for %s nodes",
+   drv->format_name);
+return NULL;
+}
+
 /*
  * Hot add/remove a BDS's child. So the user can take a child offline when
  * it is broken and take a new child online
-- 
2.17.1

[Qemu-block] [PATCH v11 18/31] block/nfs: Implement bdrv_dirname()

[Max Reitz]

While the basic idea is obvious and could be handled by the default
bdrv_dirname() implementation, we cannot generate a directory name if
the gid or uid are set, so we have to explicitly return NULL in those
cases.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/nfs.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index eab1a2c408..19ee07c321 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -855,6 +855,20 @@ static void nfs_refresh_filename(BlockDriverState *bs, 
QDict *options)
 bs->full_open_options = opts;
 }
 
+static char *nfs_dirname(BlockDriverState *bs, Error **errp)
+{
+NFSClient *client = bs->opaque;
+
+if (client->uid || client->gid) {
+bdrv_refresh_filename(bs);
+error_setg(errp, "Cannot generate a base directory for NFS node '%s'",
+   bs->filename);
+return NULL;
+}
+
+return g_strdup_printf("nfs://%s%s/", client->server->host, client->path);
+}
+
 #ifdef LIBNFS_FEATURE_PAGECACHE
 static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs,
  Error **errp)
@@ -889,6 +903,7 @@ static BlockDriver bdrv_nfs = {
 .bdrv_detach_aio_context= nfs_detach_aio_context,
 .bdrv_attach_aio_context= nfs_attach_aio_context,
 .bdrv_refresh_filename  = nfs_refresh_filename,
+.bdrv_dirname   = nfs_dirname,
 
 #ifdef LIBNFS_FEATURE_PAGECACHE
 .bdrv_co_invalidate_cache   = nfs_co_invalidate_cache,
-- 
2.17.1

[Qemu-block] [PATCH v11 17/31] block/nbd: Make bdrv_dirname() return NULL

[Max Reitz]

The generic bdrv_dirname() implementation would be able to generate some
form of directory name for many NBD nodes, but it would be always wrong.
Therefore, we have to explicitly make it an error (until NBD has some
form of specification for export paths, if it ever will).

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/nbd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index e87699fb73..bca127c8f5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -564,6 +564,16 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
QDict *options)
 bs->full_open_options = opts;
 }
 
+static char *nbd_dirname(BlockDriverState *bs, Error **errp)
+{
+/* The generic bdrv_dirname() implementation is able to work out some
+ * directory name for NBD nodes, but that would be wrong. So far there is 
no
+ * specification for how "export paths" would work, so NBD does not have
+ * directory names. */
+error_setg(errp, "Cannot generate a base directory for NBD nodes");
+return NULL;
+}
+
 static BlockDriver bdrv_nbd = {
 .format_name= "nbd",
 .protocol_name  = "nbd",
@@ -582,6 +592,7 @@ static BlockDriver bdrv_nbd = {
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
 .bdrv_co_block_status   = nbd_client_co_block_status,
+.bdrv_dirname   = nbd_dirname,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -602,6 +613,7 @@ static BlockDriver bdrv_nbd_tcp = {
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
 .bdrv_co_block_status   = nbd_client_co_block_status,
+.bdrv_dirname   = nbd_dirname,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -622,6 +634,7 @@ static BlockDriver bdrv_nbd_unix = {
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
 .bdrv_co_block_status   = nbd_client_co_block_status,
+.bdrv_dirname   = nbd_dirname,
 };
 
 static void bdrv_nbd_init(void)
-- 
2.17.1

[Qemu-block] [PATCH v11 15/31] blkverify: Make bdrv_dirname() return NULL

[Max Reitz]

blkverify's BDSs have a file BDS, but we do not want this to be
preferred over the raw node. There is no way to decide between the two
(and not really a reason to, either), so just return NULL in blkverify's
implementation of bdrv_dirname().

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block/blkverify.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/blkverify.c b/block/blkverify.c
index 035d77b64a..3c7d4c8729 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -313,6 +313,15 @@ static void blkverify_refresh_filename(BlockDriverState 
*bs, QDict *options)
 }
 }
 
+static char *blkverify_dirname(BlockDriverState *bs, Error **errp)
+{
+/* In general, there are two BDSs with different dirnames below this one;
+ * so there is no unique dirname we could return (unless both are equal by
+ * chance). Therefore, to be consistent, just always return NULL. */
+error_setg(errp, "Cannot generate a base directory for blkverify nodes");
+return NULL;
+}
+
 static BlockDriver bdrv_blkverify = {
 .format_name  = "blkverify",
 .protocol_name= "blkverify",
@@ -324,6 +333,7 @@ static BlockDriver bdrv_blkverify = {
 .bdrv_child_perm  = bdrv_filter_default_perms,
 .bdrv_getlength   = blkverify_getlength,
 .bdrv_refresh_filename= blkverify_refresh_filename,
+.bdrv_dirname = blkverify_dirname,
 
 .bdrv_co_preadv   = blkverify_co_preadv,
 .bdrv_co_pwritev  = blkverify_co_pwritev,
-- 
2.17.1

[Qemu-block] [PATCH v11 19/31] block: Use bdrv_dirname() for relative filenames

[Max Reitz]

bdrv_get_full_backing_filename_from_filename() breaks down when it comes
to JSON filenames. Using bdrv_dirname() as the basis is better because
since we have BDS, we can descend through the BDS tree to the protocol
layer, which gives us a greater probability of finding a non-JSON name;
also, bdrv_dirname() is more correct as it allows block drivers to
override the generation of that directory name in a protocol-specific
way.

We still need to keep bdrv_get_full_backing_filename_from_filename(),
though, because it has valid callers which need it during image creation
when no BDS is available yet.

This makes a test case in qemu-iotest 110, which was supposed to fail,
work. That is actually good, but we need to change the reference output
(and the comment in 110) accordingly.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c| 20 +---
 tests/qemu-iotests/110 |  3 ++-
 tests/qemu-iotests/110.out |  2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 0e5edccd15..6b4fccecc9 100644
--- a/block.c
+++ b/block.c
@@ -318,16 +318,22 @@ char *bdrv_get_full_backing_filename_from_filename(const 
char *backed,
 static char *bdrv_make_absolute_filename(BlockDriverState *relative_to,
  const char *filename, Error **errp)
 {
-char *bs_filename;
+char *dir, *full_name;
 
-bdrv_refresh_filename(relative_to);
+if (!filename || filename[0] == '\0') {
+return NULL;
+} else if (path_has_protocol(filename) || path_is_absolute(filename)) {
+return g_strdup(filename);
+}
 
-bs_filename = relative_to->exact_filename[0]
-  ? relative_to->exact_filename
-  : relative_to->filename;
+dir = bdrv_dirname(relative_to, errp);
+if (!dir) {
+return NULL;
+}
 
-return bdrv_get_full_backing_filename_from_filename(bs_filename,
-filename ?: "", errp);
+full_name = g_strconcat(dir, filename, NULL);
+g_free(dir);
+return full_name;
 }
 
 char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index 9de7369f3a..ba1b3c6c7d 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -61,7 +61,8 @@ echo '=== Non-reconstructable filename ==='
 echo
 
 # Across blkdebug without a config file, you cannot reconstruct filenames, so
-# qemu is incapable of knowing the directory of the top image
+# qemu is incapable of knowing the directory of the top image from the filename
+# alone. However, using bdrv_dirname(), it should still work.
 TEST_IMG="json:{
 'driver': '$IMGFMT',
 'file': {
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index b3584ff87f..5370bc1d26 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -14,7 +14,7 @@ backing file: t.IMGFMT.base (actual path: 
TEST_DIR/t.IMGFMT.base)
 image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", 
"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": 
"blkdebug", "set-state.0.new_state": 42}}
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
-backing file: t.IMGFMT.base (cannot determine actual path)
+backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
 
 === Backing name is always relative to the backed image ===
 
-- 
2.17.1

[Qemu-block] [PATCH v11 10/31] block: bdrv_get_full_backing_filename_from_...'s ret. val.

[Max Reitz]

Make bdrv_get_full_backing_filename_from_filename() return an allocated
string instead of placing the result in a caller-provided buffer.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 include/block/block.h |  7 +++---
 block.c   | 53 ++-
 block/vmdk.c  |  9 
 qemu-img.c| 12 --
 4 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index e5f01ebed0..bb238488c4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -484,10 +484,9 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
 char *dest, size_t sz, Error **errp);
-void bdrv_get_full_backing_filename_from_filename(const char *backed,
-  const char *backing,
-  char *dest, size_t sz,
-  Error **errp);
+char *bdrv_get_full_backing_filename_from_filename(const char *backed,
+   const char *backing,
+   Error **errp);
 
 int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
diff --git a/block.c b/block.c
index d083b02b58..d31c759091 100644
--- a/block.c
+++ b/block.c
@@ -293,20 +293,29 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 return 0;
 }
 
-void bdrv_get_full_backing_filename_from_filename(const char *backed,
-  const char *backing,
-  char *dest, size_t sz,
-  Error **errp)
+/*
+ * If @backing is empty, this function returns NULL without setting
+ * @errp.  In all other cases, NULL will only be returned with @errp
+ * set.
+ *
+ * Therefore, a return value of NULL without @errp set means that
+ * there is no backing file; if @errp is set, there is one but its
+ * absolute filename cannot be generated.
+ */
+char *bdrv_get_full_backing_filename_from_filename(const char *backed,
+   const char *backing,
+   Error **errp)
 {
-if (backing[0] == '\0' || path_has_protocol(backing) ||
-path_is_absolute(backing))
-{
-pstrcpy(dest, sz, backing);
+if (backing[0] == '\0') {
+return NULL;
+} else if (path_has_protocol(backing) || path_is_absolute(backing)) {
+return g_strdup(backing);
 } else if (backed[0] == '\0' || strstart(backed, "json:", NULL)) {
 error_setg(errp, "Cannot use relative backing file names for '%s'",
backed);
+return NULL;
 } else {
-path_combine_deprecated(dest, sz, backed, backing);
+return path_combine(backed, backing);
 }
 }
 
@@ -314,12 +323,24 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, 
char *dest, size_t sz,
 Error **errp)
 {
 char *backed;
+char *full_name;
+Error *local_error = NULL;
 
 bdrv_refresh_filename(bs);
 
 backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
- dest, sz, errp);
+
+full_name = bdrv_get_full_backing_filename_from_filename(backed,
+ bs->backing_file,
+ &local_error);
+if (full_name) {
+pstrcpy(dest, sz, full_name);
+g_free(full_name);
+} else if (local_error) {
+error_propagate(errp, local_error);
+} else if (sz > 0) {
+*dest = '\0';
+}
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -4871,17 +4892,17 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size);
 if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
 BlockDriverState *bs;
-char *full_backing = g_new0(char, PATH_MAX);
+char *full_backing;
 int back_flags;
 QDict *backing_options = NULL;
 
-bdrv_get_full_backing_filename_from_filename(filename, backing_file,
- full_backing, PATH_MAX,
- &local_err);
+full_backing =
+bdrv_get_full_backing_filename_from_filename(filename, 
backing_file,
+ &local_err);
 if (local_err) {
-g_free(full_backing);
 goto out;
 }
+asse

[Qemu-block] [PATCH v11 16/31] quorum: Make bdrv_dirname() return NULL

[Max Reitz]

While the common implementation for bdrv_dirname() should return NULL
for quorum BDSs already (because they do not have a file node and their
exact_filename field should be empty), there is no reason not to make
that explicit.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block/quorum.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index b14e9c6ec6..f831f84fd1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1101,6 +1101,16 @@ static void quorum_refresh_filename(BlockDriverState 
*bs, QDict *options)
 bs->full_open_options = opts;
 }
 
+static char *quorum_dirname(BlockDriverState *bs, Error **errp)
+{
+/* In general, there are multiple BDSs with different dirnames below this
+ * one; so there is no unique dirname we could return (unless all are equal
+ * by chance, or there is only one). Therefore, to be consistent, just
+ * always return NULL. */
+error_setg(errp, "Cannot generate a base directory for quorum nodes");
+return NULL;
+}
+
 static BlockDriver bdrv_quorum = {
 .format_name= "quorum",
 
@@ -1109,6 +1119,7 @@ static BlockDriver bdrv_quorum = {
 .bdrv_open  = quorum_open,
 .bdrv_close = quorum_close,
 .bdrv_refresh_filename  = quorum_refresh_filename,
+.bdrv_dirname   = quorum_dirname,
 
 .bdrv_co_flush_to_disk  = quorum_co_flush,
 
-- 
2.17.1

[Qemu-block] [PATCH v11 25/31] block: Do not copy exact_filename from format file

[Max Reitz]

If a format BDS's file BDS is in turn a format BDS, we cannot simply use
the same filename, because when opening a BDS tree based on a filename
alone, qemu will create only one format node on top of one protocol node
(disregarding a potential backing file).

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index c919093a04..041adb90d1 100644
--- a/block.c
+++ b/block.c
@@ -5419,9 +5419,21 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 
 bs->exact_filename[0] = '\0';
 
-/* If no specific options have been given for this BDS, the filename of
- * the underlying file should suffice for this one as well */
-if (bs->file->bs->exact_filename[0] && !generate_json_filename) {
+/*
+ * We can use the underlying file's filename if:
+ * - it has a filename,
+ * - the file is a protocol BDS, and
+ * - opening that file (as this BDS's format) will automatically create
+ *   the BDS tree we have right now, that is:
+ *   - the user did not significantly change this BDS's behavior with
+ * some explicit (strong) options
+ *   - no non-file child of this BDS has been overridden by the user
+ *   Both of these conditions are represented by 
generate_json_filename.
+ */
+if (bs->file->bs->exact_filename[0] &&
+bs->file->bs->drv->bdrv_file_open &&
+!generate_json_filename)
+{
 strcpy(bs->exact_filename, bs->file->bs->exact_filename);
 }
 }
-- 
2.17.1

[Qemu-block] [PATCH v11 30/31] block: BDS options may lack the "driver" option

[Max Reitz]

When BDSs are created by qemu itself (e.g. as filters in block jobs),
they may not have a "driver" option in their options QDict.  When
generating a json:{} filename, however, it must always be present.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 041adb90d1..ecb228b8f8 100644
--- a/block.c
+++ b/block.c
@@ -5306,6 +5306,12 @@ static bool append_strong_runtime_options(QDict *d, 
BlockDriverState *bs)
 }
 }
 
+if (!qdict_haskey(d, "driver")) {
+/* Drivers created with bdrv_new_open_driver() may not have a
+ * @driver option.  Add it here. */
+qdict_put_str(d, "driver", bs->drv->format_name);
+}
+
 return found_any;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v11 21/31] block: Add strong_runtime_opts to BlockDriver

[Max Reitz]

This new field can be set by block drivers to list the runtime options
they accept that may influence the contents of the respective BDS. As of
a follow-up patch, this list will be used by the common
bdrv_refresh_filename() implementation to decide which options to put
into BDS.full_open_options (and consequently whether a JSON filename has
to be created), thus freeing the drivers of having to implement that
logic themselves.

Additionally, this patch adds the field to all of the block drivers that
need it and sets it accordingly.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 include/block/block_int.h |  7 +++
 block/blkdebug.c  | 16 
 block/blklogwrites.c  |  8 
 block/crypto.c|  8 
 block/curl.c  | 21 +
 block/gluster.c   | 19 +++
 block/iscsi.c | 18 ++
 block/nbd.c   | 14 ++
 block/nfs.c   | 11 +++
 block/null.c  |  9 +
 block/nvme.c  |  8 
 block/qcow.c  |  7 +++
 block/qcow2.c |  7 +++
 block/quorum.c| 11 +++
 block/raw-format.c| 10 +-
 block/rbd.c   | 14 ++
 block/replication.c   |  8 
 block/sheepdog.c  | 12 
 block/ssh.c   | 12 
 block/throttle.c  |  7 +++
 block/vpc.c   |  7 +++
 block/vvfat.c | 12 
 block/vxhs.c  | 11 +++
 23 files changed, 256 insertions(+), 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f448b90ee7..7adcda9898 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -516,6 +516,13 @@ struct BlockDriver {
 void (*bdrv_register_buf)(BlockDriverState *bs, void *host, size_t size);
 void (*bdrv_unregister_buf)(BlockDriverState *bs, void *host);
 QLIST_ENTRY(BlockDriver) list;
+
+/* Pointer to a NULL-terminated array of names of strong options
+ * that can be specified for bdrv_open(). A strong option is one
+ * that changes the data of a BDS.
+ * If this pointer is NULL, the array is considered empty.
+ * "filename" and "driver" are always considered strong. */
+const char *const *strong_runtime_opts;
 };
 
 typedef struct BlockLimits {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0759452925..71b4275b98 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -888,6 +888,20 @@ static int blkdebug_reopen_prepare(BDRVReopenState 
*reopen_state,
 return 0;
 }
 
+static const char *const blkdebug_strong_runtime_opts[] = {
+"config",
+"inject-error.",
+"set-state.",
+"align",
+"max-transfer",
+"opt-write-zero",
+"max-write-zero",
+"opt-discard",
+"max-discard",
+
+NULL
+};
+
 static BlockDriver bdrv_blkdebug = {
 .format_name= "blkdebug",
 .protocol_name  = "blkdebug",
@@ -917,6 +931,8 @@ static BlockDriver bdrv_blkdebug = {
 = blkdebug_debug_remove_breakpoint,
 .bdrv_debug_resume  = blkdebug_debug_resume,
 .bdrv_debug_is_suspended= blkdebug_debug_is_suspended,
+
+.strong_runtime_opts= blkdebug_strong_runtime_opts,
 };
 
 static void bdrv_blkdebug_init(void)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 8f1b589d54..1c8e200326 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -518,6 +518,13 @@ blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t 
offset, int count)
  LOG_DISCARD_FLAG, false);
 }
 
+static const char *const blk_log_writes_strong_runtime_opts[] = {
+"log-append",
+"log-sector-size",
+
+NULL
+};
+
 static BlockDriver bdrv_blk_log_writes = {
 .format_name= "blklogwrites",
 .instance_size  = sizeof(BDRVBlkLogWritesState),
@@ -537,6 +544,7 @@ static BlockDriver bdrv_blk_log_writes = {
 .bdrv_co_block_status   = bdrv_co_block_status_from_file,
 
 .is_filter  = true,
+.strong_runtime_opts= blk_log_writes_strong_runtime_opts,
 };
 
 static void bdrv_blk_log_writes_init(void)
diff --git a/block/crypto.c b/block/crypto.c
index 33ee01bebd..62b5d1c639 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -621,6 +621,12 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs)
 return spec_info;
 }
 
+static const char *const block_crypto_strong_runtime_opts[] = {
+BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
+
+NULL
+};
+
 BlockDriver bdrv_crypto_luks = {
 .format_name= "luks",
 .instance_size  = sizeof(BlockCrypto),
@@ -642,6 +648,8 @@ BlockDriver bdrv_crypto_luks = {
 .bdrv_getlength = block_crypto_getlength,
 .bdrv_get_info  = block_crypto_get_info_luks,
 .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
+

[Qemu-block] [PATCH v11 24/31] block: Purify .bdrv_refresh_filename()

[Max Reitz]

Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
Now that we have generic code in the central bdrv_refresh_filename() for
creating BDS.full_open_options, we can drop the latter part from all
BlockDriver.bdrv_refresh_filename() implementations.

This also means that we can drop all of the existing default code for
this from the global bdrv_refresh_filename() itself.

Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
after having set BDS.full_open_options, because the block driver's
implementation should now be allowed to depend on BDS.full_open_options
being set correctly.

Finally, with this patch we can drop the @options parameter from
BlockDriver.bdrv_refresh_filename(); also, add a comment on this
function's purpose in block/block_int.h while touching its interface.

This completely obsoletes blklogwrite's implementation of
.bdrv_refresh_filename().

Signed-off-by: Max Reitz 
---
 include/block/block_int.h |   6 +-
 block.c   | 131 +++---
 block/blkdebug.c  |  54 ++--
 block/blklogwrites.c  |  23 ---
 block/blkverify.c |  16 +
 block/commit.c|   2 +-
 block/mirror.c|   2 +-
 block/nbd.c   |  23 +--
 block/nfs.c   |  36 +--
 block/null.c  |  22 ---
 block/nvme.c  |  22 ---
 block/quorum.c|  30 -
 12 files changed, 80 insertions(+), 287 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 183de0b832..26a2af86c5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -139,7 +139,11 @@ struct BlockDriver {
 Error **errp);
 int (*bdrv_make_empty)(BlockDriverState *bs);
 
-void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+/*
+ * Refreshes the bs->exact_filename field. If that is impossible,
+ * bs->exact_filename has to be left empty.
+ */
+void (*bdrv_refresh_filename)(BlockDriverState *bs);
 
 /*
  * Gathers the open options for all children into @target.
diff --git a/block.c b/block.c
index 761fadf5bc..c919093a04 100644
--- a/block.c
+++ b/block.c
@@ -5309,33 +5309,6 @@ static bool append_strong_runtime_options(QDict *d, 
BlockDriverState *bs)
 return found_any;
 }
 
-static bool append_open_options(QDict *d, BlockDriverState *bs)
-{
-const QDictEntry *entry;
-QemuOptDesc *desc;
-bool found_any = false;
-
-for (entry = qdict_first(bs->options); entry;
- entry = qdict_next(bs->options, entry))
-{
-/* Exclude all non-driver-specific options */
-for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
-if (!strcmp(qdict_entry_key(entry), desc->name)) {
-break;
-}
-}
-if (desc->name) {
-continue;
-}
-
-qdict_put_obj(d, qdict_entry_key(entry),
-  qobject_ref(qdict_entry_value(entry)));
-found_any = true;
-}
-
-return found_any;
-}
-
 /* Note: This function may return false positives; it may return true
  * even if opening the backing file specified by bs's image header
  * would result in exactly bs->backing. */
@@ -5369,6 +5342,8 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 BdrvChild *child;
 QDict *opts;
 bool backing_overridden;
+bool generate_json_filename; /* Whether our default implementation should
+fill exact_filename (false) or not (true) 
*/
 
 if (!drv) {
 return;
@@ -5404,90 +5379,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 backing_overridden = false;
 }
 
-if (drv->bdrv_refresh_filename) {
-/* Obsolete information is of no use here, so drop the old file name
- * information before refreshing it */
-bs->exact_filename[0] = '\0';
-if (bs->full_open_options) {
-qobject_unref(bs->full_open_options);
-bs->full_open_options = NULL;
-}
-
-opts = qdict_new();
-append_open_options(opts, bs);
-drv->bdrv_refresh_filename(bs, opts);
-qobject_unref(opts);
-} else if (bs->file) {
-/* Try to reconstruct valid information from the underlying file */
-bool has_open_options;
-
-bs->exact_filename[0] = '\0';
-if (bs->full_open_options) {
-qobject_unref(bs->full_open_options);
-bs->full_open_options = NULL;
-}
-
-opts = qdict_new();
-has_open_options = append_open_options(opts, bs);
-has_open_options |= backing_overridden;
-
-/* If no specific options have been given for this BDS, the filename of
- * the underlying file should suffice for this one as well */
-if (bs->file->bs->exact_filename[0

[Qemu-block] [PATCH v11 31/31] iotests: Test json:{} filenames of internal BDSs

[Max Reitz]

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/224 | 139 +
 tests/qemu-iotests/224.out |  18 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 158 insertions(+)
 create mode 100755 tests/qemu-iotests/224
 create mode 100644 tests/qemu-iotests/224.out

diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
new file mode 100755
index 00..ef652ad488
--- /dev/null
+++ b/tests/qemu-iotests/224
@@ -0,0 +1,139 @@
+#!/usr/bin/env python
+#
+# Test json:{} filenames with qemu-internal BDSs
+# (the one of commit, to be precise)
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# Creator/Owner: Max Reitz 
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent, filter_testfiles, \
+filter_imgfmt
+import json
+
+# Need backing file support (for arbitrary backing formats)
+iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+# There are two variations of this test:
+# (1) We do not set filter_node_name.  In that case, the commit_top
+# driver should not appear anywhere.
+# (2) We do set filter_node_name.  In that case, it should appear.
+#
+# This for loop executes both.
+for filter_node_name in False, True:
+log('')
+log('--- filter_node_name: %s ---' % filter_node_name)
+log('')
+
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('mid.img') as mid_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+assert qemu_img('create', '-f', iotests.imgfmt,
+base_img_path, '64M') == 0
+assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+mid_img_path) == 0
+assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+top_img_path) == 0
+
+# Something to commit
+assert qemu_io_silent(mid_img_path, '-c', 'write -P 1 0 1M') == 0
+
+vm.launch()
+
+# Change the bottom-most image's backing file (to null-co://)
+# to enforce json:{} filenames
+vm.qmp_log('blockdev-add',
+node_name='top',
+driver=iotests.imgfmt,
+file={
+'driver': 'file',
+'filename': top_img_path
+},
+backing={
+'node-name': 'mid',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': mid_img_path
+},
+'backing': {
+'node-name': 'base',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': base_img_path
+},
+'backing': {
+'driver': 'null-co'
+}
+}
+},
+filters=[filter_testfiles, filter_imgfmt])
+
+# As long as block-commit does not accept node names, we have to
+# get our mid/base filenames here
+mid_name = vm.node_info('mid')['image']['filename']
+base_name = vm.node_info('base')['image']['filename']
+
+assert mid_name[:5] == 'json:'
+assert base_name[:5] == 'json:'
+
+# Start the block job
+if filter_node_name:
+vm.qmp_log('block-commit',
+job_id='commit',
+device='top',
+filter_node_name='filter_node',
+top=mid_name,
+base=base_name,
+speed=1,
+filters=[filter_testfiles, filter_imgfmt])
+else:
+vm.qmp_log('block-commit',
+job_id='commit',
+device='top',
+top=mid_name,
+base=base_name,
+speed=1,
+filters=[filter_testfiles, filter_imgfmt])
+
+ 

[Qemu-block] [PATCH v11 29/31] block/null: Generate filename even with latency-ns

[Max Reitz]

While we cannot represent the latency-ns option in a filename, it is not
a strong option so not being able to should not stop us from generating
a filename nonetheless.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/null.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/null.c b/block/null.c
index 1c56a0ef01..a322929478 100644
--- a/block/null.c
+++ b/block/null.c
@@ -248,7 +248,8 @@ static void null_refresh_filename(BlockDriverState *bs)
 {
 /* These options can be ignored */
 if (strcmp(qdict_entry_key(e), "filename") &&
-strcmp(qdict_entry_key(e), "driver"))
+strcmp(qdict_entry_key(e), "driver") &&
+strcmp(qdict_entry_key(e), NULL_OPT_LATENCY))
 {
 return;
 }
-- 
2.17.1

[Qemu-block] [PATCH v11 26/31] block/nvme: Fix bdrv_refresh_filename()

[Max Reitz]

Currently, nvme's bdrv_refresh_filename() is an exact copy of null's
implementation.  However, for null, "null-co://" and "null-aio://" are
indeed valid filenames -- for nvme, they are not, as a device address is
still required.

The correct implementation should generate a filename of the form
"nvme://[PCI address]/[namespace]" (as the comment above
nvme_parse_filename() describes).

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/nvme.c | 23 +--
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 0df7cb30fb..860a3d7113 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -111,6 +111,9 @@ typedef struct {
 
 /* Total size of mapped qiov, accessed under dma_map_lock */
 int dma_map_count;
+
+/* PCI address (required for nvme_refresh_filename()) */
+char *device;
 } BDRVNVMeState;
 
 #define NVME_BLOCK_OPT_DEVICE "device"
@@ -558,6 +561,7 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 
 qemu_co_mutex_init(&s->dma_map_lock);
 qemu_co_queue_init(&s->dma_flush_queue);
+s->device = g_strdup(device);
 s->nsid = namespace;
 s->aio_context = bdrv_get_aio_context(bs);
 ret = event_notifier_init(&s->irq_notifier, 0);
@@ -730,6 +734,8 @@ static void nvme_close(BlockDriverState *bs)
 event_notifier_cleanup(&s->irq_notifier);
 qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
 qemu_vfio_close(s->vfio);
+
+g_free(s->device);
 }
 
 static int nvme_file_open(BlockDriverState *bs, QDict *options, int flags,
@@ -1060,21 +1066,10 @@ static int nvme_reopen_prepare(BDRVReopenState 
*reopen_state,
 
 static void nvme_refresh_filename(BlockDriverState *bs)
 {
-const QDictEntry *e;
-
-for (e = qdict_first(bs->full_open_options); e;
- e = qdict_next(bs->full_open_options, e))
-{
-/* These options can be ignored */
-if (strcmp(qdict_entry_key(e), "filename") &&
-strcmp(qdict_entry_key(e), "driver"))
-{
-return;
-}
-}
+BDRVNVMeState *s = bs->opaque;
 
-snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
- bs->drv->format_name);
+snprintf(bs->exact_filename, sizeof(bs->exact_filename), "nvme://%s/%i",
+ s->device, s->nsid);
 }
 
 static void nvme_refresh_limits(BlockDriverState *bs, Error **errp)
-- 
2.17.1

[Qemu-block] [PATCH v11 27/31] block/curl: Harmonize option defaults

[Max Reitz]

Both of the defaults we currently have in the curl driver are named
based on a slightly different schema, let's unify that and call both
CURL_BLOCK_OPT_${NAME}_DEFAULT.

While at it, we can add a macro for the third option for which a default
exists, namely "sslverify".

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/curl.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index dbbb72b5d7..96790fdc70 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -73,8 +73,6 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 
 #define CURL_NUM_STATES 8
 #define CURL_NUM_ACB8
-#define READ_AHEAD_DEFAULT (256 * 1024)
-#define CURL_TIMEOUT_DEFAULT 5
 #define CURL_TIMEOUT_MAX 1
 
 #define CURL_BLOCK_OPT_URL   "url"
@@ -88,6 +86,10 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
 #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
 
+#define CURL_BLOCK_OPT_READAHEAD_DEFAULT (256 * 1024)
+#define CURL_BLOCK_OPT_SSLVERIFY_DEFAULT true
+#define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
+
 struct BDRVCURLState;
 
 static bool libcurl_initialized;
@@ -708,7 +710,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 s->readahead_size = qemu_opt_get_size(opts, CURL_BLOCK_OPT_READAHEAD,
-  READ_AHEAD_DEFAULT);
+  CURL_BLOCK_OPT_READAHEAD_DEFAULT);
 if ((s->readahead_size & 0x1ff) != 0) {
 error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512",
s->readahead_size);
@@ -716,13 +718,14 @@ static int curl_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 
 s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
- CURL_TIMEOUT_DEFAULT);
+ CURL_BLOCK_OPT_TIMEOUT_DEFAULT);
 if (s->timeout > CURL_TIMEOUT_MAX) {
 error_setg(errp, "timeout parameter is too large or negative");
 goto out_noclean;
 }
 
-s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
+s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY,
+ CURL_BLOCK_OPT_SSLVERIFY_DEFAULT);
 
 cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
 cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
-- 
2.17.1

[Qemu-block] [PATCH v11 22/31] block: Add BlockDriver.bdrv_gather_child_options

[Max Reitz]

Some follow-up patches will rework the way bs->full_open_options is
refreshed in bdrv_refresh_filename(). The new implementation will remove
the need for the block drivers' bdrv_refresh_filename() implementations
to set bs->full_open_options; instead, it will be generic and use static
information from each block driver.

However, by implementing bdrv_gather_child_options(), block drivers will
still be able to override the way the full_open_options of their
children are incorporated into their own.

We need to implement this function for VMDK because we have to prevent
the generic implementation from gathering the options of all children:
It is not possible to specify options for the extents through the
runtime options.

For quorum, the child names that would be used by the generic
implementation and the ones that we actually (currently) want to use
differ. See quorum_gather_child_options() for more information.

Note that both of these are cases which are not ideal: In case of VMDK
it would probably be nice to be able to specify options for all extents.
In case of quorum, the current runtime option structure is simply broken
and needs to be fixed (but that is left for another patch).

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 include/block/block_int.h | 24 +++
 block/quorum.c| 40 +++
 block/vmdk.c  | 19 +++
 3 files changed, 83 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7adcda9898..183de0b832 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -141,6 +141,30 @@ struct BlockDriver {
 
 void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
 
+/*
+ * Gathers the open options for all children into @target.
+ * A simple format driver (without backing file support) might
+ * implement this function like this:
+ *
+ * QINCREF(bs->file->bs->full_open_options);
+ * qdict_put(target, "file", bs->file->bs->full_open_options);
+ *
+ * If not specified, the generic implementation will simply put
+ * all children's options under their respective name.
+ *
+ * @backing_overridden is true when bs->backing seems not to be
+ * the child that would result from opening bs->backing_file.
+ * Therefore, if it is true, the backing child's options should be
+ * gathered; otherwise, there is no need since the backing child
+ * is the one implied by the image header.
+ *
+ * Note that ideally this function would not be needed.  Every
+ * block driver which implements it is probably doing something
+ * shady regarding its runtime option structure.
+ */
+void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target,
+  bool backing_overridden);
+
 /*
  * Returns an allocated string which is the directory name of this BDS: It
  * will be used to make relative filenames absolute by prepending this
diff --git a/block/quorum.c b/block/quorum.c
index d42d27445e..644b173d53 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1101,6 +1101,45 @@ static void quorum_refresh_filename(BlockDriverState 
*bs, QDict *options)
 bs->full_open_options = opts;
 }
 
+static void quorum_gather_child_options(BlockDriverState *bs, QDict *target,
+bool backing_overridden)
+{
+BDRVQuorumState *s = bs->opaque;
+QList *children_list;
+int i;
+
+/*
+ * The generic implementation for gathering child options in
+ * bdrv_refresh_filename() would use the names of the children
+ * as specified for bdrv_open_child() or bdrv_attach_child(),
+ * which is "children.%u" with %u being a value
+ * (s->next_child_index) that is incremented each time a new child
+ * is added (and never decremented).  Since children can be
+ * deleted at runtime, there may be gaps in that enumeration.
+ * When creating a new quorum BDS and specifying the children for
+ * it through runtime options, the enumeration used there may not
+ * have any gaps, though.
+ *
+ * Therefore, we have to create a new gap-less enumeration here
+ * (which we can achieve by simply putting all of the children's
+ * full_open_options into a QList).
+ *
+ * XXX: Note that there are issues with the current child option
+ *  structure quorum uses (such as the fact that children do
+ *  not really have unique permanent names).  Therefore, this
+ *  is going to have to change in the future and ideally we
+ *  want quorum to be covered by the generic implementation.
+ */
+
+children_list = qlist_new();
+qdict_put(target, "children", children_list);
+
+for (i = 0; i < s->num_children; i++) {
+qlist_append(children_list,
+ qobject_ref(s->children[i]->bs->full_open_options));
+  

[Qemu-block] [PATCH v11 23/31] block: Generically refresh runtime options

[Max Reitz]

Instead of having every block driver which implements
bdrv_refresh_filename() copy all of the strong runtime options over to
bs->full_open_options, implement this process generically in
bdrv_refresh_filename().

This patch only adds this new generic implementation, it does not remove
the old functionality. This is done in a follow-up patch.

With this patch, some superfluous information (that should never have
been there) may be removed from some JSON filenames, as can be seen in
the change to iotest 110's reference output.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c| 116 -
 tests/qemu-iotests/110.out |   2 +-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 6b4fccecc9..761fadf5bc 100644
--- a/block.c
+++ b/block.c
@@ -5223,6 +5223,92 @@ out:
 return to_replace_bs;
 }
 
+/**
+ * Iterates through the list of runtime option keys that are said to
+ * be "strong" for a BDS.  An option is called "strong" if it changes
+ * a BDS's data.  For example, the null block driver's "size" and
+ * "read-zeroes" options are strong, but its "latency-ns" option is
+ * not.
+ *
+ * If a key returned by this function ends with a dot, all options
+ * starting with that prefix are strong.
+ */
+static const char *const *strong_options(BlockDriverState *bs,
+ const char *const *curopt)
+{
+static const char *const global_options[] = {
+"driver", "filename", NULL
+};
+
+if (!curopt) {
+return &global_options[0];
+}
+
+curopt++;
+if (curopt == &global_options[ARRAY_SIZE(global_options) - 1] && bs->drv) {
+curopt = bs->drv->strong_runtime_opts;
+}
+
+return (curopt && *curopt) ? curopt : NULL;
+}
+
+/**
+ * Copies all strong runtime options from bs->options to the given
+ * QDict.  The set of strong option keys is determined by invoking
+ * strong_options().
+ *
+ * Returns true iff any strong option was present in bs->options (and
+ * thus copied to the target QDict) with the exception of "filename"
+ * and "driver".  The caller is expected to use this value to decide
+ * whether the existence of strong options prevents the generation of
+ * a plain filename.
+ */
+static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs)
+{
+bool found_any = false;
+const char *const *option_name = NULL;
+
+if (!bs->drv) {
+return false;
+}
+
+while ((option_name = strong_options(bs, option_name))) {
+bool option_given = false;
+
+assert(strlen(*option_name) > 0);
+if ((*option_name)[strlen(*option_name) - 1] != '.') {
+QObject *entry = qdict_get(bs->options, *option_name);
+if (!entry) {
+continue;
+}
+
+qdict_put_obj(d, *option_name, qobject_ref(entry));
+option_given = true;
+} else {
+const QDictEntry *entry;
+for (entry = qdict_first(bs->options); entry;
+ entry = qdict_next(bs->options, entry))
+{
+if (strstart(qdict_entry_key(entry), *option_name, NULL)) {
+qdict_put_obj(d, qdict_entry_key(entry),
+  qobject_ref(qdict_entry_value(entry)));
+option_given = true;
+}
+}
+}
+
+/* While "driver" and "filename" need to be included in a JSON 
filename,
+ * their existence does not prohibit generation of a plain filename. */
+if (!found_any && option_given &&
+strcmp(*option_name, "driver") && strcmp(*option_name, "filename"))
+{
+found_any = true;
+}
+}
+
+return found_any;
+}
+
 static bool append_open_options(QDict *d, BlockDriverState *bs)
 {
 const QDictEntry *entry;
@@ -5399,9 +5485,37 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 bs->full_open_options = opts;
 }
 
+/* Gather the options QDict */
+opts = qdict_new();
+append_strong_runtime_options(opts, bs);
+
+if (drv->bdrv_gather_child_options) {
+/* Some block drivers may not want to present all of their children's
+ * options, or name them differently from BdrvChild.name */
+drv->bdrv_gather_child_options(bs, opts, backing_overridden);
+} else {
+QLIST_FOREACH(child, &bs->children, next) {
+if (child->role == &child_backing && !backing_overridden) {
+/* We can skip the backing BDS if it has not been overridden */
+continue;
+}
+
+qdict_put(opts, child->name,
+  qobject_ref(child->bs->full_open_options));
+}
+
+if (backing_overridden && !bs->backing) {
+/* Force no backing file */
+qdict_put_null(opts, "backing");
+}
+}
+
+qobject_unref(bs->full_open_options);
+  

[Qemu-block] [PATCH v11 28/31] block/curl: Implement bdrv_refresh_filename()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/curl.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 96790fdc70..a5d3e64b84 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -963,6 +963,23 @@ static int64_t curl_getlength(BlockDriverState *bs)
 return s->len;
 }
 
+static void curl_refresh_filename(BlockDriverState *bs)
+{
+BDRVCURLState *s = bs->opaque;
+
+/* "readahead" and "timeout" do not change the guest-visible data,
+ * so ignore them */
+if (s->sslverify != CURL_BLOCK_OPT_SSLVERIFY_DEFAULT ||
+s->cookie || s->username || s->password || s->proxyusername ||
+s->proxypassword)
+{
+return;
+}
+
+pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), s->url);
+}
+
+
 static const char *const curl_strong_runtime_opts[] = {
 CURL_BLOCK_OPT_URL,
 CURL_BLOCK_OPT_SSLVERIFY,
@@ -991,6 +1008,7 @@ static BlockDriver bdrv_http = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .strong_runtime_opts= curl_strong_runtime_opts,
 };
 
@@ -1009,6 +1027,7 @@ static BlockDriver bdrv_https = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .strong_runtime_opts= curl_strong_runtime_opts,
 };
 
@@ -1027,6 +1046,7 @@ static BlockDriver bdrv_ftp = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .strong_runtime_opts= curl_strong_runtime_opts,
 };
 
@@ -1045,6 +1065,7 @@ static BlockDriver bdrv_ftps = {
 .bdrv_detach_aio_context= curl_detach_aio_context,
 .bdrv_attach_aio_context= curl_attach_aio_context,
 
+.bdrv_refresh_filename  = curl_refresh_filename,
 .strong_runtime_opts= curl_strong_runtime_opts,
 };
 
-- 
2.17.1