[Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

Attributes are simple flags, associated with individual timers for their whole 
lifetime.
They intended to be used to mark individual timers for special handling by 
various qemu features operating at qemu core level.
New/init functions family in timer interface updated and refactored (new 
'attribute' argument added, timer_list replaced with timer_list_group+type 
combinations, comments improved to avoid info duplication).
Also existing aio interface extended with attribute-enabled variants of 
functions, which create/initialize timers.

Signed-off-by: Artem Pisarenko 
---

Notes:
v2:
- timer creation/initialize functions reworked and and their unnecessary 
variants removed (as Paolo Bonzini suggested)
- also their comments improved to avoid info duplication

 include/block/aio.h   |  57 ++---
 include/qemu/timer.h  | 128 ++
 tests/ptimer-test-stubs.c |  13 +++--
 util/qemu-timer.c |  18 +--
 4 files changed, 146 insertions(+), 70 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index f08630c..fce9d48 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -388,6 +388,31 @@ struct LinuxAioState *aio_setup_linux_aio(AioContext *ctx, 
Error **errp);
 struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
 
 /**
+ * aio_timer_new_with_attrs:
+ * @ctx: the aio context
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to 
assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Allocate a new timer (with attributes) attached to the context @ctx.
+ * The function is responsible for memory allocation.
+ *
+ * The preferred interface is aio_timer_init or aio_timer_init_with_attrs.
+ * Use that unless you really need dynamic memory allocation.
+ *
+ * Returns: a pointer to the new timer
+ */
+static inline QEMUTimer *aio_timer_new_with_attrs(AioContext *ctx,
+  QEMUClockType type,
+  int scale, int attributes,
+  QEMUTimerCB *cb, void 
*opaque)
+{
+return timer_new_full(&ctx->tlg, type, scale, attributes, cb, opaque);
+}
+
+/**
  * aio_timer_new:
  * @ctx: the aio context
  * @type: the clock type
@@ -396,10 +421,7 @@ struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
  * @opaque: the opaque pointer to pass to the callback
  *
  * Allocate a new timer attached to the context @ctx.
- * The function is responsible for memory allocation.
- *
- * The preferred interface is aio_timer_init. Use that
- * unless you really need dynamic memory allocation.
+ * See aio_timer_new_with_attrs for details.
  *
  * Returns: a pointer to the new timer
  */
@@ -407,7 +429,28 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, 
QEMUClockType type,
int scale,
QEMUTimerCB *cb, void *opaque)
 {
-return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
+return timer_new_full(&ctx->tlg, type, scale, 0, cb, opaque);
+}
+
+/**
+ * aio_timer_init_with_attrs:
+ * @ctx: the aio context
+ * @ts: the timer
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to 
assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Initialise a new timer (with attributes) attached to the context @ctx.
+ * The caller is responsible for memory allocation.
+ */
+static inline void aio_timer_init_with_attrs(AioContext *ctx,
+ QEMUTimer *ts, QEMUClockType type,
+ int scale, int attributes,
+ QEMUTimerCB *cb, void *opaque)
+{
+timer_init_full(ts, &ctx->tlg, type, scale, attributes, cb, opaque);
 }
 
 /**
@@ -420,14 +463,14 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, 
QEMUClockType type,
  * @opaque: the opaque pointer to pass to the callback
  *
  * Initialise a new timer attached to the context @ctx.
- * The caller is responsible for memory allocation.
+ * See aio_timer_init_with_attrs for details.
  */
 static inline void aio_timer_init(AioContext *ctx,
   QEMUTimer *ts, QEMUClockType type,
   int scale,
   QEMUTimerCB *cb, void *opaque)
 {
-timer_init_tl(ts, ctx->tlg.tl[type], scale, cb, opaque);
+timer_init_full(ts, &ctx->tlg, type, scale, 0, cb, opaque);
 }
 
 /**
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 39ea907..e225ad4 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,6 +52,28 @@ typedef enum {
 QEMU_CLOCK_MAX
 } QEMUClockType;
 
+/**
+ * QEMU Ti

Re: [Qemu-block] [PATCH v2 1/8] block: Update flags in bdrv_set_read_only()

[Alberto Garcia]

On Fri 12 Oct 2018 01:55:25 PM CEST, Kevin Wolf wrote:
> To fully change the read-only state of a node, we must not only change
> bs->read_only, but also update bs->open_flags.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Stefan Hajnoczi]

On Wed, Oct 17, 2018 at 02:24:19PM +0600, Artem Pisarenko wrote:
> Attributes are simple flags, associated with individual timers for their 
> whole lifetime.
> They intended to be used to mark individual timers for special handling by 
> various qemu features operating at qemu core level.

I'm worried that this sentence suggests various parts of QEMU will stash
state in ts->attributes.  That's messy and they shouldn't do this.  Make
the field private to qemu-timer.c.

Attributes should only affect qemu-timer.c behavior.  Other parts of
QEMU should not act differently based on timer attributes (i.e. checking
bits).  If they need to do that then it suggests something isn't
properly encapsulated in qemu-timer.c.

> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..fce9d48 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -388,6 +388,31 @@ struct LinuxAioState *aio_setup_linux_aio(AioContext 
> *ctx, Error **errp);
>  struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
>  
>  /**
> + * aio_timer_new_with_attrs:
> + * @ctx: the aio context
> + * @type: the clock type
> + * @scale: the scale
> + * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to 
> assign

Further down in this patch the notation is QEMU_TIMER_ATTR_, which I
think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
macro.  Please use the QEMU_TIMER_ATTR_ notation consistently.

> +/**
> + * QEMU Timer attributes:
> + *
> + * An individual timer may be assigned with one or multiple attributes when
> + * initialized.
> + * Attribute is a static flag, meaning that timer has corresponding property.
> + * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
> + * which used to initialize timer, stored to 'attributes' member and can be
> + * retrieved externally with timer_get_attributes() call.
> + * Values of QEMUTimerAttrBit aren't used directly,
> + * instead each attribute in bit set accessed with QEMU_TIMER_ATTR_ 
> macro,
> + * where  is a unique part of attribute identifier.
> + *
> + * No attributes defined currently.
> + */
> +
> +typedef enum {
> +QEMU_TIMER_ATTRBIT__NONE
> +} QEMUTimerAttrBit;
> +
> +#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)

What is the purpose of this bit?  I guess it's just here as a
placeholder because no real bits have been defined yet.  Hopefully the
next patch removes it (/* This placeholder is removed in the next patch
*/ would be a nice way to document this for reviewers).

The enum isn't needed and makes debugging harder since the bit number is
implicit in the enum ordering.  This alternative is clearer and more
concise:

  #define QEMU_TIMER_ATTR_foo BIT(n)


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 17/10/2018 11:12, Stefan Hajnoczi wrote:
>> Attributes are simple flags, associated with individual timers for their 
>> whole lifetime.
>> They intended to be used to mark individual timers for special handling by 
>> various qemu features operating at qemu core level.
> I'm worried that this sentence suggests various parts of QEMU will stash
> state in ts->attributes.  That's messy and they shouldn't do this.  Make
> the field private to qemu-timer.c.

Yes, the contents of the fields are private.  Are you suggesting a
different wording for the commit message or the "QEMU Timer attributes"
doc comment, or something more than that?  Possibly removing
timer_get_attributes altogether?

Paolo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/vhdx: Don't take address of fields in packed structs

[Stefan Hajnoczi]

On Tue, Oct 16, 2018 at 06:09:38PM +0100, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> Signed-off-by: Peter Maydell 
> ---
> Usual disclaimer: produced with "make check" only, but purely
> automated conversion should be safe.
> 
>  block/vhdx.h|  12 ++---
>  block/vhdx-endian.c | 118 ++--
>  block/vhdx-log.c|   4 +-
>  block/vhdx.c|  18 +++
>  4 files changed, 76 insertions(+), 76 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] block/vdi: Don't take address of fields in packed structs

[Stefan Hajnoczi]

On Tue, Oct 16, 2018 at 06:25:03PM +0100, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> There are other places where we take the address of a packed member
> in this file for other purposes than passing it to a byteswap
> function (all the calls to qemu_uuid_*()); we leave those for now.
> 
> Signed-off-by: Peter Maydell 
> ---
> Another "tested with make check" auto-conversion patch. In this
> case, as noted above, it doesn't fix all the warnings for the file,
> but we might as well put the easy part of the fix in. I'm not sure
> what to do with the qemu_uuid_*() calls. Something like
>  QemuUUID uuid_link = header->uuid_link;
> and then using "qemu_uuid_is_null(uuid_link)" rather than

I would take this route.  (I think you mean qemu_uuid_is_null(&uuid_link).)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] migration/block-dirty-bitmap: fix Coverity CID1390625

[Stefan Hajnoczi]

On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Theoretically possible that we finish the skipping loop with bs = NULL
> and the following code will crash trying to dereference it. Fix that.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  migration/block-dirty-bitmap.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 477826330c..6de808f95f 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>  bs = backing_bs(bs);
>  }
>  
> +if (!bs || bs->implicit) {
> +continue;
> +}
> +
>  for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>   bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>  {

Previous discussion:
http://qemu.11.n7.nabble.com/PATCH-migration-Appease-coverity-skip-empty-block-trees-td582504.html

I've CCed John so he can take a look.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

> Further down in this patch the notation is QEMU_TIMER_ATTR_, which I
> think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
> macro.  Please use the QEMU_TIMER_ATTR_ notation consistently.

Yes, I've just forgot to update comments after previous patch version,
where it actually was macro.

> What is the purpose of this bit?  I guess it's just here as a
> placeholder because no real bits have been defined yet.  Hopefully the
> next patch removes it (/* This placeholder is removed in the next patch
> */ would be a nice way to document this for reviewers).

It's just to prevent compilation errors, as required by
https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches

> The enum isn't needed and makes debugging harder since the bit number is
> implicit in the enum ordering.  This alternative is clearer and more
> concise:
>
>   #define QEMU_TIMER_ATTR_foo BIT(n)

Agree.

>ср, 17 окт. 2018 г. в 15:15, Paolo Bonzini :
>On 17/10/2018 11:12, Stefan Hajnoczi wrote:
>>> Attributes are simple flags, associated with individual timers for
their whole lifetime.
>>> They intended to be used to mark individual timers for special handling
by various qemu features operating at qemu core level.
>> I'm worried that this sentence suggests various parts of QEMU will stash
>> state in ts->attributes.  That's messy and they shouldn't do this.  Make
>> the field private to qemu-timer.c.
>
> Yes, the contents of the fields are private.  Are you suggesting a
> different wording for the commit message or the "QEMU Timer attributes"
> doc comment, or something more than that?  Possibly removing
> timer_get_attributes altogether?

Actually, attributes aren't intended to be changed externally of timers
code.
The purpose of timer_get_attributes() is just informational, same like if
timer_get_clock(), timer_get_scale(), etc. would exist. But since none of
such accessors exists, adding just one for 'attributes' looks exceptional.
And it isn't being used even after whole patch series applied. As such, it
could be just removed.
Any suggestons to improve commit message and/or comments, if any ?
-- 

С уважением,
  Артем Писаренко

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 17/10/2018 12:57, Artem Pisarenko wrote:
>> Further down in this patch the notation is QEMU_TIMER_ATTR_, which I
>> think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
>> macro.  Please use the QEMU_TIMER_ATTR_ notation consistently.
> 
> Yes, I've just forgot to update comments after previous patch version,
> where it actually was macro.
> 
>> What is the purpose of this bit?  I guess it's just here as a
>> placeholder because no real bits have been defined yet.  Hopefully the
>> next patch removes it (/* This placeholder is removed in the next patch
>> */ would be a nice way to document this for reviewers).
> 
> It's just to prevent compilation errors, as required by
> https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
> 
>> The enum isn't needed and makes debugging harder since the bit number is
>> implicit in the enum ordering.  This alternative is clearer and more
>> concise:
>> 
>>   #define QEMU_TIMER_ATTR_foo BIT(n)
> 
> Agree.

Like this?

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 86ce70f20e..ef7526e389 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -55,25 +55,13 @@ typedef enum {
 /**
  * QEMU Timer attributes:
  *
- * An individual timer may be assigned with one or multiple attributes when
- * initialized.
- * Attribute is a static flag, meaning that timer has corresponding
property.
- * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
- * which used to initialize timer, stored to 'attributes' member and can be
- * retrieved externally with timer_get_attributes() call.
- * Values of QEMUTimerAttrBit aren't used directly,
- * instead each attribute in bit set accessed with QEMU_TIMER_ATTR_
macro,
- * where  is a unique part of attribute identifier.
+ * An individual timer may be given one or multiple attributes when
initialized.
+ * Each attribute corresponds to one bit.  Attributes modify the processing
+ * of timers when they fire.
  *
  * No attributes defined currently.
  */

-typedef enum {
-QEMU_TIMER_ATTRBIT__NONE
-} QEMUTimerAttrBit;
-
-#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)
-
 typedef struct QEMUTimerList QEMUTimerList;

 struct QEMUTimerListGroup {
@@ -640,14 +628,6 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType
type, QEMUTimerCB *cb,
 return timer_new(type, SCALE_MS, cb, opaque);
 }

-/**
- * timer_get_attributes:
- * @ts: the timer
- *
- * Return 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values
- */
-int timer_get_attributes(QEMUTimer *ts);
-
 /**
  * timer_deinit:
  * @ts: the timer to be de-initialised
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 2046b68c15..04527a343f 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -355,11 +355,6 @@ void timer_init_full(QEMUTimer *ts,
 ts->expire_time = -1;
 }

-int timer_get_attributes(QEMUTimer *ts)
-{
-return ts->attributes;
-}
-
 void timer_deinit(QEMUTimer *ts)
 {
 assert(ts->expire_time == -1);

Re: [Qemu-block] [PATCH v3 1/6] qemu-nbd: add support for authorization of TLS clients

[Juan Quintela]

Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> Currently any client which can complete the TLS handshake is able to use
> the NBD server. The server admin can turn on the 'verify-peer' option
> for the x509 creds to require the client to provide a x509 certificate.
> This means the client will have to acquire a certificate from the CA
> before they are permitted to use the NBD server. This is still a fairly
> low bar to cross.
>
> This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> takes the ID of a previously added 'QAuthZ' object instance. This will
> be used to validate the client's x509 distinguished name. Clients
> failing the authorization check will not be permitted to use the NBD
> server.
>
> For example to setup authorization that only allows connection from a client
> whose x509 certificate distinguished name is
>
>CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
>
> use:
>
>   qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
> endpoint=server,verify-peer=yes \
>--object authz-simple,id=auth0,identity=CN=laptop.example.com,,\
> O=Example Org,,L=London,,ST=London,,C=GB \
>--tls-creds tls0 \
>--tls-authz authz0
>  other qemu-nbd args...
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Juan Quintela 

Re: [Qemu-block] [PATCH v3 2/6] nbd: allow authorization with nbd-server-start QMP command

[Juan Quintela]

Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> As with the previous patch to qemu-nbd, the nbd-server-start QMP command
> also needs to be able to specify authorization when enabling TLS encryption.
>
> First the client must create a QAuthZ object instance using the
> 'object-add' command:
>
>{
>  'execute': 'object-add',
>  'arguments': {
>'qom-type': 'authz-list',
>'id': 'authz0',
>'parameters': {
>  'policy': 'deny',
>  'rules': [
>{
>  'match': '*CN=fred',
>  'policy': 'allow'
>}
>  ]
>}
>  }
>}
>
> They can then reference this in the new 'tls-authz' parameter when
> executing the 'nbd-server-start' command:
>
>{
>  'execute': 'nbd-server-start',
>  'arguments': {
>'addr': {
>'type': 'inet',
>'host': '127.0.0.1',
>'port': '9000'
>},
>'tls-creds': 'tls0',
>'tls-authz': 'authz0'
>  }
>}
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Juan Quintela 

similar to previous patch in series.

Re: [Qemu-block] [PATCH v3 3/6] migration: add support for a "tls-authz" migration parameter

[Juan Quintela]

Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> The QEMU instance that runs as the server for the migration data
> transport (ie the target QEMU) needs to be able to configure access
> control so it can prevent unauthorized clients initiating an incoming
> migration. This adds a new 'tls-authz' migration parameter that is used
> to provide the QOM ID of a QAuthZ subclass instance that provides the
> access control check. This is checked against the x509 certificate
> obtained during the TLS handshake.
>
> For example, when starting a QEMU for incoming migration, it is
> possible to give an example identity of the source QEMU that is
> intended to be connecting later:
>
>   $QEMU \
>  -monitor stdio \
>  -incoming defer \
>  ...other args...
>
>   (qemu) object_add tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
>  endpoint=server,verify-peer=yes \
>   (qemu) object_add authz-simple,id=auth0,identity=CN=laptop.example.com,,\
>  O=Example Org,,L=London,,ST=London,,C=GB \
>   (qemu) migrate_incoming tcp:localhost:9000
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Juan Quintela 

Re: [Qemu-block] [PATCH v3 4/6] chardev: add support for authorization for TLS clients

[Juan Quintela]

Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> Currently any client which can complete the TLS handshake is able to use
> a chardev server. The server admin can turn on the 'verify-peer' option
> for the x509 creds to require the client to provide a x509
> certificate. This means the client will have to acquire a certificate
> from the CA before they are permitted to use the chardev server. This is
> still a fairly low bar.
>
> This adds a 'tls-authz=OBJECT-ID' option to the socket chardev backend
> which takes the ID of a previously added 'QAuthZ' object instance. This
> will be used to validate the client's x509 distinguished name. Clients
> failing the check will not be permitted to use the chardev server.
>
> For example to setup authorization that only allows connection from a
> client whose x509 certificate distinguished name contains 'CN=fred', you
> would use:
>
>   $QEMU -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
> endpoint=server,verify-peer=yes \
> -object authz-simple,id=authz0,identity=CN=laptop.example.com,,\
> O=Example Org,,L=London,,ST=London,,C=GB \
> -chardev socket,host=127.0.0.1,port=9000,server,\
>tls-creds=tls0,tls-authz=authz0 \
> ...other qemu args...
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Juan Quintela 

[Qemu-block] [PATCH v2 09/14] block: Use bdrv_reopen_set_read_only() in the mirror driver

[Alberto Garcia]

The 'block-commit' QMP command is implemented internally using two
different drivers. If the source image is the active layer then the
mirror driver is used (commit_active_start()), otherwise the commit
driver is used (commit_start()).

In both cases the destination image must be put temporarily in
read-write mode. This is done correctly in the latter case, but what
commit_active_start() does is copy all flags instead.

This patch replaces the bdrv_reopen() calls in that function with
bdrv_reopen_set_read_only() so that only the read-only status is
changed.

A similar change is made in mirror_exit(), which is also used by the
'drive-mirror' and 'blockdev-mirror' commands.

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

diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..41b6cbaad6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -672,9 +672,10 @@ static int mirror_exit_common(Job *job)
 
 if (s->should_complete && !abort) {
 BlockDriverState *to_replace = s->to_replace ?: src;
+bool ro = bdrv_is_read_only(to_replace);
 
-if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
-bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
+if (ro != bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, ro, NULL);
 }
 
 /* The mirror job has no requests in flight any more, but we need to
@@ -1692,13 +1693,15 @@ void commit_active_start(const char *job_id, 
BlockDriverState *bs,
  BlockCompletionFunc *cb, void *opaque,
  bool auto_complete, Error **errp)
 {
-int orig_base_flags;
+bool base_read_only;
 Error *local_err = NULL;
 
-orig_base_flags = bdrv_get_flags(base);
+base_read_only = bdrv_is_read_only(base);
 
-if (bdrv_reopen(base, bs->open_flags, errp)) {
-return;
+if (base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
+return;
+}
 }
 
 mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
@@ -1717,6 +1720,8 @@ void commit_active_start(const char *job_id, 
BlockDriverState *bs,
 error_restore_flags:
 /* ignore error and errp for bdrv_reopen, because we want to propagate
  * the original error */
-bdrv_reopen(base, orig_base_flags, NULL);
+if (base_read_only) {
+bdrv_reopen_set_read_only(base, true, NULL);
+}
 return;
 }
-- 
2.11.0

[Qemu-block] [PATCH v2 05/14] block: Use bdrv_reopen_set_read_only() in bdrv_commit()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

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

diff --git a/block/commit.c b/block/commit.c
index a53c2d04b0..53148e610b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -391,7 +391,7 @@ int bdrv_commit(BlockDriverState *bs)
 BlockDriverState *commit_top_bs = NULL;
 BlockDriver *drv = bs->drv;
 int64_t offset, length, backing_length;
-int ro, open_flags;
+int ro;
 int64_t n;
 int ret = 0;
 uint8_t *buf = NULL;
@@ -410,10 +410,9 @@ int bdrv_commit(BlockDriverState *bs)
 }
 
 ro = bs->backing->bs->read_only;
-open_flags =  bs->backing->bs->open_flags;
 
 if (ro) {
-if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
+if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
 return -EACCES;
 }
 }
@@ -523,7 +522,7 @@ ro_cleanup:
 
 if (ro) {
 /* ignoring error return here */
-bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
+bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
 }
 
 return ret;
-- 
2.11.0

[Qemu-block] [PATCH v2 02/14] block: Add bdrv_reopen_set_read_only()

[Alberto Garcia]

Most callers of bdrv_reopen() only use it to switch a BlockDriverState
between read-only and read-write, so this patch adds a new function
that does just that.

We also want to get rid of the flags parameter in the bdrv_reopen()
API, so this function sets the "read-only" option and passes the
original flags (which will then be updated in bdrv_reopen_prepare()).

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block.c   | 17 +
 include/block/block.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/block.c b/block.c
index 47d5fe2f91..40c3a3d30b 100644
--- a/block.c
+++ b/block.c
@@ -3098,6 +3098,23 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, 
Error **errp)
 return ret;
 }
 
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+  Error **errp)
+{
+int ret;
+BlockReopenQueue *queue;
+QDict *opts = qdict_new();
+
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+bdrv_subtree_drained_begin(bs);
+queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+bdrv_subtree_drained_end(bs);
+
+return ret;
+}
+
 static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
   BdrvChild *c)
 {
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..97317fdfb6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -301,6 +301,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 QDict *options, int flags);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error 
**errp);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+  Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
-- 
2.11.0

[Qemu-block] [PATCH v2 13/14] block: Remove flags parameter from bdrv_reopen_queue()

[Alberto Garcia]

Now that all callers are passing all flag changes as QDict options,
the flags parameter is no longer necessary, so we can get rid of it.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block.c   | 5 +++--
 block/replication.c   | 6 ++
 include/block/block.h | 3 +--
 qemu-io-cmds.c| 2 +-
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 406c2d1da7..194200908d 100644
--- a/block.c
+++ b/block.c
@@ -3012,8 +3012,9 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
 BlockDriverState *bs,
-QDict *options, int flags)
+QDict *options)
 {
+int flags = bdrv_get_flags(bs);
 return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
NULL, NULL, 0);
 }
@@ -3087,7 +3088,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool 
read_only,
 qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
 
 bdrv_subtree_drained_begin(bs);
-queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+queue = bdrv_reopen_queue(NULL, bs, opts);
 ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
 bdrv_subtree_drained_end(bs);
 
diff --git a/block/replication.c b/block/replication.c
index 481a225924..fdbfe47fa4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -393,19 +393,17 @@ static void reopen_backing_file(BlockDriverState *bs, 
bool writable,
 bdrv_subtree_drained_begin(s->secondary_disk->bs);
 
 if (s->orig_hidden_read_only) {
-int flags = bdrv_get_flags(s->hidden_disk->bs);
 QDict *opts = qdict_new();
 qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
 reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
- opts, flags);
+ opts);
 }
 
 if (s->orig_secondary_read_only) {
-int flags = bdrv_get_flags(s->secondary_disk->bs);
 QDict *opts = qdict_new();
 qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
 reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- opts, flags);
+ opts);
 }
 
 if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index 3649136689..38d5c1adf4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -297,8 +297,7 @@ BlockDriverState *bdrv_open(const char *filename, const 
char *reference,
 BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs,
-QDict *options, int flags);
+BlockDriverState *bs, QDict *options);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error 
**errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
   Error **errp);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index b49a816cc8..e272e5c80b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 }
 
 bdrv_subtree_drained_begin(bs);
-brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+brq = bdrv_reopen_queue(NULL, bs, opts);
 bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
 bdrv_subtree_drained_end(bs);
 
-- 
2.11.0

[Qemu-block] [PATCH v2 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Alberto Garcia]

Now that all callers are passing the new options using the QDict we no
longer need the 'flags' parameter.

This patch makes the following changes:

   1) The update_options_from_flags() call is no longer necessary
  so it can be removed.

   2) The update_flags_from_options() call is now used in all cases,
  and is moved down a few lines so it happens after the options
  QDict contains the final set of values.

   3) The flags parameter is removed. Now the flags are initialized
  using the current value (for the top-level node) or the parent
  flags (after inherit_options()). In both cases the initial
  values are updated to reflect the new options in the QDict. This
  happens in bdrv_reopen_queue_child() (as explained above) and in
  bdrv_reopen_prepare().

Signed-off-by: Alberto Garcia 
---
 block.c | 62 ++
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/block.c b/block.c
index 194200908d..810b4083fe 100644
--- a/block.c
+++ b/block.c
@@ -2876,7 +2876,6 @@ BlockDriverState *bdrv_open(const char *filename, const 
char *reference,
 static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
  BlockDriverState *bs,
  QDict *options,
- int flags,
  const BdrvChildRole *role,
  QDict *parent_options,
  int parent_flags)
@@ -2885,7 +2884,10 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 BlockReopenQueueEntry *bs_entry;
 BdrvChild *child;
-QDict *old_options, *explicit_options;
+QDict *old_options, *explicit_options, *options_copy;
+int flags;
+QemuOpts *opts;
+Error *local_err = NULL;
 
 /* Make sure that the caller remembered to use a drained section. This is
  * important to avoid graph changes between the recursive queuing here and
@@ -2911,22 +2913,11 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 /*
  * Precedence of options:
  * 1. Explicitly passed in options (highest)
- * 2. Set in flags (only for top level)
- * 3. Retained from explicitly set options of bs
- * 4. Inherited from parent node
- * 5. Retained from effective options of bs
+ * 2. Retained from explicitly set options of bs
+ * 3. Inherited from parent node
+ * 4. Retained from effective options of bs
  */
 
-if (!parent_options) {
-/*
- * Any setting represented by flags is always updated. If the
- * corresponding QDict option is set, it takes precedence. Otherwise
- * the flag is translated into a QDict option. The old setting of bs is
- * not considered.
- */
-update_options_from_flags(options, flags);
-}
-
 /* Old explicitly set values (don't overwrite by inherited value) */
 if (bs_entry) {
 old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
@@ -2940,23 +2931,10 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 /* Inherit from parent node */
 if (parent_options) {
-QemuOpts *opts;
-QDict *options_copy;
-Error *local_err = NULL;
-assert(!flags);
+flags = 0;
 role->inherit_options(&flags, options, parent_flags, parent_options);
-options_copy = qdict_clone_shallow(options);
-opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options_copy, &local_err);
-/* Don't call update_flags_from_options() if the options are wrong.
- * bdrv_reopen_prepare() will later return an error. */
-if (!local_err) {
-update_flags_from_options(&flags, opts);
-} else {
-error_free(local_err);
-}
-qemu_opts_del(opts);
-qobject_unref(options_copy);
+} else {
+flags = bdrv_get_flags(bs);
 }
 
 /* Old values are used for options that aren't set yet */
@@ -2964,6 +2942,20 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 bdrv_join_options(bs, options, old_options);
 qobject_unref(old_options);
 
+/* We have the final set of options so let's update the flags */
+options_copy = qdict_clone_shallow(options);
+opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+qemu_opts_absorb_qdict(opts, options_copy, &local_err);
+/* Don't call update_flags_from_options() if the options are wrong.
+ * bdrv_reopen_prepare() will later return an error. */
+if (!local_err) {
+update_flags_from_options(&flags, opts);
+} else {
+error_free(local_err);
+}
+qemu_opts_del(opts)

[Qemu-block] [PATCH v2 03/14] block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

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

diff --git a/block.c b/block.c
index 40c3a3d30b..39d40f4556 100644
--- a/block.c
+++ b/block.c
@@ -1058,11 +1058,11 @@ static int bdrv_backing_update_filename(BdrvChild *c, 
BlockDriverState *base,
 const char *filename, Error **errp)
 {
 BlockDriverState *parent = c->opaque;
-int orig_flags = bdrv_get_flags(parent);
+bool read_only = bdrv_is_read_only(parent);
 int ret;
 
-if (!(orig_flags & BDRV_O_RDWR)) {
-ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp);
+if (read_only) {
+ret = bdrv_reopen_set_read_only(parent, false, errp);
 if (ret < 0) {
 return ret;
 }
@@ -1074,8 +1074,8 @@ static int bdrv_backing_update_filename(BdrvChild *c, 
BlockDriverState *base,
 error_setg_errno(errp, -ret, "Could not update backing file link");
 }
 
-if (!(orig_flags & BDRV_O_RDWR)) {
-bdrv_reopen(parent, orig_flags, NULL);
+if (read_only) {
+bdrv_reopen_set_read_only(parent, true, NULL);
 }
 
 return ret;
-- 
2.11.0

[Qemu-block] [PATCH v2 06/14] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

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

diff --git a/block/stream.c b/block/stream.c
index 81a7ec8ece..262d280ccd 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
 BlockDriverState *base;
 BlockdevOnError on_error;
 char *backing_file_str;
-int bs_flags;
+bool bs_read_only;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -89,10 +89,10 @@ static void stream_clean(Job *job)
 BlockDriverState *bs = blk_bs(bjob->blk);
 
 /* Reopen the image back in read-only mode if necessary */
-if (s->bs_flags != bdrv_get_flags(bs)) {
+if (s->bs_read_only) {
 /* Give up write permissions before making it read-only */
 blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-bdrv_reopen(bs, s->bs_flags, NULL);
+bdrv_reopen_set_read_only(bs, true, NULL);
 }
 
 g_free(s->backing_file_str);
@@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState 
*bs,
 {
 StreamBlockJob *s;
 BlockDriverState *iter;
-int orig_bs_flags;
+int bs_read_only;
 
 /* Make sure that the image is opened in read-write mode */
-orig_bs_flags = bdrv_get_flags(bs);
-if (!(orig_bs_flags & BDRV_O_RDWR)) {
-if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+bs_read_only = bdrv_is_read_only(bs);
+if (bs_read_only) {
+if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
 return;
 }
 }
@@ -261,7 +261,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 
 s->base = base;
 s->backing_file_str = g_strdup(backing_file_str);
-s->bs_flags = orig_bs_flags;
+s->bs_read_only = bs_read_only;
 
 s->on_error = on_error;
 trace_stream_start(bs, base, s);
@@ -269,7 +269,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 return;
 
 fail:
-if (orig_bs_flags != bdrv_get_flags(bs)) {
-bdrv_reopen(bs, orig_bs_flags, NULL);
+if (bs_read_only) {
+bdrv_reopen_set_read_only(bs, true, NULL);
 }
 }
-- 
2.11.0

[Qemu-block] [PATCH v2 04/14] block: Use bdrv_reopen_set_read_only() in commit_start/complete()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

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

diff --git a/block/commit.c b/block/commit.c
index a2da5740b0..a53c2d04b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -38,7 +38,7 @@ typedef struct CommitBlockJob {
 BlockBackend *base;
 BlockDriverState *base_bs;
 BlockdevOnError on_error;
-int base_flags;
+bool base_read_only;
 char *backing_file_str;
 } CommitBlockJob;
 
@@ -124,8 +124,8 @@ static void commit_clean(Job *job)
 /* restore base open flags here if appropriate (e.g., change the base back
  * to r/o). These reopens do not need to be atomic, since we won't abort
  * even on failure here */
-if (s->base_flags != bdrv_get_flags(s->base_bs)) {
-bdrv_reopen(s->base_bs, s->base_flags, NULL);
+if (s->base_read_only) {
+bdrv_reopen_set_read_only(s->base_bs, true, NULL);
 }
 
 g_free(s->backing_file_str);
@@ -264,7 +264,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
   const char *filter_node_name, Error **errp)
 {
 CommitBlockJob *s;
-int orig_base_flags;
 BlockDriverState *iter;
 BlockDriverState *commit_top_bs = NULL;
 Error *local_err = NULL;
@@ -283,11 +282,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 }
 
 /* convert base to r/w, if necessary */
-orig_base_flags = bdrv_get_flags(base);
-if (!(orig_base_flags & BDRV_O_RDWR)) {
-bdrv_reopen(base, orig_base_flags | BDRV_O_RDWR, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+s->base_read_only = bdrv_is_read_only(base);
+if (s->base_read_only) {
+if (bdrv_reopen_set_read_only(base, false, errp) != 0) {
 goto fail;
 }
 }
@@ -363,7 +360,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 goto fail;
 }
 
-s->base_flags = orig_base_flags;
 s->backing_file_str = g_strdup(backing_file_str);
 s->on_error = on_error;
 
-- 
2.11.0

[Qemu-block] [PATCH v2 08/14] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()

[Alberto Garcia]

This patch replaces the bdrv_reopen() call that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f64569795c..7a57572e1a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1701,8 +1701,7 @@ static void external_snapshot_commit(BlkActionState 
*common)
  * bdrv_reopen_multiple() across all the entries at once, because we
  * don't want to abort all of them if one of them fails the reopen */
 if (!atomic_read(&state->old_bs->copy_on_read)) {
-bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-NULL);
+bdrv_reopen_set_read_only(state->old_bs, true, NULL);
 }
 
 aio_context_release(aio_context);
-- 
2.11.0

[Qemu-block] [PATCH v2 07/14] block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

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

diff --git a/blockdev.c b/blockdev.c
index a8755bd908..f64569795c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4064,7 +4064,6 @@ void qmp_change_backing_file(const char *device,
 BlockDriverState *image_bs = NULL;
 Error *local_err = NULL;
 bool ro;
-int open_flags;
 int ret;
 
 bs = qmp_get_root_bs(device, errp);
@@ -4106,13 +4105,10 @@ void qmp_change_backing_file(const char *device,
 }
 
 /* if not r/w, reopen to make r/w */
-open_flags = image_bs->open_flags;
 ro = bdrv_is_read_only(image_bs);
 
 if (ro) {
-bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (bdrv_reopen_set_read_only(image_bs, false, errp) != 0) {
 goto out;
 }
 }
@@ -4128,7 +4124,7 @@ void qmp_change_backing_file(const char *device,
 }
 
 if (ro) {
-bdrv_reopen(image_bs, open_flags, &local_err);
+bdrv_reopen_set_read_only(image_bs, true, &local_err);
 error_propagate(errp, local_err);
 }
 
-- 
2.11.0

[Qemu-block] [PATCH v2 01/14] block: Don't call update_flags_from_options() if the options are wrong

[Alberto Garcia]

If qemu_opts_absorb_qdict() fails and we carry on and call
update_flags_from_options() then that can result on a failed
assertion:

   $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
   block.c:1101: update_flags_from_options: Assertion `qemu_opt_find(opts, 
BDRV_OPT_CACHE_DIRECT)' failed.
   Aborted

This only happens in bdrv_reopen_queue_child(). Although this function
doesn't return errors, we can simply keep the wrong options in the
reopen queue and later bdrv_reopen_prepare() will fail.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block.c| 11 +--
 tests/qemu-iotests/133 |  6 ++
 tests/qemu-iotests/133.out |  4 
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 7710b399a3..47d5fe2f91 100644
--- a/block.c
+++ b/block.c
@@ -2942,12 +2942,19 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 if (parent_options) {
 QemuOpts *opts;
 QDict *options_copy;
+Error *local_err = NULL;
 assert(!flags);
 role->inherit_options(&flags, options, parent_flags, parent_options);
 options_copy = qdict_clone_shallow(options);
 opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options_copy, NULL);
-update_flags_from_options(&flags, opts);
+qemu_opts_absorb_qdict(opts, options_copy, &local_err);
+/* Don't call update_flags_from_options() if the options are wrong.
+ * bdrv_reopen_prepare() will later return an error. */
+if (!local_err) {
+update_flags_from_options(&flags, opts);
+} else {
+error_free(local_err);
+}
 qemu_opts_del(opts);
 qobject_unref(options_copy);
 }
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index af6b3e1dd4..b9f17c996f 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -92,6 +92,12 @@ echo
 IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
 "json:{'driver': 'null-co', 'size': 65536}"
 
+echo
+echo "=== Check that invalid options are handled correctly ==="
+echo
+
+$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index f4a85aeb63..570f623b6b 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -24,4 +24,8 @@ Cannot change the option 'driver'
 
 format name: null-co
 format name: null-co
+
+=== Check that invalid options are handled correctly ===
+
+Parameter 'read-only' expects 'on' or 'off'
 *** done
-- 
2.11.0

[Qemu-block] [PATCH v2 11/14] qemu-io: Put flag changes in the options QDict in reopen_f()

[Alberto Garcia]

When reopen_f() puts a block device in the reopen queue, some of the
new options are passed using a QDict, but others ("read-only" and the
cache options) are passed as flags.

This patch puts those flags in the QDict. This way the flags parameter
becomes redundant and we'll be able to get rid of it in a subsequent
patch.

Signed-off-by: Alberto Garcia 
---
 qemu-io-cmds.c | 27 ++-
 tests/qemu-iotests/133 |  9 +
 tests/qemu-iotests/133.out |  8 
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index db0b3ee5ef..b49a816cc8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu-io.h"
 #include "sysemu/block-backend.h"
 #include "block/block.h"
@@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 int flags = bs->open_flags;
 bool writethrough = !blk_enable_write_cache(blk);
 bool has_rw_option = false;
+bool has_cache_option = false;
 
 BlockReopenQueue *brq;
 Error *local_err = NULL;
@@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 error_report("Invalid cache option: %s", optarg);
 return -EINVAL;
 }
+has_cache_option = true;
 break;
 case 'o':
 if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
@@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 }
 
 qopts = qemu_opts_find(&reopen_opts, NULL);
-opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
 qemu_opts_reset(&reopen_opts);
 
+if (qdict_haskey(opts, BDRV_OPT_READ_ONLY)) {
+if (has_rw_option) {
+error_report("Cannot set both -r/-w and '" BDRV_OPT_READ_ONLY "'");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
+}
+
+if (qdict_haskey(opts, BDRV_OPT_CACHE_DIRECT) ||
+qdict_haskey(opts, BDRV_OPT_CACHE_NO_FLUSH)) {
+if (has_cache_option) {
+error_report("Cannot set both -c and the cache options");
+qobject_unref(opts);
+return -EINVAL;
+}
+} else {
+qdict_put_bool(opts, BDRV_OPT_CACHE_DIRECT, flags & BDRV_O_NOCACHE);
+qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
+}
+
 bdrv_subtree_drained_begin(bs);
 brq = bdrv_reopen_queue(NULL, bs, opts, flags);
 bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index b9f17c996f..3cbe72c42b 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -98,6 +98,15 @@ echo
 
 $QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
 
+echo
+echo "=== Check that mixing -c/-r/-w and their corresponding options is 
forbidden ==="
+echo
+
+$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index 570f623b6b..aa8cf19f9e 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -28,4 +28,12 @@ format name: null-co
 === Check that invalid options are handled correctly ===
 
 Parameter 'read-only' expects 'on' or 'off'
+
+=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===
+
+Cannot set both -r/-w and 'read-only'
+Cannot set both -r/-w and 'read-only'
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
 *** done
-- 
2.11.0

[Qemu-block] [PATCH v2 00/14] Don't pass flags to bdrv_reopen_queue()

[Alberto Garcia]

Hi all,

when reopening a BlockDriverState using bdrv_reopen() and friends the
new options can be specified either with a QDict or with flags. Both
methods overlap and that makes the semantics and the implementation
unnecessarily complicated.

This series removes the 'flags' parameter from these functions, so
from now on all option changes must be specified using a QDict. Apart
from simplifying the API, a few bugs are fixed along the way. See the
individual patches for more details.

This was tested with the current master (09558375a634e17cea6cfbfec88).

Regards,

Berto

v2:
- Patches 4 and 9: Fixed trivial rebase conflict
- Patch 11: Update messages [Max]
- Patch 12: Add comment and use bdrv_is_read_only() instead of
using the flags directly [Max]
- Patch 14: Remove inner block and move all variable declarations to
the beginning of the function [Max]

v1: https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00483.html
- Initial version

Output of backport-diff against v1:

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/14:[] [--] 'block: Don't call update_flags_from_options() if the 
options are wrong'
002/14:[] [--] 'block: Add bdrv_reopen_set_read_only()'
003/14:[] [--] 'block: Use bdrv_reopen_set_read_only() in 
bdrv_backing_update_filename()'
004/14:[0006] [FC] 'block: Use bdrv_reopen_set_read_only() in 
commit_start/complete()'
005/14:[] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_commit()'
006/14:[] [-C] 'block: Use bdrv_reopen_set_read_only() in 
stream_start/complete()'
007/14:[] [--] 'block: Use bdrv_reopen_set_read_only() in 
qmp_change_backing_file()'
008/14:[] [--] 'block: Use bdrv_reopen_set_read_only() in 
external_snapshot_commit()'
009/14:[0003] [FC] 'block: Use bdrv_reopen_set_read_only() in the mirror driver'
010/14:[] [--] 'block: Drop bdrv_reopen()'
011/14:[0018] [FC] 'qemu-io: Put flag changes in the options QDict in 
reopen_f()'
012/14:[0011] [FC] 'block: Clean up reopen_backing_file() in 
block/replication.c'
013/14:[] [--] 'block: Remove flags parameter from bdrv_reopen_queue()'
014/14:[0042] [FC] 'block: Stop passing flags to bdrv_reopen_queue_child()'

Alberto Garcia (14):
  block: Don't call update_flags_from_options() if the options are wrong
  block: Add bdrv_reopen_set_read_only()
  block: Use bdrv_reopen_set_read_only() in
bdrv_backing_update_filename()
  block: Use bdrv_reopen_set_read_only() in commit_start/complete()
  block: Use bdrv_reopen_set_read_only() in bdrv_commit()
  block: Use bdrv_reopen_set_read_only() in stream_start/complete()
  block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
  block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
  block: Use bdrv_reopen_set_read_only() in the mirror driver
  block: Drop bdrv_reopen()
  qemu-io: Put flag changes in the options QDict in reopen_f()
  block: Clean up reopen_backing_file() in block/replication.c
  block: Remove flags parameter from bdrv_reopen_queue()
  block: Stop passing flags to bdrv_reopen_queue_child()

 block.c| 86 +-
 block/commit.c | 23 +
 block/mirror.c | 19 ++
 block/replication.c| 43 ++-
 block/stream.c | 20 +--
 blockdev.c | 11 ++
 include/block/block.h  |  6 ++--
 qemu-io-cmds.c | 29 ++--
 tests/qemu-iotests/133 | 15 
 tests/qemu-iotests/133.out | 12 +++
 10 files changed, 150 insertions(+), 114 deletions(-)

-- 
2.11.0

[Qemu-block] [PATCH v2 12/14] block: Clean up reopen_backing_file() in block/replication.c

[Alberto Garcia]

This function is used to put the hidden and secondary disks in
read-write mode before launching the backup job, and back in read-only
mode afterwards.

This patch does the following changes:

  - Use an options QDict with the "read-only" option instead of
passing the changes as flags only.

  - Simplify the code (it was unnecessarily complicated and verbose).

  - Fix a bug due to which the secondary disk was not being put back
in read-only mode when writable=false (because in this case
orig_secondary_flags always had the BDRV_O_RDWR flag set).

  - Stop clearing the BDRV_O_INACTIVE flag.

The flags parameter to bdrv_reopen_queue() becomes redundant and we'll
be able to get rid of it in a subsequent patch.

Signed-off-by: Alberto Garcia 
---
 block/replication.c | 45 +
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 6349d6958e..481a225924 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -20,6 +20,7 @@
 #include "block/block_backup.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "replication.h"
 
 typedef enum {
@@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {
 char *top_id;
 ReplicationState *rs;
 Error *blocker;
-int orig_hidden_flags;
-int orig_secondary_flags;
+bool orig_hidden_read_only;
+bool orig_secondary_read_only;
 int error;
 } BDRVReplicationState;
 
@@ -371,44 +372,40 @@ static void secondary_do_checkpoint(BDRVReplicationState 
*s, Error **errp)
 }
 }
 
+/* This function is supposed to be called twice:
+ * first with writable = true, then with writable = false.
+ * The first call puts s->hidden_disk and s->secondary_disk in
+ * r/w mode, and the second puts them back in their original state.
+ */
 static void reopen_backing_file(BlockDriverState *bs, bool writable,
 Error **errp)
 {
 BDRVReplicationState *s = bs->opaque;
 BlockReopenQueue *reopen_queue = NULL;
-int orig_hidden_flags, orig_secondary_flags;
-int new_hidden_flags, new_secondary_flags;
 Error *local_err = NULL;
 
 if (writable) {
-orig_hidden_flags = s->orig_hidden_flags =
-bdrv_get_flags(s->hidden_disk->bs);
-new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-orig_secondary_flags = s->orig_secondary_flags =
-bdrv_get_flags(s->secondary_disk->bs);
-new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) &
- ~BDRV_O_INACTIVE;
-} else {
-orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_hidden_flags = s->orig_hidden_flags;
-orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) &
-~BDRV_O_INACTIVE;
-new_secondary_flags = s->orig_secondary_flags;
+s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
+s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
 }
 
 bdrv_subtree_drained_begin(s->hidden_disk->bs);
 bdrv_subtree_drained_begin(s->secondary_disk->bs);
 
-if (orig_hidden_flags != new_hidden_flags) {
-reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, 
NULL,
- new_hidden_flags);
+if (s->orig_hidden_read_only) {
+int flags = bdrv_get_flags(s->hidden_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
+reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+ opts, flags);
 }
 
-if (!(orig_secondary_flags & BDRV_O_RDWR)) {
+if (s->orig_secondary_read_only) {
+int flags = bdrv_get_flags(s->secondary_disk->bs);
+QDict *opts = qdict_new();
+qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
 reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- NULL, new_secondary_flags);
+ opts, flags);
 }
 
 if (reopen_queue) {
-- 
2.11.0

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

Yes, but without words ..."when they fire" in attributes comments.

ср, 17 окт. 2018 г. в 17:24, Paolo Bonzini :

> On 17/10/2018 12:57, Artem Pisarenko wrote:
> >> Further down in this patch the notation is QEMU_TIMER_ATTR_, which I
> >> think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
> >> macro.  Please use the QEMU_TIMER_ATTR_ notation consistently.
> >
> > Yes, I've just forgot to update comments after previous patch version,
> > where it actually was macro.
> >
> >> What is the purpose of this bit?  I guess it's just here as a
> >> placeholder because no real bits have been defined yet.  Hopefully the
> >> next patch removes it (/* This placeholder is removed in the next patch
> >> */ would be a nice way to document this for reviewers).
> >
> > It's just to prevent compilation errors, as required by
> > https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
> >
> >> The enum isn't needed and makes debugging harder since the bit number is
> >> implicit in the enum ordering.  This alternative is clearer and more
> >> concise:
> >>
> >>   #define QEMU_TIMER_ATTR_foo BIT(n)
> >
> > Agree.
>
> Like this?
>
> ...
>
> --

С уважением,
  Артем Писаренко

[Qemu-block] [PATCH v2 10/14] block: Drop bdrv_reopen()

[Alberto Garcia]

No one is using this function anymore, so we can safely remove it.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block.c   | 21 -
 include/block/block.h |  1 -
 2 files changed, 22 deletions(-)

diff --git a/block.c b/block.c
index 39d40f4556..406c2d1da7 100644
--- a/block.c
+++ b/block.c
@@ -3077,27 +3077,6 @@ cleanup:
 return ret;
 }
 
-
-/* Reopen a single BlockDriverState with the specified flags. */
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
-{
-int ret = -1;
-Error *local_err = NULL;
-BlockReopenQueue *queue;
-
-bdrv_subtree_drained_begin(bs);
-
-queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
-ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-}
-
-bdrv_subtree_drained_end(bs);
-
-return ret;
-}
-
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
   Error **errp)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 97317fdfb6..3649136689 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -300,7 +300,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 BlockDriverState *bs,
 QDict *options, int flags);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error 
**errp);
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
   Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
-- 
2.11.0

[Qemu-block] [PATCH] vpc: Don't leak opts in vpc_open()

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index bf294abfa7..eace2ba484 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -454,10 +454,12 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 qemu_co_mutex_init(&s->lock);
+qemu_opts_del(opts);
 
 return 0;
 
 fail:
+qemu_opts_del(opts);
 qemu_vfree(s->pagetable);
 #ifdef CACHE
 g_free(s->pageentry_u8);
-- 
2.19.1

[Qemu-block] [PATCH 1/3] qemu-iotests: Modern shellscripting (use $() instead of ``)

[Mao Zhongyi]

Various shell files contain a mix between obsolete ``
and modern $(); It would be nice to convert to using $()
everywhere.

`pwd` and `basename $0` are in 231 files under directory
tests/qemu-iotests, so replaced it with the following:

sed -i 's/`pwd`/$(pwd)/g' $(git grep -l "\`pwd\`")
sed -i 's/`basename $0`/$(basename $0)/g' $(git grep -l "basename \$0")

A small amount of the rest is manually modified.

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: qemu-block@nongnu.org

Signed-off-by: Mao Zhongyi 
---
 tests/qemu-iotests/001|  4 +-
 tests/qemu-iotests/002|  4 +-
 tests/qemu-iotests/003|  4 +-
 tests/qemu-iotests/004|  4 +-
 tests/qemu-iotests/005|  4 +-
 tests/qemu-iotests/007|  6 +--
 tests/qemu-iotests/008|  4 +-
 tests/qemu-iotests/009|  4 +-
 tests/qemu-iotests/010|  4 +-
 tests/qemu-iotests/011|  6 +--
 tests/qemu-iotests/012|  4 +-
 tests/qemu-iotests/013|  4 +-
 tests/qemu-iotests/014|  6 +--
 tests/qemu-iotests/015|  4 +-
 tests/qemu-iotests/017|  4 +-
 tests/qemu-iotests/018|  4 +-
 tests/qemu-iotests/019|  4 +-
 tests/qemu-iotests/020|  4 +-
 tests/qemu-iotests/021|  4 +-
 tests/qemu-iotests/022|  4 +-
 tests/qemu-iotests/023|  4 +-
 tests/qemu-iotests/024|  4 +-
 tests/qemu-iotests/025|  4 +-
 tests/qemu-iotests/026|  4 +-
 tests/qemu-iotests/027|  4 +-
 tests/qemu-iotests/028|  4 +-
 tests/qemu-iotests/029|  4 +-
 tests/qemu-iotests/031|  4 +-
 tests/qemu-iotests/032|  4 +-
 tests/qemu-iotests/033|  4 +-
 tests/qemu-iotests/034|  4 +-
 tests/qemu-iotests/035|  4 +-
 tests/qemu-iotests/036|  4 +-
 tests/qemu-iotests/037|  4 +-
 tests/qemu-iotests/038|  4 +-
 tests/qemu-iotests/039|  4 +-
 tests/qemu-iotests/042|  4 +-
 tests/qemu-iotests/043|  4 +-
 tests/qemu-iotests/046|  4 +-
 tests/qemu-iotests/047|  4 +-
 tests/qemu-iotests/048|  2 +-
 tests/qemu-iotests/049|  4 +-
 tests/qemu-iotests/050|  4 +-
 tests/qemu-iotests/051|  4 +-
 tests/qemu-iotests/052|  4 +-
 tests/qemu-iotests/053|  4 +-
 tests/qemu-iotests/054|  4 +-
 tests/qemu-iotests/058|  4 +-
 tests/qemu-iotests/059|  4 +-
 tests/qemu-iotests/061|  4 +-
 tests/qemu-iotests/062|  4 +-
 tests/qemu-iotests/063|  4 +-
 tests/qemu-iotests/064|  4 +-
 tests/qemu-iotests/067|  4 +-
 tests/qemu-iotests/070|  4 +-
 tests/qemu-iotests/073|  4 +-
 tests/qemu-iotests/074|  2 +-
 tests/qemu-iotests/075|  4 +-
 tests/qemu-iotests/076|  4 +-
 tests/qemu-iotests/077|  4 +-
 tests/qemu-iotests/078|  4 +-
 tests/qemu-iotests/079|  4 +-
 tests/qemu-iotests/080|  4 +-
 tests/qemu-iotests/081|  4 +-
 tests/qemu-iotests/082|  4 +-
 tests/qemu-iotests/083|  4 +-
 tests/qemu-iotests/084|  4 +-
 tests/qemu-iotests/085|  4 +-
 tests/qemu-iotests/086|  4 +-
 tests/qemu-iotests/087|  4 +-
 tests/qemu-iotests/088|  4 +-
 tests/qemu-iotests/091|  4 +-
 tests/qemu-iotests/092|  4 +-
 tests/qemu-iotests/095|  4 +-
 tests/qemu-iotests/101|  4 +-
 tests/qemu-iotests/104|  4 +-
 tests/qemu-iotests/105|  4 +-
 tests/qemu-iotests/116|  4 +-
 tests/qemu-iotests/128|  4 +-
 tests/qemu-iotests/131|  4 +-
 tests/qemu-iotests/133|  4 +-
 tests/qemu-iotests/134|  4 +-
 tests/qemu-iotests/135|  4 +-
 tests/qemu-iotests/142|  4 +-
 tests/qemu-iotests/144|  4 +-
 tests/qemu-iotests/145|  4 +-
 tests/qemu-iotests/146|  4 +-
 tests/qemu-iotests/154|  4 +-
 tests/qemu-iotests/158|  4 +-
 tests/qemu-iotests/171|  4 +-
 tests/qemu-iotests/172|  4 +-
 tests/qemu-iotests/173|  4 +-
 tests/qemu-iotests/174|  4 +-
 tests/qemu-iotests/175|  4 +-
 tests/qemu-iotests/177|  4 +-
 tests/qemu-iotests/178|  4 +-
 tests/qemu-iotests/181|  4 +-
 tests/qemu-iotests/183|  4 +-
 tests/qemu-iotests/184|  4 +-
 tests/qemu-iotests/185|  4 +-
 tests/qemu-iotests/186|  4 +-
 tests/qemu-iotests/187|  4 +-
 tests/qemu-iotests/188|  4 +-
 tests/qemu-iotests/189|  4 +-
 tests/qemu

[Qemu-block] [PATCH 1/3] quorum: Remove quorum_err()

[Alberto Garcia]

This is a static function with only one caller, so there's no need to
keep it. Inlining the code in quorum_compare() makes it much simpler.

Signed-off-by: Alberto Garcia 
Reported-by: Markus Armbruster 
---
 block/quorum.c | 24 +---
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index eb526cc0f1..b1b777baef 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -437,23 +437,7 @@ static bool quorum_iovec_compare(QEMUIOVector *a, 
QEMUIOVector *b)
 return true;
 }
 
-static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB *acb,
-  const char *fmt, ...)
-{
-va_list ap;
-
-va_start(ap, fmt);
-fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ",
-acb->offset, acb->bytes);
-vfprintf(stderr, fmt, ap);
-fprintf(stderr, "\n");
-va_end(ap);
-exit(1);
-}
-
-static bool quorum_compare(QuorumAIOCB *acb,
-   QEMUIOVector *a,
-   QEMUIOVector *b)
+static bool quorum_compare(QuorumAIOCB *acb, QEMUIOVector *a, QEMUIOVector *b)
 {
 BDRVQuorumState *s = acb->bs->opaque;
 ssize_t offset;
@@ -462,8 +446,10 @@ static bool quorum_compare(QuorumAIOCB *acb,
 if (s->is_blkverify) {
 offset = qemu_iovec_compare(a, b);
 if (offset != -1) {
-quorum_err(acb, "contents mismatch at offset %" PRIu64,
-   acb->offset + offset);
+fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64
+" contents mismatch at offset %" PRIu64 "\n",
+acb->offset, acb->bytes, acb->offset + offset);
+exit(1);
 }
 return true;
 }
-- 
2.11.0

[Qemu-block] [PATCH 3/3] iotest: Test the blkverify mode of the Quorum driver

[Alberto Garcia]

Signed-off-by: Alberto Garcia 
---
 tests/qemu-iotests/081 | 30 ++
 tests/qemu-iotests/081.out | 16 
 2 files changed, 46 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index da3fb0984b..0ea010afbf 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -168,6 +168,36 @@ echo "== checking that quorum is broken =="
 
 $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
 
+echo
+echo "== checking the blkverify mode with broken content =="
+
+quorum="driver=raw,file.driver=quorum,file.vote-threshold=2,file.blkverify=on"
+quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
+quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
+quorum="$quorum,file.children.0.driver=raw"
+quorum="$quorum,file.children.1.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== writing the same data to both files =="
+
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with valid content =="
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with invalid settings =="
+
+quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw"
+quorum="$quorum,file.children.2.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 2533c31c78..2f12c890e9 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -55,4 +55,20 @@ wrote 10485760/10485760 bytes at offset 0
 
 == checking that quorum is broken ==
 read failed: Input/output error
+
+== checking the blkverify mode with broken content ==
+quorum: offset=0 bytes=10485760 contents mismatch at offset 0
+
+== writing the same data to both files ==
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with valid content ==
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with invalid settings ==
+can't open: blkverify=on can only be set if there are exactly two files and 
vote-threshold is 2
 *** done
-- 
2.11.0

[Qemu-block] [PATCH 0/3] Misc Quorum fixes

[Alberto Garcia]

Hi,

Here's a couple of small fixes for the Quorum driver plus new test
cases. See the individual patches for details.

Regards,

Berto

Alberto Garcia (3):
  quorum: Remove quorum_err()
  quorum: Return an error if the blkverify mode has invalid settings
  iotest: Test the blkverify mode of the Quorum driver

 block/quorum.c | 37 +++--
 tests/qemu-iotests/081 | 30 ++
 tests/qemu-iotests/081.out | 16 
 3 files changed, 57 insertions(+), 26 deletions(-)

-- 
2.11.0

Re: [Qemu-block] [PATCH] block/vhdx: Don't take address of fields in packed structs

[Kevin Wolf]

Am 16.10.2018 um 19:09 hat Peter Maydell geschrieben:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> Signed-off-by: Peter Maydell 
> ---
> Usual disclaimer: produced with "make check" only, but purely
> automated conversion should be safe.

More relevant, qemu-iotests for vhdx passes, too. I don't think "make
check" would run any of the modified code.

Thanks, applied to the block branch

Kevin

[Qemu-block] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Alberto Garcia]

The blkverify mode of Quorum can only be enabled if the number of
children is exactly two and the value of vote-threshold is also two.

If the user tries to enable it but the other settings are incorrect
then QEMU simply prints an error message to stderr and carries on
disabling the blkverify setting.

This patch makes quorum_open() fail and return an error in this case.

Signed-off-by: Alberto Garcia 
Reported-by: Markus Armbruster 
---
 block/quorum.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index b1b777baef..6188ff 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -912,13 +912,12 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->read_pattern = ret;
 
 if (s->read_pattern == QUORUM_READ_PATTERN_QUORUM) {
-/* is the driver in blkverify mode */
-if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) &&
-s->num_children == 2 && s->threshold == 2) {
-s->is_blkverify = true;
-} else if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false)) {
-fprintf(stderr, "blkverify mode is set by setting blkverify=on "
-"and using two files with vote_threshold=2\n");
+s->is_blkverify = qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false);
+if (s->is_blkverify && (s->num_children != 2 || s->threshold != 2)) {
+error_setg(&local_err, "blkverify=on can only be set if there are "
+   "exactly two files and vote-threshold is 2");
+ret = -EINVAL;
+goto exit;
 }
 
 s->rewrite_corrupted = qemu_opt_get_bool(opts, QUORUM_OPT_REWRITE,
-- 
2.11.0

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 17/10/2018 15:07, Artem Pisarenko wrote:
> Yes, but without words ..."when they fire" in attributes comments.

For now the only attribute applies when timers fire; that sentence was a
way to clarify the intended scope and address Stefan's comment.  I can
remove it too, but I'd like to understand if you have other ideas for
attributes.

Paolo

Re: [Qemu-block] [PATCH] block/vdi: Don't take address of fields in packed structs

[Kevin Wolf]

Am 16.10.2018 um 19:25 hat Peter Maydell geschrieben:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> There are other places where we take the address of a packed member
> in this file for other purposes than passing it to a byteswap
> function (all the calls to qemu_uuid_*()); we leave those for now.
> 
> Signed-off-by: Peter Maydell 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

>ср, 17 окт. 2018 г. в 20:43, Paolo Bonzini:
>On 17/10/2018 15:07, Artem Pisarenko wrote:
>> Yes, but without words ..."when they fire" in attributes comments.
>
> For now the only attribute applies when timers fire; that sentence was a
> way to clarify the intended scope and address Stefan's comment.  I can
> remove it too, but I'd like to understand if you have other ideas for
> attributes.

No, I haven't. I don't think that this partcular narrowing has any
relevance to Stefan's comment, so I just considered it to be unnecessary. I
don't insist.
-- 

С уважением,
  Артем Писаренко

Re: [Qemu-block] [PATCH] vpc: Don't leak opts in vpc_open()

[Alberto Garcia]

On Wed 17 Oct 2018 03:33:50 PM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Kevin Wolf]

Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> The blkverify mode of Quorum can only be enabled if the number of
> children is exactly two and the value of vote-threshold is also two.
> 
> If the user tries to enable it but the other settings are incorrect
> then QEMU simply prints an error message to stderr and carries on
> disabling the blkverify setting.
> 
> This patch makes quorum_open() fail and return an error in this case.
> 
> Signed-off-by: Alberto Garcia 
> Reported-by: Markus Armbruster 

While reviewing this, I think I found another problem: Shouldn't
.bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
probably assert in .bdrv_del_child that s->is_blkverify == false after
checking s->num_children <= s->threshold)

Kevin

Re: [Qemu-block] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations

[Anton Nefedov]

On 8/10/2018 7:43 PM, Kevin Wolf wrote:
> Am 08.10.2018 um 18:04 hat Anton Nefedov geschrieben:
>>
>>
>> On 8/10/2018 6:46 PM, Kevin Wolf wrote:
>>> Am 08.10.2018 um 17:25 hat Anton Nefedov geschrieben:


 On 8/10/2018 6:03 PM, Kevin Wolf wrote:
> Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben:
>> On 4/10/2018 6:33 PM, Kevin Wolf wrote:
>>> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
 Signed-off-by: Anton Nefedov 
 Reviewed-by: Alberto Garcia 
 ---
  hw/ide/core.c | 12 
  1 file changed, 12 insertions(+)

 diff --git a/hw/ide/core.c b/hw/ide/core.c
 index 2c62efc..352429b 100644
 --- a/hw/ide/core.c
 +++ b/hw/ide/core.c
 @@ -440,6 +440,14 @@ static void ide_issue_trim_cb(void *opaque, int 
 ret)
  TrimAIOCB *iocb = opaque;
  IDEState *s = iocb->s;
  
 +if (iocb->i >= 0) {
 +if (ret >= 0) {
 +block_acct_done(blk_get_stats(s->blk), &s->acct);
 +} else {
 +block_acct_failed(blk_get_stats(s->blk), &s->acct);
 +}
 +}
 +
  if (ret >= 0) {
  while (iocb->j < iocb->qiov->niov) {
  int j = iocb->j;
 @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int 
 ret)
  goto done;
  }
  
 +block_acct_start(blk_get_stats(s->blk), &s->acct,
 + count << BDRV_SECTOR_BITS, 
 BLOCK_ACCT_UNMAP);
 +
  /* Got an entry! Submit and exit.  */
  iocb->aiocb = blk_aio_pdiscard(s->blk,
 sector << 
 BDRV_SECTOR_BITS,
 @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
  }
  
  if (ret == -EINVAL) {
 +block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
>>>
>>> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
>>> also for reads and writes, and each of them could return -EINVAL.
>>>
>>
>> Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(
>>
>>> Also, -EINVAL doesn't necessarily mean that the guest driver did
>>> something wrong, it could also be the result of a host problem.
>>> Therefore, it isn't right to call block_acct_invalid() here - especially
>>> since the request may already have been accounted for as either done or
>>> failed in ide_issue_trim_cb().
>>>
>>
>> Couldn't be accounted done with such retcode;
>> and it seems I shouldnt do block_acct_failed() there anyway - or it's
>> accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()
>>
>> But if EINVAL (from further layers) should not be accounted as an
>> invalid op, then it should be accounted failed instead, the thing that
>> current code does not do.
>> (and which brings us back to possible double-accounting if we account
>> invalid in ide_issue_trim_cb() )
>
> Yes, commit caeadbc8ba4 was already wrong in assuming that there is
> only one possible source for -EINVAL.
>
>>> Instead, I think it would be better to immediately account for invalid
>>> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
>>> know for sure that indeed !ide_sect_range_ok() is the cause for the
>>> -EINVAL return code.
>>>
>> So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
>> acct_failed there, and filter off TRIM commands in the common
>> accounting.
>
> blk_aio_discard() can fail with -EINVAL, too, so getting this error code
> from a TRIM command doesn't mean anything. It can still have multiple
> possible sources.
>

 I meant that common ide_dma_cb() should account EINVAL (along with other
 errors) as failed, unless it's TRIM, which means it's already
 accounted (either invalid or failed)
>>>
>>> Oh, you would already account for failure in ide_issue_trim_cb(), too,
>>> but only if it's EINVAL? That feels like it would complicate the code
>>> quite a bit.
>>>
>>
>> No, no :) ide_issue_trim_cb does the proper accounting (failed/invalid)
>> for TRIM.
>> Then common path (ide_dma_cb()) does not account TRIM operations at all
>> regardless of the error code. No need to check for TRIM specifically if
>> we have BLOCK_ACCT_NONE.
>>
>>> And actually, didn't commit caeadbc8ba4 break werror=stop for requests
>>> returning -EINVAL because we don't call ide_handle_rw_error() any more?
>>>
>>
>> Yes.
>>
>> Read/write ignore werror=stop for invalid range case (not for EINVAL).
>> I wonder if it's cru

Re: [Qemu-block] [PATCH 0/3] Misc Quorum fixes

[Kevin Wolf]

Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> Hi,
> 
> Here's a couple of small fixes for the Quorum driver plus new test
> cases. See the individual patches for details.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Alberto Garcia]

On Wed 17 Oct 2018 05:31:32 PM CEST, Kevin Wolf wrote:
> Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
>> The blkverify mode of Quorum can only be enabled if the number of
>> children is exactly two and the value of vote-threshold is also two.
>> 
>> If the user tries to enable it but the other settings are incorrect
>> then QEMU simply prints an error message to stderr and carries on
>> disabling the blkverify setting.
>> 
>> This patch makes quorum_open() fail and return an error in this case.
>> 
>> Signed-off-by: Alberto Garcia 
>> Reported-by: Markus Armbruster 
>
> While reviewing this, I think I found another problem: Shouldn't
> .bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
> probably assert in .bdrv_del_child that s->is_blkverify == false after
> checking s->num_children <= s->threshold)

I think you're right. Do you want me to send this in a later patch or do
you prefer that I make it part of this series?

Berto

Re: [Qemu-block] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Kevin Wolf]

Am 17.10.2018 um 17:34 hat Alberto Garcia geschrieben:
> On Wed 17 Oct 2018 05:31:32 PM CEST, Kevin Wolf wrote:
> > Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> >> The blkverify mode of Quorum can only be enabled if the number of
> >> children is exactly two and the value of vote-threshold is also two.
> >> 
> >> If the user tries to enable it but the other settings are incorrect
> >> then QEMU simply prints an error message to stderr and carries on
> >> disabling the blkverify setting.
> >> 
> >> This patch makes quorum_open() fail and return an error in this case.
> >> 
> >> Signed-off-by: Alberto Garcia 
> >> Reported-by: Markus Armbruster 
> >
> > While reviewing this, I think I found another problem: Shouldn't
> > .bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
> > probably assert in .bdrv_del_child that s->is_blkverify == false after
> > checking s->num_children <= s->threshold)
> 
> I think you're right. Do you want me to send this in a later patch or do
> you prefer that I make it part of this series?

This series looked good, so no need to respin. A separate patch is fine.

Kevin

[Qemu-block] [PATCH v3 4/9] nbd: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open a read-write NBD
connection if the server provides a read-write export, but instead of
erroring out for read-only exports, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/nbd-client.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9686ecbd5e..76e9ca3abe 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -992,11 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
 logout("Failed to negotiate with the NBD server\n");
 return ret;
 }
-if (client->info.flags & NBD_FLAG_READ_ONLY &&
-!bdrv_is_read_only(bs)) {
-error_setg(errp,
-   "request for write access conflicts with read-only export");
-return -EACCES;
+if (client->info.flags & NBD_FLAG_READ_ONLY) {
+ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
+if (ret < 0) {
+return ret;
+}
 }
 if (client->info.flags & NBD_FLAG_SEND_FUA) {
 bs->supported_write_flags = BDRV_REQ_FUA;
-- 
2.19.1

[Qemu-block] [PATCH v3 9/9] block: Make auto-read-only=on default for -drive

[Kevin Wolf]

While we want machine interfaces like -blockdev and QMP blockdev-add to
add as little auto-detection as possible so that management tools are
explicit about their needs, -drive is a convenience option for human
users. Enabling auto-read-only=on by default there enables users to use
read-only images for read-only guest devices without having to specify
read-only=on explicitly. If they try to attach the image to a read-write
device, they will still get an error message.

Signed-off-by: Kevin Wolf 
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index a8755bd908..ab57a8a53a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -590,6 +590,7 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
 qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
   read_only ? "on" : "off");
+qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
 assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
 if (runstate_check(RUN_STATE_INMIGRATE)) {
-- 
2.19.1

[Qemu-block] [PATCH v3 1/9] block: Update flags in bdrv_set_read_only()

[Kevin Wolf]

To fully change the read-only state of a node, we must not only change
bs->read_only, but also update bs->open_flags.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 0d6e5f1a76..d7bd6d29b4 100644
--- a/block.c
+++ b/block.c
@@ -281,6 +281,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 }
 
 bs->read_only = read_only;
+
+if (read_only) {
+bs->open_flags &= ~BDRV_O_RDWR;
+} else {
+bs->open_flags |= BDRV_O_RDWR;
+}
+
 return 0;
 }
 
-- 
2.19.1

[Qemu-block] [PATCH v3 0/9] block: Add auto-read-only option

[Kevin Wolf]

See patch 2 for an explanation of the motivation.

v3:
- Clarified QAPI schema documentation that auto-read-only can only
  degrade read-write to read-only, not the other way round [Eric]
- Don't refuse to set copy-on-read=on and auto-read-only=on at the same
  time; only complain when actually trying to degrade to read-only
- Let bdrv_apply_auto_read_only() return -EACCESS on all errors
- Fixed file-posix and gluster implementations [Eric, Niels]
- Added a patch to make auto-read-only=on the default for human user
  interfaces (-drive/-hda/...)

v2:
- Turn bdrv_set_read_only() into bdrv_apply_auto_read_only()
- Support the option in a lot more block drivers

Kevin Wolf (9):
  block: Update flags in bdrv_set_read_only()
  block: Add auto-read-only option
  block: Require auto-read-only for existing fallbacks
  nbd: Support auto-read-only option
  file-posix: Support auto-read-only option
  curl: Support auto-read-only option
  gluster: Support auto-read-only option
  iscsi: Support auto-read-only option
  block: Make auto-read-only=on default for -drive

 qapi/block-core.json  |  7 ++
 include/block/block.h |  5 +++-
 block.c   | 54 +++
 block/bochs.c | 17 +-
 block/cloop.c | 16 -
 block/curl.c  |  8 +++
 block/dmg.c   | 16 -
 block/file-posix.c| 19 ---
 block/gluster.c   | 12 --
 block/iscsi.c |  8 ---
 block/nbd-client.c| 10 
 block/rbd.c   | 14 ---
 block/vvfat.c | 11 +++--
 blockdev.c|  1 +
 14 files changed, 120 insertions(+), 78 deletions(-)

-- 
2.19.1

[Qemu-block] [PATCH v3 6/9] curl: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, just degrade to
read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/curl.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index fabb2b4da7..db5d2bd8ef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -684,10 +684,10 @@ static int curl_open(BlockDriverState *bs, QDict 
*options, int flags,
 const char *protocol_delimiter;
 int ret;
 
-
-if (flags & BDRV_O_RDWR) {
-error_setg(errp, "curl block device does not support writes");
-return -EROFS;
+ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes",
+errp);
+if (ret < 0) {
+return ret;
 }
 
 if (!libcurl_initialized) {
-- 
2.19.1

[Qemu-block] [PATCH v3 3/9] block: Require auto-read-only for existing fallbacks

[Kevin Wolf]

Some block drivers have traditionally changed their node to read-only
mode without asking the user. This behaviour has been marked deprecated
since 2.11, expecting users to provide an explicit read-only=on option.

Now that we have auto-read-only=on, enable these drivers to make use of
the option.

This is the only use of bdrv_set_read_only(), so we can make it a bit
more specific and turn it into a bdrv_apply_auto_read_only() that is
more convenient for drivers to use.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h |  3 ++-
 block.c   | 42 +++---
 block/bochs.c | 17 ++---
 block/cloop.c | 16 +---
 block/dmg.c   | 16 +---
 block/rbd.c   | 14 --
 block/vvfat.c | 10 ++
 7 files changed, 51 insertions(+), 67 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 580b3716c3..7f5453b45b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
 bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
bool ignore_allow_rdw, Error **errp);
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+  Error **errp);
 bool bdrv_is_writable(BlockDriverState *bs);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ed73522cc6..a97e324f39 100644
--- a/block.c
+++ b/block.c
@@ -266,29 +266,41 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
read_only,
 return 0;
 }
 
-/* TODO Remove (deprecated since 2.11)
- * Block drivers are not supposed to automatically change bs->read_only.
- * Instead, they should just check whether they can provide what the user
- * explicitly requested and error out if read-write is requested, but they can
- * only provide read-only access. */
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set
+ * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg
+ * is not NULL, it is used as the error message for the Error object.
+ */
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+  Error **errp)
 {
 int ret = 0;
 
-ret = bdrv_can_set_read_only(bs, read_only, false, errp);
-if (ret < 0) {
-return ret;
+if (!(bs->open_flags & BDRV_O_RDWR)) {
+return 0;
+}
+if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+goto fail;
 }
 
-bs->read_only = read_only;
-
-if (read_only) {
-bs->open_flags &= ~BDRV_O_RDWR;
-} else {
-bs->open_flags |= BDRV_O_RDWR;
+ret = bdrv_can_set_read_only(bs, true, false, NULL);
+if (ret < 0) {
+goto fail;
 }
 
+bs->read_only = true;
+bs->open_flags &= ~BDRV_O_RDWR;
+
 return 0;
+
+fail:
+error_setg(errp, "%s", errmsg ?: "Image is read-only");
+return -EACCES;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..22e7d44211 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,23 +105,18 @@ static int bochs_open(BlockDriverState *bs, QDict 
*options, int flags,
 struct bochs_header bochs;
 int ret;
 
+/* No write support yet */
+ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+if (ret < 0) {
+return ret;
+}
+
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
false, errp);
 if (!bs->file) {
 return -EINVAL;
 }
 
-if (!bdrv_is_read_only(bs)) {
-error_report("Opening bochs images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
-ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-if (ret < 0) {
-return ret;
-}
-}
-
 ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
 if (ret < 0) {
 return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..df2b85f723 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,23 +67,17 @@ static int cloop_open(BlockDriverState *bs, QDict *options, 
int flags,
 uint32_t offsets_size, max_compressed_block_size = 1, i;
 int ret;
 
+ret = bdrv_apply

[Qemu-block] [PATCH v3 2/9] block: Add auto-read-only option

[Kevin Wolf]

If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.

Backing files should work on read-only storage, but at the same time, a
block job like commit should be able to reopen them read-write if they
are on read-write storage. However, without option inheritance, reopen
only changes the read-only option for the root node (typically the
format layer), but not the protocol layer, so reopening fails (the
format layer wants to get write permissions, but the protocol layer is
still read-only).

A simple workaround for the problem in the management tool would be to
open the protocol layer always read-write and to make only the format
layer read-only for backing files. However, sometimes the file is
actually stored on read-only storage and we don't know whether the image
can be opened read-write (for example, for NBD it depends on the server
we're trying to connect to). This adds an option that makes QEMU try to
open the image read-write, but allows it to degrade to a read-only mode
without returning an error.

The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.

Unfortunately, this more useful behaviour is also a lot harder to
implement, and libvirt needs a solution now before it can switch to
-blockdev, so let's start with this easier approach for now.

Instead of adding a new auto-read-only option, turning the existing
read-only into an enum (with a bool alternate for compatibility) was
considered, but it complicated the implementation to the point that it
didn't seem to be worth it.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json  |  7 +++
 include/block/block.h |  2 ++
 block.c   | 17 +
 block/vvfat.c |  1 +
 4 files changed, 27 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..2ac4e69e68 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,12 @@
 # either generally or in certain configurations. In this case,
 # the default value does not work and the option must be
 # specified explicitly.
+# @auto-read-only: if true and @read-only is false, QEMU may automatically
+#  decide not to open the image read-write as requested, but
+#  fall back to read-only instead (and switch between the modes
+#  later), e.g. depending on whether the image file is writable
+#  or whether a writing user is attached to the node
+#  (default: false, since 3.1)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 # (default: off)
 # @force-share:   force share all permission on added nodes.
@@ -3666,6 +3672,7 @@
 '*discard': 'BlockdevDiscardOptions',
 '*cache': 'BlockdevCacheOptions',
 '*read-only': 'bool',
+'*auto-read-only': 'bool',
 '*force-share': 'bool',
 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..580b3716c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -115,6 +115,7 @@ typedef struct HDGeometry {
   select an appropriate protocol driver,
   ignoring the format layer */
 #define BDRV_O_NO_IO   0x1 /* don't initialize for I/O */
+#define BDRV_O_AUTO_RDONLY 0x2 /* degrade to read-only if opening 
read-write fails */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
@@ -125,6 +126,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY  "read-only"
+#define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
 #define BDRV_OPT_DISCARD"discard"
 #define BDRV_OPT_FORCE_SHARE"force-share"
 
diff --git a/block.c b/block.c
index d7bd6d29b4..ed73522cc6 100644
--- a/block.c
+++ b/block.c
@@ -930,6 +930,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 
 /* Inherit the read-only option from the parent if it's not set */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_AUTO_READ_ONLY);
 
 /* Our block drivers take care to send flushes and respect unmap policy,
  * so we can default to enable both on lower layers regardless of the
@@ -1053,6 +1054,7 @@ static void bdrv_backing_options(int *child_flags, QDict 
*child_opt

[Qemu-block] [PATCH v3 8/9] iscsi: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the volume
read-write if we have the permissions, but instead of erroring out for
read-only volumes, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/iscsi.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..130ae26f5f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1878,9 +1878,11 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* Check the write protect flag of the LUN if we want to write */
 if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
 iscsilun->write_protected) {
-error_setg(errp, "Cannot open a write protected LUN as read-write");
-ret = -EACCES;
-goto out;
+ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp);
+if (ret < 0) {
+goto out;
+}
+flags &= ~BDRV_O_RDWR;
 }
 
 iscsi_readcapacity_sync(iscsilun, &local_err);
-- 
2.19.1

[Qemu-block] [PATCH v3 7/9] gluster: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf 
---
 block/gluster.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..5e300c96c8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -849,8 +849,16 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict 
*options,
 qemu_gluster_parse_flags(bdrv_flags, &open_flags);
 
 s->fd = glfs_open(s->glfs, gconf->path, open_flags);
-if (!s->fd) {
-ret = -errno;
+ret = s->fd ? 0 : -errno;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
+s->fd = glfs_open(s->glfs, gconf->path, open_flags);
+ret = s->fd ? 0 : -errno;
+}
 }
 
 s->supports_seek_data = qemu_gluster_test_seek(s->fd);
-- 
2.19.1

[Qemu-block] [PATCH v3 5/9] file-posix: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf 
---
 block/file-posix.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2da3a76355..0c1b81ce4b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -527,9 +527,22 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 
 s->fd = -1;
 fd = qemu_open(filename, s->open_flags, 0644);
-if (fd < 0) {
-ret = -errno;
-error_setg_errno(errp, errno, "Could not open '%s'", filename);
+ret = fd < 0 ? -errno : 0;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+bdrv_flags &= ~BDRV_O_RDWR;
+raw_parse_flags(bdrv_flags, &s->open_flags);
+assert(!(s->open_flags & O_CREAT));
+fd = qemu_open(filename, s->open_flags);
+ret = fd < 0 ? -errno : 0;
+}
+}
+
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not open '%s'", filename);
 if (ret == -EROFS) {
 ret = -EACCES;
 }
-- 
2.19.1

Re: [Qemu-block] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[Eric Blake]

On 10/2/18 6:02 PM, John Snow wrote:

based on: jsnow/bitmaps staging branch

This series builds on a previous standalone patch and adjusts
the permission for all (or most) of the QMP bitmap commands.

V4:
  - Replace "in-use" with "in use"
  - Replace "user_modifiable" version with "user_locked"
  - Remove more usages of frozen-and-or-locked in NBD.

John Snow (6):
   block/dirty-bitmaps: add user_locked status checker
   block/dirty-bitmaps: fix merge permissions
   block/dirty-bitmaps: allow clear on disabled bitmaps
   block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
   block/backup: prohibit backup from using in use bitmaps
   nbd: forbid use of frozen bitmaps


Just now noticing that our docs are slightly out of sync with these changes:

# @DirtyBitmapStatus:
#
# An enumeration of possible states that a dirty bitmap can report to 
the user.

#
# @frozen: The bitmap is currently in-use by a backup operation or block 
job,

#  and is immutable.
#
# @disabled: The bitmap is currently in-use by an internal operation and is
#read-only. It can still be deleted.

This state is also reachable when using x-block-dirty-bitmap-disable, 
which is not an internal operation.  We probably also ought to document 
that this state is a prereq to x-nbd-server-add-bitmap, since...


#
# @active: The bitmap is actively monitoring for new writes, and can be 
cleared,

#  deleted, or used for backup operations.

...active is not valid for that usage.

#
# @locked: The bitmap is currently in-use by some operation and can not be
#  cleared, deleted, or used for backup operations. (Since 2.12)


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

Re: [Qemu-block] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[John Snow]

On 10/17/2018 02:24 PM, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> based on: jsnow/bitmaps staging branch
>>
>> This series builds on a previous standalone patch and adjusts
>> the permission for all (or most) of the QMP bitmap commands.
>>
>> V4:
>>   - Replace "in-use" with "in use"
>>   - Replace "user_modifiable" version with "user_locked"
>>   - Remove more usages of frozen-and-or-locked in NBD.
>>
>> John Snow (6):
>>    block/dirty-bitmaps: add user_locked status checker
>>    block/dirty-bitmaps: fix merge permissions
>>    block/dirty-bitmaps: allow clear on disabled bitmaps
>>    block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
>>    block/backup: prohibit backup from using in use bitmaps
>>    nbd: forbid use of frozen bitmaps
> 
> Just now noticing that our docs are slightly out of sync with these
> changes:
> 
> # @DirtyBitmapStatus:
> #
> # An enumeration of possible states that a dirty bitmap can report to
> the user.
> #
> # @frozen: The bitmap is currently in-use by a backup operation or block
> job,
> #  and is immutable.
> #
> # @disabled: The bitmap is currently in-use by an internal operation and is
> #    read-only. It can still be deleted.
> 
> This state is also reachable when using x-block-dirty-bitmap-disable,
> which is not an internal operation.  We probably also ought to document
> that this state is a prereq to x-nbd-server-add-bitmap, since...
> 
> #
> # @active: The bitmap is actively monitoring for new writes, and can be
> cleared,
> #  deleted, or used for backup operations.
> 
> ...active is not valid for that usage.
> 
> #
> # @locked: The bitmap is currently in-use by some operation and can not be
> #  cleared, deleted, or used for backup operations. (Since 2.12)
> 
> 

Ah, you're right. The rationale on disabled there is a bit wrong. I
think active is still correct, but the pull workflow has some extra
criteria.

I'll give a shot at touching this up.

--js

Re: [Qemu-block] [PATCH v3 7/9] gluster: Support auto-read-only option

[Niels de Vos]

On Wed, Oct 17, 2018 at 06:41:58PM +0200, Kevin Wolf wrote:
> If read-only=off, but auto-read-only=on is given, open the file
> read-write if we have the permissions, but instead of erroring out for
> read-only files, just degrade to read-only.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/gluster.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 4fd55a9cc5..5e300c96c8 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -849,8 +849,16 @@ static int qemu_gluster_open(BlockDriverState *bs,  
> QDict *options,
>  qemu_gluster_parse_flags(bdrv_flags, &open_flags);
>  
>  s->fd = glfs_open(s->glfs, gconf->path, open_flags);
> -if (!s->fd) {
> -ret = -errno;
> +ret = s->fd ? 0 : -errno;
> +
> +if (ret == -EACCES || ret == -EROFS) {
> +/* Try to degrade to read-only, but if it doesn't work, still use the
> + * normal error message. */
> +if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
> +open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
> +s->fd = glfs_open(s->glfs, gconf->path, open_flags);
> +ret = s->fd ? 0 : -errno;
> +}
>  }
>  
>  s->supports_seek_data = qemu_gluster_test_seek(s->fd);
> -- 

Looks good to me, thanks.

Reviewed-by: Niels de Vos 

Re: [Qemu-block] [PATCH v3 2/9] block: Add auto-read-only option

[Eric Blake]

On 10/17/18 11:41 AM, Kevin Wolf wrote:

If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.




The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.




+++ b/qapi/block-core.json
@@ -3651,6 +3651,12 @@
  # either generally or in certain configurations. In this case,
  # the default value does not work and the option must be
  # specified explicitly.
+# @auto-read-only: if true and @read-only is false, QEMU may automatically
+#  decide not to open the image read-write as requested, but
+#  fall back to read-only instead (and switch between the modes
+#  later), e.g. depending on whether the image file is writable
+#  or whether a writing user is attached to the node
+#  (default: false, since 3.1)


Definitely better wording compared to v2.

Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [PATCH v3 3/9] block: Require auto-read-only for existing fallbacks

[Eric Blake]

On 10/17/18 11:41 AM, Kevin Wolf wrote:

Some block drivers have traditionally changed their node to read-only
mode without asking the user. This behaviour has been marked deprecated
since 2.11, expecting users to provide an explicit read-only=on option.

Now that we have auto-read-only=on, enable these drivers to make use of
the option.

This is the only use of bdrv_set_read_only(), so we can make it a bit
more specific and turn it into a bdrv_apply_auto_read_only() that is
more convenient for drivers to use.

Signed-off-by: Kevin Wolf 
---



+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set
+ * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg
+ * is not NULL, it is used as the error message for the Error object.


Works for me.


+++ b/block/rbd.c
@@ -780,16 +780,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
  /* If we are using an rbd snapshot, we must be r/o, otherwise
   * leave as-is */
  if (s->snap != NULL) {
-if (!bdrv_is_read_only(bs)) {
-error_report("Opening rbd snapshots without an explicit "
- "read-only=on option is deprecated. Future versions "
- "will refuse to open the image instead of "
- "automatically marking the image read-only.");
-r = bdrv_set_read_only(bs, true, &local_err);
-if (r < 0) {
-error_propagate(errp, local_err);
-goto failed_open;
-}
+r = bdrv_apply_auto_read_only(bs, "rbd snapshots are read-only", errp);
+if (r < 0) {
+rbd_close(s->image);
+goto failed_open;


That rbd_close() is an independent bugfix.  Should probably be split to 
a separate commit, or at a minimum called out in the commit message as 
intentional.


Actually, is it really needed to prevent a leak, or does the existing 
rados_shutdown() in failed_open already implicitly cover the actions of 
rbd_close()?


With the RBD issue touched up,
Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [PATCH v3 5/9] file-posix: Support auto-read-only option

[Eric Blake]

On 10/17/18 11:41 AM, Kevin Wolf wrote:

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf 
---
  block/file-posix.c | 19 ---
  1 file changed, 16 insertions(+), 3 deletions(-)



Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [PATCH v3 9/9] block: Make auto-read-only=on default for -drive

[Eric Blake]

On 10/17/18 11:42 AM, Kevin Wolf wrote:

While we want machine interfaces like -blockdev and QMP blockdev-add to
add as little auto-detection as possible so that management tools are
explicit about their needs, -drive is a convenience option for human
users. Enabling auto-read-only=on by default there enables users to use
read-only images for read-only guest devices without having to specify
read-only=on explicitly. If they try to attach the image to a read-write
device, they will still get an error message.

Signed-off-by: Kevin Wolf 
---
  blockdev.c | 1 +
  1 file changed, 1 insertion(+)



Reviewed-by: Eric Blake 

However, the obligatory: "iotests coverage?"

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

Re: [Qemu-block] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions

[Eric Blake]

On 10/2/18 6:08 PM, John Snow wrote:


John Snow (6):
   block/dirty-bitmaps: add user_locked status checker
   block/dirty-bitmaps: fix merge permissions
   block/dirty-bitmaps: allow clear on disabled bitmaps
   block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
   block/backup: prohibit backup from using in use bitmaps
   nbd: forbid use of frozen bitmaps





Planning on submitting tests for 3.1, but with the merge queue opened up
again I'm pretty keen on getting some stable commit IDs for these so far.


Food for thought when writing tests:
I've just noticed with my progress on libvirt code that for 
error-recovery purposes, if we decide to make 
x-block-dirty-bitmap-disable fail on a disabled bitmap (or enable on an 
enabled map) that it would be nice to have a distinct error code; or 
even better to just be a silent no-op.  That's because when libvirt has 
the situation:


bitmap1 (disabled), bitmap2 (enabled)

and wants to delete the point in time between those two, it will call:

enable bitmap1
merge bitmap2 into bitmap1
remove bitmap2

Removing a bitmap is not part of 'transaction', and there is no pressing 
reason for it to be (other than convenience) - which means this is at 
least 2 commands (if the first two are a transaction) or 3 (if all are 
done independently).


But because it is not a transaction, libvirt can conceivably succeed at 
reenabling bitmap1 but then fail because the guest shut down before 
merging bitmap2 into 1.  When the guest is restarted, there is no real 
data loss (bitmap1 and 2 are now both active, which is a little bit less 
efficient for guest writes than having just one active), so the user can 
attempt to have libvirt retry the deletion sequence.  If asking for 
bitmap1 to be enabled fails (because it is already enabled), then 
libvirt will have to add workaround code (either to detect that the 
error iss different than other failures, or to pre-probe whether the 
bitmap is already enabled); whereas if the request is a no-op, things 
are easier from libvirt's point of view.


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

[Qemu-block] [PATCH v2 1/3] qemu-iotests: Modern shellscripting (use $() instead of ``)

[Mao Zhongyi]

Various shell files contain a mix between obsolete ``
and modern $(); It would be nice to convert to using $()
everywhere.

`pwd` and `basename $0` are in 231 files under directory
tests/qemu-iotests, so replaced it with the following:

sed -i 's/`pwd`/$(pwd)/g' $(git grep -l "\`pwd\`")
sed -i 's/`basename $0`/$(basename $0)/g' $(git grep -l "basename \$0")

A small amount of the rest is manually modified.

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: qemu-block@nongnu.org

Signed-off-by: Mao Zhongyi 
---
 tests/qemu-iotests/001|  4 +-
 tests/qemu-iotests/002|  4 +-
 tests/qemu-iotests/003|  4 +-
 tests/qemu-iotests/004|  4 +-
 tests/qemu-iotests/005|  4 +-
 tests/qemu-iotests/007|  6 +--
 tests/qemu-iotests/008|  4 +-
 tests/qemu-iotests/009|  4 +-
 tests/qemu-iotests/010|  4 +-
 tests/qemu-iotests/011|  6 +--
 tests/qemu-iotests/012|  4 +-
 tests/qemu-iotests/013|  4 +-
 tests/qemu-iotests/014|  6 +--
 tests/qemu-iotests/015|  4 +-
 tests/qemu-iotests/017|  4 +-
 tests/qemu-iotests/018|  4 +-
 tests/qemu-iotests/019|  4 +-
 tests/qemu-iotests/020|  4 +-
 tests/qemu-iotests/021|  4 +-
 tests/qemu-iotests/022|  4 +-
 tests/qemu-iotests/023|  4 +-
 tests/qemu-iotests/024|  4 +-
 tests/qemu-iotests/025|  4 +-
 tests/qemu-iotests/026|  4 +-
 tests/qemu-iotests/027|  4 +-
 tests/qemu-iotests/028|  4 +-
 tests/qemu-iotests/029|  4 +-
 tests/qemu-iotests/031|  4 +-
 tests/qemu-iotests/032|  4 +-
 tests/qemu-iotests/033|  4 +-
 tests/qemu-iotests/034|  4 +-
 tests/qemu-iotests/035|  4 +-
 tests/qemu-iotests/036|  4 +-
 tests/qemu-iotests/037|  4 +-
 tests/qemu-iotests/038|  4 +-
 tests/qemu-iotests/039|  4 +-
 tests/qemu-iotests/042|  4 +-
 tests/qemu-iotests/043|  4 +-
 tests/qemu-iotests/046|  4 +-
 tests/qemu-iotests/047|  4 +-
 tests/qemu-iotests/048|  2 +-
 tests/qemu-iotests/049|  4 +-
 tests/qemu-iotests/050|  4 +-
 tests/qemu-iotests/051|  4 +-
 tests/qemu-iotests/052|  4 +-
 tests/qemu-iotests/053|  4 +-
 tests/qemu-iotests/054|  4 +-
 tests/qemu-iotests/058|  4 +-
 tests/qemu-iotests/059|  4 +-
 tests/qemu-iotests/061|  4 +-
 tests/qemu-iotests/062|  4 +-
 tests/qemu-iotests/063|  4 +-
 tests/qemu-iotests/064|  4 +-
 tests/qemu-iotests/067|  4 +-
 tests/qemu-iotests/070|  4 +-
 tests/qemu-iotests/073|  4 +-
 tests/qemu-iotests/074|  2 +-
 tests/qemu-iotests/075|  4 +-
 tests/qemu-iotests/076|  4 +-
 tests/qemu-iotests/077|  4 +-
 tests/qemu-iotests/078|  4 +-
 tests/qemu-iotests/079|  4 +-
 tests/qemu-iotests/080|  4 +-
 tests/qemu-iotests/081|  4 +-
 tests/qemu-iotests/082|  4 +-
 tests/qemu-iotests/083|  4 +-
 tests/qemu-iotests/084|  4 +-
 tests/qemu-iotests/085|  4 +-
 tests/qemu-iotests/086|  4 +-
 tests/qemu-iotests/087|  4 +-
 tests/qemu-iotests/088|  4 +-
 tests/qemu-iotests/091|  4 +-
 tests/qemu-iotests/092|  4 +-
 tests/qemu-iotests/095|  4 +-
 tests/qemu-iotests/101|  4 +-
 tests/qemu-iotests/104|  4 +-
 tests/qemu-iotests/105|  4 +-
 tests/qemu-iotests/116|  4 +-
 tests/qemu-iotests/128|  4 +-
 tests/qemu-iotests/131|  4 +-
 tests/qemu-iotests/133|  4 +-
 tests/qemu-iotests/134|  4 +-
 tests/qemu-iotests/135|  4 +-
 tests/qemu-iotests/142|  4 +-
 tests/qemu-iotests/144|  4 +-
 tests/qemu-iotests/145|  4 +-
 tests/qemu-iotests/146|  4 +-
 tests/qemu-iotests/154|  4 +-
 tests/qemu-iotests/158|  4 +-
 tests/qemu-iotests/171|  4 +-
 tests/qemu-iotests/172|  4 +-
 tests/qemu-iotests/173|  4 +-
 tests/qemu-iotests/174|  4 +-
 tests/qemu-iotests/175|  4 +-
 tests/qemu-iotests/177|  4 +-
 tests/qemu-iotests/178|  4 +-
 tests/qemu-iotests/181|  4 +-
 tests/qemu-iotests/183|  4 +-
 tests/qemu-iotests/184|  4 +-
 tests/qemu-iotests/185|  4 +-
 tests/qemu-iotests/186|  4 +-
 tests/qemu-iotests/187|  4 +-
 tests/qemu-iotests/188|  4 +-
 tests/qemu-iotests/189|  4 +-
 tests/qemu

Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/3] qemu-iotests: Modern shellscripting (use $() instead of ``)

[Eric Blake]

On 10/17/18 10:17 PM, Mao Zhongyi wrote:

Various shell files contain a mix between obsolete ``
and modern $(); It would be nice to convert to using $()
everywhere.

`pwd` and `basename $0` are in 231 files under directory
tests/qemu-iotests, so replaced it with the following:

sed -i 's/`pwd`/$(pwd)/g' $(git grep -l "\`pwd\`")


No. Instead, I'd rather a separate patch that does:

s/`pwd`/$PWD/
s/\$(pwd)/$PWD/

since POSIX requires $PWD to be sane, and thus save us a wasted forked 
process.



sed -i 's/`basename $0`/$(basename $0)/g' $(git grep -l "basename \$0")

A small amount of the rest is manually modified.

Cc: kw...@redhat.com
Cc: mre...@redhat.com
Cc: qemu-block@nongnu.org

Signed-off-by: Mao Zhongyi 
---



+++ b/tests/qemu-iotests/001
@@ -21,10 +21,10 @@
  # creator
  owner=h...@lst.de
  
-seq=`basename $0`

+seq=$(basename $0)
  echo "QA output created by $seq"
  
-here=`pwd`

+here=$(pwd)
  status=1  # failure is the default!


At one point, someone (Jeff?) proposed a cleanup patch that got rid of a 
lot of cruft in iotests, including the fact that scripts that don't use 
$seq don't need to assign seq=$(basename $0).  We should probably revive 
that rather than just making pointless churn on stuff that is garbage 
anyways.  But I don't have time to look up a URL to that older series at 
the moment.


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