Re: [Qemu-block] [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring

[Maxim Levitsky]

On Wed, 2019-08-21 at 16:39 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:07PM +0300, Maxim Levitsky wrote:
> > * rename the write_func to create_write_func,
> >   and init_func to create_init_func
> >   this is  preparation for other write_func that will
> >   be used to update the encryption keys.
> > 
> > No functional changes
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/crypto.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 8237424ae6..42a3f0898b 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -51,7 +51,6 @@ static int block_crypto_probe_generic(QCryptoBlockFormat 
> > format,
> >  }
> >  }
> >  
> > -
> 
> Unrelated whitespace change
> 
> >  static ssize_t block_crypto_read_func(QCryptoBlock *block,
> >size_t offset,
> >uint8_t *buf,
> > @@ -77,7 +76,7 @@ struct BlockCryptoCreateData {
> >  };
> >  
> >  
> > -static ssize_t block_crypto_write_func(QCryptoBlock *block,
> > +static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
> > size_t offset,
> > const uint8_t *buf,
> > size_t buflen,
> 
> Re-indent.
> 
> > @@ -95,8 +94,7 @@ static ssize_t block_crypto_write_func(QCryptoBlock 
> > *block,
> >  return ret;
> >  }
> >  
> > -
> 
> Unrelated whitespace
> 
> > -static ssize_t block_crypto_init_func(QCryptoBlock *block,
> > +static ssize_t block_crypto_create_init_func(QCryptoBlock *block,
> >size_t headerlen,
> >void *opaque,
> >Error **errp)
> 
> Re-indent.
> 
> > @@ -108,7 +106,8 @@ static ssize_t block_crypto_init_func(QCryptoBlock 
> > *block,
> >  return -EFBIG;
> >  }
> >  
> > -/* User provided size should reflect amount of space made
> > +/*
> > + * User provided size should reflect amount of space made
> 
> Unrelated whitespace
> 
> >   * available to the guest, so we must take account of that
> >   * which will be used by the crypto header
> >   */
> > @@ -117,6 +116,8 @@ static ssize_t block_crypto_init_func(QCryptoBlock 
> > *block,
> >  }
> >  
> >  
> > +
> > +
> 
> Unrelated whitespace
> 
> >  static QemuOptsList block_crypto_runtime_opts_luks = {
> >  .name = "crypto",
> >  .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
> > @@ -272,8 +273,8 @@ static int 
> > block_crypto_co_create_generic(BlockDriverState *bs,
> >  };
> >  
> >  crypto = qcrypto_block_create(opts, NULL,
> > -  block_crypto_init_func,
> > -  block_crypto_write_func,
> > +  block_crypto_create_init_func,
> > +  block_crypto_create_write_func,
> >&data,
> >errp);
> 
> With the whitespace changes removed & indent fixed
> 
> Reviewed-by: Daniel P. Berrangé 
> 
> 
> Regards,
> Daniel

Thanks you!

Best regards,
Maxim Levitsky

Re: [Qemu-block] [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring

[Maxim Levitsky]

On Tue, 2019-08-20 at 18:38 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > * rename the write_func to create_write_func,
> >   and init_func to create_init_func
> >   this is  preparation for other write_func that will
> >   be used to update the encryption keys.
> > 
> > No functional changes
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/crypto.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> 
> I’m not quite sure why you remove or add blank lines seemingly at random...

Basically to have consistent two space separation between all functions.
A bit of OCD I confess :-)

> 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 8237424ae6..42a3f0898b 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> 
> [...]
> 
> > @@ -77,7 +76,7 @@ struct BlockCryptoCreateData {
> >  };
> >  
> >  
> > -static ssize_t block_crypto_write_func(QCryptoBlock *block,
> > +static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
> > size_t offset,
> > const uint8_t *buf,
> > size_t buflen,
> 
> Alignment should be kept at the opening parentheses.
Opps. I am still trying to learn that rule. Fixed.

> 
> But other than those two things, why not.
> 
> Max
> 

Best regards,
Thanks for the review
Maxim Levitsky

Re: [Qemu-block] [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring

[Maxim Levitsky]

On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > This is also a preparation for key read/write/erase functions
> > 
> > * use master key len from the header
> > * prefer to use crypto params in the QCryptoBlockLUKS
> >   over passing them as function arguments
> > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> > * Add comments to various crypto parameters in the QCryptoBlockLUKS
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 213 ++--
> >  1 file changed, 105 insertions(+), 108 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 409ab50f20..48213abde7 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> 
> [...]
> 
> > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct 
> > QCryptoBlockLUKSHeader) != 592);
> >  struct QCryptoBlockLUKS {
> >  QCryptoBlockLUKSHeader header;
> >  
> > -/* Cache parsed versions of what's in header fields,
> > - * as we can't rely on QCryptoBlock.cipher being
> > - * non-NULL */
> 
> Hm, why remove this comment?

Because the intended uses of these fields changed.
As Daniel explained to me initially none of the crypto parameters
were stored in the QCryptoBlockLUKS, and thus there were all passed
as function arguments.
Later when qemu-img info was added/implemented, there was need to 'cache' them
in the header so that info command could use them after image was opened.

Now after my changes this is no longer true. now these crypto parameters are 
set early
on create/load and used everywhere to avoid passing them over and over to each
function.

> 
> > +/* Main encryption algorithm used for encryption*/
> >  QCryptoCipherAlgorithm cipher_alg;
> > +
> > +/* Mode of encryption for the selected encryption algorithm */
> >  QCryptoCipherMode cipher_mode;
> > +
> > +/* Initialization vector generation algorithm */
> >  QCryptoIVGenAlgorithm ivgen_alg;
> > +
> > +/* Hash algorithm used for IV generation*/
> >  QCryptoHashAlgorithm ivgen_hash_alg;
> > +
> > +/*
> > + * Encryption algorithm used for IV generation.
> > + * Usually the same as main encryption algorithm
> > + */
> > +QCryptoCipherAlgorithm ivgen_cipher_alg;
> > +
> > +/* Hash algorithm used in pbkdf2 function */
> >  QCryptoHashAlgorithm hash_alg;
> >  };
> >  
> > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm 
> > cipher,
> >  }
> >  }
> >  
> > +static int masterkeylen(QCryptoBlockLUKS *luks)
> 
> This should be a const pointer.
I haven't had const in mind while writing this but you are right.
Fixed.


> 
> > +{
> > +return luks->header.key_bytes;
> > +}
> > +
> > +
> >  /*
> >   * Given a key slot, and user password, this will attempt to unlock
> >   * the master encryption key from the key slot.
> > @@ -410,21 +430,15 @@ 
> > qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> >   */
> >  static int
> >  qcrypto_block_luks_load_key(QCryptoBlock *block,
> > -QCryptoBlockLUKSKeySlot *slot,
> > +uint slot_idx,
> 
> Did you use uint on purpose or do you mean a plain “unsigned”?
Well there are just 8 slots, but yea I don't mind to make this an unsigned int.


> 
> >  const char *password,
> > -QCryptoCipherAlgorithm cipheralg,
> > -QCryptoCipherMode ciphermode,
> > -QCryptoHashAlgorithm hash,
> > -QCryptoIVGenAlgorithm ivalg,
> > -QCryptoCipherAlgorithm ivcipheralg,
> > -QCryptoHashAlgorithm ivhash,
> >  uint8_t *masterkey,
> > -size_t masterkeylen,
> >  QCryptoBlockReadFunc readfunc,
> >  void *opaque,
> >  Error **errp)
> >  {
> >  QCryptoBlockLUKS *luks = block->opaque;
> > +QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> 
> I think this is a great opportunity to make this a const pointer.
Agree. Done.
> 
> >  uint8_t *splitkey;
> >  size_t splitkeylen;
> >  uint8_t *possiblekey;
> 
> [...]
> 
> > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> >  goto fail;
> >  }
> >  
> > +cipher_mode = g_strdup(luks->header.cipher_mode);
> > +
> 
> This should be freed under the fail label.
> 
> (And maybe the fact that this no longer modifies
> luks->header.cipher_mode should be mentioned in the commit message, I
> don’t know.)

I swear I documented that in the commit message... yea in the next commit (:-()
Fixed that now.

> 
> >  /*
> >   * The cipher_mode header contains a string that we have
> >   * to further parse, of the format
> 
> [...]
> 
> > @@ -730,13 +718,13

Re: [Qemu-block] [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always

[Maxim Levitsky]

On Thu, 2019-08-22 at 02:01 +0300, Nir Soffer wrote:
> On Wed, Aug 14, 2019, 23:23 Maxim Levitsky  wrote:
> 
> > While there are other places where these are still stored in memory,
> > this is still one less key material area that can be sniffed with
> > various side channel attacks
> > 
> > 
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 52 ++---
> >  1 file changed, 44 insertions(+), 8 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index e1a4df94b7..336e633df4 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -1023,8 +1023,18 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
> >   cleanup:
> >  qcrypto_ivgen_free(ivgen);
> >  qcrypto_cipher_free(cipher);
> > -g_free(splitkey);
> > -g_free(possiblekey);
> > +
> > +if (splitkey) {
> > +memset(splitkey, 0, splitkeylen);
> > 
> 
> I think we need memset_s() or similar replacement, see
> 
> https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

You raise a very very good point here! Thanks!!

Best regards,
Maxim Levitsky

Re: [Qemu-block] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always

[Nir Soffer]

On Wed, Aug 14, 2019, 23:23 Maxim Levitsky  wrote:

> While there are other places where these are still stored in memory,
> this is still one less key material area that can be sniffed with
> various side channel attacks
>
>
>
> Signed-off-by: Maxim Levitsky 
> ---
>  crypto/block-luks.c | 52 ++---
>  1 file changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index e1a4df94b7..336e633df4 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -1023,8 +1023,18 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
>   cleanup:
>  qcrypto_ivgen_free(ivgen);
>  qcrypto_cipher_free(cipher);
> -g_free(splitkey);
> -g_free(possiblekey);
> +
> +if (splitkey) {
> +memset(splitkey, 0, splitkeylen);
>

I think we need memset_s() or similar replacement, see

https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

+g_free(splitkey);
> +}
> +
> +if (possiblekey) {
> +memset(possiblekey, 0, masterkeylen(luks));
> +g_free(possiblekey);
> +
> +}
> +
>  return ret;
>  }
>
> @@ -1161,16 +1171,34 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>  block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
>  block->payload_offset = luks->header.payload_offset *
> block->sector_size;
>
> -g_free(masterkey);
> -g_free(password);
> +if (masterkey) {
> +memset(masterkey, 0, masterkeylen(luks));
> +g_free(masterkey);
> +}
> +
> +if (password) {
> +memset(password, 0, strlen(password));
> +g_free(password);
> +}
> +
>  return 0;
>
>   fail:
> -g_free(masterkey);
> +
> +if (masterkey) {
> +memset(masterkey, 0, masterkeylen(luks));
> +g_free(masterkey);
> +}
> +
> +if (password) {
> +memset(password, 0, strlen(password));
> +g_free(password);
> +}
> +
>  qcrypto_block_free_cipher(block);
>  qcrypto_ivgen_free(block->ivgen);
> +
>  g_free(luks);
> -g_free(password);
>  return ret;
>  }
>
> @@ -1459,7 +1487,10 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>
>  memset(masterkey, 0, luks->header.key_bytes);
>  g_free(masterkey);
> +
> +memset(password, 0, strlen(password));
>  g_free(password);
> +
>  g_free(cipher_mode_spec);
>
>  return 0;
> @@ -1467,9 +1498,14 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>   error:
>  if (masterkey) {
>  memset(masterkey, 0, luks->header.key_bytes);
> +g_free(masterkey);
>  }
> -g_free(masterkey);
> -g_free(password);
> +
> +if (password) {
> +memset(password, 0, strlen(password));
> +g_free(password);
> +}
> +
>  g_free(cipher_mode_spec);
>
>  qcrypto_block_free_cipher(block);
> --
> 2.17.2
>
>
>

Re: [Qemu-block] [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions

[Maxim Levitsky]

On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > With upcoming key management, the header will
> > need to be stored after the image is created.
> > 
> > Extracting load header isn't strictly needed, but
> > do this anyway for the symmetry.
> > 
> > Also I extracted a function that does basic sanity
> > checks on the just read header, and a function
> > which parses all the crypto format to make the
> > code a bit more readable, plus now the code
> > doesn't destruct the in-header cipher-mode string,
> > so that the header now can be stored many times,
> > which is needed for the key management.
> > 
> > Also this allows to contain the endianess conversions
> > in these functions alone
> > 
> > The header is no longer endian swapped in place,
> > to prevent (mostly theoretical races I think)
> > races where someone could see the header in the
> > process of beeing byteswapped.
> 
> The formatting looks weird, it doesn’t look quite 72 characters wide...
>  (what commit messages normally use)
Could you elaborate on that? I thought that code should not
exceed 80 character limit.

> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 756 ++--
> >  1 file changed, 440 insertions(+), 316 deletions(-)
> 
> Also, this commit is just too big.

Yea, but it has no functional changes.
I can split it further, but that won't help much IMHO.

Best regards,
Maxim Levitsky

Re: [Qemu-block] [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always

[Maxim Levitsky]

On Tue, 2019-08-20 at 20:12 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > While there are other places where these are still stored in memory,
> > this is still one less key material area that can be sniffed with
> > various side channel attacks
> > 
> > 
> > 
> 
> (Many empty lines here)
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 52 ++---
> >  1 file changed, 44 insertions(+), 8 deletions(-)
> 
> Wouldn’t it make sense to introduce a dedicated function for this?

Absolutely. I was mostly focused on fixing all the cases first.
I usually refactor such ugly code at the end, but this time I forgot
to do so.

Plus I need to pick a place where to put such function (it can be useful in any 
place in qemu), 
and first check if maybe glib already has such free+scrub function implemented 
somewhere.

Best regards,
Maxim Levitsky

Re: [Qemu-block] [Qemu-devel] [PATCH 12/13] qemu-img: implement key management

[Maxim Levitsky]

On Tue, 2019-08-20 at 20:29 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/crypto.c   |  16 ++
> >  block/crypto.h   |   3 +
> >  qemu-img-cmds.hx |  13 +
> >  qemu-img.c   | 140 +++
> >  4 files changed, 172 insertions(+)
> 
> Yes, this seems a bit weird.  Putting it under amend seems like the
> natural thing if that works; if not, I think it should be a single
> qemu-img subcommand instead of two.
> 
> Max
> 
Agree, thats why RFC.

Best regards,
Maxim Levitsky

Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)

[Maxim Levitsky]

On Tue, 2019-08-20 at 20:27 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > This adds:
> > 
> > * x-blockdev-update-encryption and x-blockdev-erase-encryption qmp commands
> >   Both commands take the QCryptoKeyManageOptions
> >   the x-blockdev-update-encryption is meant for non destructive addition
> >   of key slots / whatever the encryption driver supports in the future
> > 
> >   x-blockdev-erase-encryption is meant for destructive encryption key erase,
> >   in some cases even without way to recover the data.
> > 
> > 
> > * bdrv_setup_encryption callback in the block driver
> >   This callback does both the above functions with 'action' parameter
> > 
> > * QCryptoKeyManageOptions with set of options that drivers can use for 
> > encryption managment
> >   Currently it has all the options that LUKS needs, and later it can be 
> > extended
> >   (via union) to support more encryption drivers if needed
> > 
> > * blk_setup_encryption / bdrv_setup_encryption - the usual block layer 
> > wrappers.
> >   Note that bdrv_setup_encryption takes BlockDriverState and not BdrvChild,
> >   for the ease of use from the qmp code. It is not expected that this 
> > function
> >   will be used by anything but qmp and qemu-img code
> > 
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/block-backend.c  |  9 
> >  block/io.c | 24 
> >  blockdev.c | 40 ++
> >  include/block/block.h  | 12 ++
> >  include/block/block_int.h  | 11 ++
> >  include/sysemu/block-backend.h |  7 ++
> >  qapi/block-core.json   | 36 ++
> >  qapi/crypto.json   | 26 ++
> >  8 files changed, 165 insertions(+)
> 
> Now I don’t know whether you want to keep this interface at all, because
> the cover letter seemed to imply you’d prefer a QMP amend.  But let it
> be said that a QMP amend is no trivial task.  I think the most difficult
> bit is that the qcow2 implementation currently is inherently an offline
> operation.  It isn’t a good idea to use it on a live image.  (Maybe it
> works, but it’s definitely not what I had in mind when I wrote it.)
> 
> So I’ll still take a quick glance at the interface here.
> 
> [...]
> 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 0d43d4f37c..53ed411eed 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5327,3 +5327,39 @@
> >'data' : { 'node-name': 'str',
> >   'iothread': 'StrOrNull',
> >   '*force': 'bool' } }
> > +
> > +
> > +##
> > +# @x-blockdev-update-encryption:
> > +#
> > +# Update the encryption keys for an encrypted block device
> > +#
> > +# @node-name:Name of the blockdev to operate on
> > +# @force: Disable safety checks (use with care)
> > +# @options:   Driver specific options
> > +#
> > +
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-update-encryption',
> > +  'data': { 'node-name' : 'str',
> > +'*force' : 'bool',
> > +'options': 'QCryptoEncryptionSetupOptions' } }
> > +
> > +##
> > +# @x-blockdev-erase-encryption:
> > +#
> > +# Erase the encryption keys for an encrypted block device
> > +#
> > +# @node-name:Name of the blockdev to operate on
> 
> Why the tab?
Because checkpatch.pl doesn't warn about this :-)

> 
> > +# @force: Disable safety checks (use with care)
> 
> I think being a bit more verbose wouldn’t hurt.
> 
> (Same above.)
True about this - this is another reason this is RFC,

I honestly didn't finish the documentation,
since the sudden change to drop all of this interface.


> 
> > +# @options:   Driver specific options
> > +#
> > +# Returns: @QCryptoKeyManageResult
> > +#
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-erase-encryption',
> > +  'data': { 'node-name' : 'str',
> > +'*force' : 'bool',
> > +'options': 'QCryptoEncryptionSetupOptions' } }
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..69e8b086db 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -309,3 +309,29 @@
> >'base': 'QCryptoBlockInfoBase',
> >'discriminator': 'format',
> >'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> > +
> > +
> > +##
> > +# @QCryptoEncryptionSetupOptions:
> > +#
> > +# Driver specific options for encryption key management.
> 
> The options do seem LUKS-specific, but the name of this structure does not.
This is because to be not luks specific we must use some kind of an union
which means that the user has to specify the driver which I didn't want to do.
Now all of you convinced me ( :-) ) to do this so this will be done when I 
switch
to the amend interface.

> 
> > +# @key-secret: the ID of a QCryptoSecret object providing the password
> > +#  to add or to erase (optional for erase)
> > +#
> > +# @old-key-secret: t

Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)

[Maxim Levitsky]

On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > This adds:
> > 
> > * x-blockdev-update-encryption and x-blockdev-erase-encryption qmp commands
> >   Both commands take the QCryptoKeyManageOptions
> >   the x-blockdev-update-encryption is meant for non destructive addition
> >   of key slots / whatever the encryption driver supports in the future
> > 
> >   x-blockdev-erase-encryption is meant for destructive encryption key erase,
> >   in some cases even without way to recover the data.
> > 
> > 
> > * bdrv_setup_encryption callback in the block driver
> >   This callback does both the above functions with 'action' parameter
> > 
> > * QCryptoKeyManageOptions with set of options that drivers can use for 
> > encryption managment
> >   Currently it has all the options that LUKS needs, and later it can be 
> > extended
> >   (via union) to support more encryption drivers if needed
> > 
> > * blk_setup_encryption / bdrv_setup_encryption - the usual block layer 
> > wrappers.
> >   Note that bdrv_setup_encryption takes BlockDriverState and not BdrvChild,
> >   for the ease of use from the qmp code. It is not expected that this 
> > function
> >   will be used by anything but qmp and qemu-img code
> > 
> > 
> > Signed-off-by: Maxim Levitsky 
> 
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 0d43d4f37c..53ed411eed 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5327,3 +5327,39 @@
> >'data' : { 'node-name': 'str',
> >   'iothread': 'StrOrNull',
> >   '*force': 'bool' } }
> > +
> > +
> > +##
> > +# @x-blockdev-update-encryption:
> > +#
> > +# Update the encryption keys for an encrypted block device
> > +#
> > +# @node-name:Name of the blockdev to operate on
> > +# @force: Disable safety checks (use with care)
> 
> What checks excactly are disabled?
Ability to overwrite an used slot with a different password. 
If overwrite fails, the image won't be recoverable.

The safe way is to add a new slot, then erase the old
one, but this changes the slot where the password
is stored, unless this procedure is used twice

> 
> > +# @options:   Driver specific options
> > +#
> > +
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-update-encryption',
> > +  'data': { 'node-name' : 'str',
> > +'*force' : 'bool',
> > +'options': 'QCryptoEncryptionSetupOptions' } }
> > +
> > +##
> > +# @x-blockdev-erase-encryption:
> > +#
> > +# Erase the encryption keys for an encrypted block device
> > +#
> > +# @node-name:Name of the blockdev to operate on
> > +# @force: Disable safety checks (use with care)
> 
> Likewise.
1. Erase a slot which is already marked as
erased. Mostly harmless but pointless as well.

2. Erase last keyslot. This irreversibly destroys
any ability to read the data from the device,
unless a backup of the header and the key material is
done prior. Still can be useful when it is desired to
erase the data fast.


> 
> > +# @options:   Driver specific options
> > +#
> > +# Returns: @QCryptoKeyManageResult
> 
> Doc comment claims the command returns something, even though it
> doesn't.  Please fix.  Sadly, the doc generator fails to flag that.
This is leftover, fixed now although most likely this interface will die.
I was initially planning to return
information on which slot was allocated when user left that
decision to the driver.

> 
> > +#
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-erase-encryption',
> > +  'data': { 'node-name' : 'str',
> > +'*force' : 'bool',
> > +'options': 'QCryptoEncryptionSetupOptions' } }
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..69e8b086db 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -309,3 +309,29 @@
> >'base': 'QCryptoBlockInfoBase',
> >'discriminator': 'format',
> >'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> > +
> > +
> > +##
> > +# @QCryptoEncryptionSetupOptions:
> > +#
> > +# Driver specific options for encryption key management.
> 
> Specific to which driver?

This is the same issue, of not beeing able to detect an union.

I was planning to have an union here where we could add
add the driver specific options if we need to have another crypto driver,
however since I discovered that union needs user to pass the driver name,
I just placed it in a struct.

So this struct is supposed to represent driver specific options, but
currently contains only luks options.

> 
> > +#
> > +# @key-secret: the ID of a QCryptoSecret object providing the password
> > +#  to add or to erase (optional for erase)
> > +#
> > +# @old-key-secret: the ID of a QCryptoSecret object providing the password
> > +#  that can currently unlock the image
> > +#
> > +# @slot: Key slot to update/erase
> > +#(optional, for update will select a free slot,
> > +#for erase will era

Re: [Qemu-block] [PATCH 0/3] block/io_uring: fix EINTR and resubmit short reads

[John Snow]

On 7/15/19 4:19 PM, Stefan Hajnoczi wrote:
> Short reads are possible with cache=writeback (see Patch 3 for details).
> Handle this by resubmitting requests until the read is completed.
> 
> Patch 1 adds trace events useful for debugging io_uring.
> 
> Patch 2 fixes EINTR.  This lays the groundwork for resubmitting requests in
> Patch 3.
> 
> Aarushi: Feel free to squash this into your patch series if you are happy with
> the code, I don't mind if the authorship information is lost.  After applying
> these patches I can successfully boot a Fedora 30 guest qcow2 file with
> cache=writeback.
> 
> Based-on: <20190610134905.22294-1-mehta.aar...@gmail.com>
> 
> Stefan Hajnoczi (3):
>   block/io_uring: add submission and completion trace events
>   block/io_uring: fix EINTR request resubmission
>   block/io_uring: resubmit short buffered reads
> 
>  block/io_uring.c   | 136 ++---
>  block/trace-events |   6 +-
>  2 files changed, 108 insertions(+), 34 deletions(-)
> 

Since this is over the 30 days mark, I'm going to assume this WAS
squashed into Aarushi's patchset, and it's safe to drop this from the
review queue for now?

--js

Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Maxim Levitsky]

On Tue, 2019-08-20 at 19:59 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> 
> [...]
> 
> > Testing. This was lightly tested with manual testing and with few iotests 
> > that I prepared.
> > I haven't yet tested fully the write sharing behavior, nor did I run the 
> > whole iotests
> > suite to see if this code causes some regressions. Since I will need 
> > probably
> > to rewrite some chunks of it to change to 'amend' interface, I decided to 
> > post it now,
> > to see if you have other ideas/comments to add.
> 
> I can see that, because half of the qcow2 tests that contain the string
> “secret” break:
> 
> Failures: 087 134 158 178 188 198 206
> Failed 7 of 13 tests
> 
> Also, 210 when run with -luks.
> 
> Some are just due to different test outputs (because you change
> _filter_img_create to filter some encrypt.* parameters), but some of
> them are due to aborts.  All of them look like different kinds of heap
> corruptions.
> 
> 
> I can fully understand not running all iotests (because only the
> maintainers do that before pull requests), but just running the iotests
> that immediately concern a series seems prudent to me (unless the series
> is trivial).
> 
> (Just “(cd tests/qemu-iotests && grep -l secret ???)” tells you which
> tests to run that may concern themselves with qcow2 encryption, for
> example.)
> 
> 
> So I suppose I’ll stop reviewing the series in detail and just give a
> more cursory glance from now on.

Sorry about that! I posted this as RFC, and the reason it is mostly done as 
opposed to typical RFC which might not
even contain any code was that for most of the time I was sure that API of this 
is straightforward and won't need
any significant discussion, and in the last minute after I discussed with Kevin 
on IRC one 
obscure case of block backend permissions that was failing, he told me about 
the amend interface.
Next time I guess, when new a API is involved, I will post an API RFC first 
always and then start the implementation.

I fixed both issues that iotests uncovered locally, now all luks and most qcow2 
tests pass 
(118 and 194 sometimes fail with qcow2, and this happens regardless of my 
patches, and same for 162 which seems to fail
always now, also regardless of my patches.
I have a git head after the merge window opened so probably some bugs were 
added, and maybe already fixed)


The first issue was in 'qcrypto-luks: implement the encryption key management'
where I accidentally stored the name of the secret without strdup'ing in the 
create flow, so I got double free,
which indeed caused the heap corruptions you have seen.

Basically this line:
luks->secret = options->u.luks.key_secret;

The second issue as you mention is indeed the change in filters I did. Do you 
agree with that change btw?
If you ask me, I would even change the filter further and filter all the image 
options from the qemu command line since these are test inputs anyway.
This allowed me to have the same test for both luks and qcow2 luks encrypted 
test.

Also I didn't even expect you to run the iotests for me, but
rather just wanted a general RFC level feedback on the whole thing, that is why 
I even mentioned that I didn't run them.
So sorry for the trouble I caused!

I btw don't agree with you that only maintainers need to run all the iotests 
fully. 
I think that the patch submitter  should run all the tests that he can to catch 
as many problems as he can,
_unless_ of course this is an RFC.


Best regards,
Thanks for the review,
Sorry again for the trouble,

Maxim Levitsky

Re: [Qemu-block] QEMU bitmap backup usability FAQ

[John Snow]

On 8/21/19 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> [CC Nikolay]
> 
> 21.08.2019 1:25, John Snow wrote:
>> Hi, downstream here at Red Hat I've been fielding some questions about
>> the usability and feature readiness of Bitmaps (and related features) in
>> QEMU.
>>
>> Here are some questions I answered internally that I am copying to the
>> list for two reasons:
>>
>> (1) To make sure my answers are actually correct, and
>> (2) To share this pseudo-reference with the community at large.
>>
>> This is long, and mostly for reference. There's a summary at the bottom
>> with some todo items and observations about the usability of the feature
>> as it exists in QEMU.
>>
>> Before too long, I intend to send a more summarized "roadmap" mail which
>> details all of the current and remaining work to be done in and around
>> the bitmaps feature in QEMU.
>>
>>
>> Questions:
>>
>>> "What format(s) is/are required for this functionality?"
>>
>>  From the QEMU API, any format can be used to create and author
>> incremental backups. The only known format limitations are:
>>
>> 1. Persistent bitmaps cannot be created on any format except qcow2,
>> although there are hooks to add support to other formats at a later date
>> if desired.
>>
>> DANGER CAVEAT #1: Adding bitmaps to QEMU by default creates transient
>> bitmaps instead of persistent ones.
>>
>> Possible TODO: Allow users to 'upgrade' transient bitmaps to persistent
>> ones in case they made a mistake.
> 
> I doubt, as in my opinion real users of Qemu are not people but libvirt, which
> should never make such mistake.
> 

Right, that's largely been the consensus here; but there is some concern
that libvirt might not be the only user of QEMU, so I felt it was worth
documenting some of the crucial moments where it was "easy" to get it wrong.

I think like it or not, the API that QEMU presents has to be considered
on its own without libvirt because it's not a given that libvirt will
forever and always be the only user of QEMU.

I do think that any problems of this kind that can be solved in libvirt
are not immediate, crucial action items. libvirt WILL be the major user
of these features.

However, try as we might, releasing a set of primitive operations that
offer 998 ways to corrupt your data and 2 ways to manage it correctly
are going to provoke some questions from people who are trying to work
with that API, including from libvirt developers.

It might be the conclusion that it's libvirt's job to safeguard the user
from themselves, but we at least need to present consistent and clear
information about the way we expect/anticipate people to use the APIs,
because people DO keep asking me about several of these issues and the
usability problems they perceive with the QEMU API.

So this thread was largely in attempt to explore what some "solutions"
to perceived problems look like, mostly to come to the conclusion that
the actual "must-haves" list in QEMU is not very long compared to the
"nice-to-haves?" list.

>>
>>
>> 2. When using push backups (blockdev-backup, drive-backup), you may use
>> any format as a target format. >
>> DANGER CAVEAT #2: without backing file and/or filesystem-less sparse
>> support, these images will be unusable.
> 
> You mean incremental backups of course, as the whole document is about 
> bitmaps.
> 

Ah, yes, incremental push backups. Full backups are of course not a
problem. :)

>>
>> EXAMPLE: Backing up to a raw file loses allocation information, so we
>> can no longer distinguish between zeroes and unallocated regions. The
>> cluster size is also lost. This file will not be usable without
>> additional metadata recorded elsewhere.*
>>
>> (* This is complicated, but it is in theory possible to do a push backup
>> to e.g. an NBD target with custom server code that saves allocation
>> information to a metadata file, which would allow you to reconstruct
>> backups. For instance, recording in a .json file which extents were
>> written out would allow you to -- with a custom binary -- write this
>> information on top of a base file to reconstruct a backup.)
>>
>>
>> 3. Any format can be used for either shared storage or live storage
>> migrations. There are TWO distinct mechanisms for migrating bitmaps:
>>
>> A) The bitmap is flushed to storage and re-opened on the destination.
>> This is only supported for qcow2 and shared-storage migrations.
> 
> cons: flushing/reopening is done during migration downtime, so if you have
> a lot of bitmap data (for example, 64k granulared bitmap for 16tb disk is
> ~30MB, and there may be several bitmaps) downtime will become long.
> 

Worth documenting the drawback, yes.

>>
>> B) The bitmap is live-migrated to the destination. This is supported for
>> any format and can be used for either shared storage or live storage
>> migrations.
>>
>> DANGER CAVEAT #3: The second bitmap migration technique there is an
>> optional migration capability that must be enabled explicitly.
>> Otherwise, some migration

Re: [Qemu-block] [Qemu-devel] [PATCH] block/backup: install notifier during creation

[John Snow]

On 8/21/19 10:41 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 23:13, John Snow wrote:
>> Backup jobs may yield prior to installing their handler, because of the
>> job_co_entry shim which guarantees that a job won't begin work until
>> we are ready to start an entire transaction.
>>
>> Unfortunately, this makes proving correctness about transactional
>> points-in-time for backup hard to reason about. Make it explicitly clear
>> by moving the handler registration to creation time, and changing the
>> write notifier to a no-op until the job is started.
>>
>> Reported-by: Vladimir Sementsov-Ogievskiy 
>> Signed-off-by: John Snow 
>> ---
>>   block/backup.c | 32 +++-
>>   include/qemu/job.h |  5 +
>>   job.c  |  2 +-
>>   3 files changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index 07d751aea4..4df5b95415 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -344,6 +344,13 @@ static int coroutine_fn backup_before_write_notify(
>>   assert(QEMU_IS_ALIGNED(req->offset, BDRV_SECTOR_SIZE));
>>   assert(QEMU_IS_ALIGNED(req->bytes, BDRV_SECTOR_SIZE));
>>   
>> +/* The handler is installed at creation time; the actual point-in-time
>> + * starts at job_start(). Transactions guarantee those two points are
>> + * the same point in time. */
>> +if (!job_started(&job->common.job)) {
>> +return 0;
>> +}
> 
> Hmm, sorry if it is a stupid question, I'm not good in multiprocessing and in
> Qemu iothreads..
> 
> job_started just reads job->co. If bs runs in iothread, and therefore 
> write-notifier
> is in iothread, when job_start is called from main thread.. Is it guaranteed 
> that
> write-notifier will see job->co variable change early enough to not miss 
> guest write?
> Should not job->co be volatile for example or something like this?
> 
> If not think about this patch looks good for me.
> 

You know, it's a really good question.
So good, in fact, that I have no idea.

¯\_(ツ)_/¯

I'm fairly certain that IO will not come in until the .clean phase of a
qmp_transaction, because bdrv_drained_begin(bs) is called during
.prepare, and we activate the handler (by starting the job) in .commit.
We do not end the drained section until .clean.

I'm not fully clear on what threading guarantees we have otherwise,
though; is it possible that "Thread A" would somehow lift the bdrv_drain
on an IO thread ("Thread B") and, after that, "Thread B" would somehow
still be able to see an outdated version of job->co that was set by
"Thread A"?

I doubt it; but I can't prove it.

Paolo, may I please ask you for a consult here as our resident
volatility expert?

--js

>> +
>>   return backup_do_cow(job, req->offset, req->bytes, NULL, true);
>>   }
>>   
>> @@ -398,6 +405,12 @@ static void backup_clean(Job *job)
>>   BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>>   BlockDriverState *bs = blk_bs(s->common.blk);
>>   
>> +/* cancelled before job_start: remove write_notifier */
>> +if (s->before_write.notify) {
>> +notifier_with_return_remove(&s->before_write);
>> +s->before_write.notify = NULL;
>> +}
>> +
>>   if (s->copy_bitmap) {
>>   bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
>>   s->copy_bitmap = NULL;
>> @@ -527,17 +540,8 @@ static void backup_init_copy_bitmap(BackupBlockJob *job)
>>   static int coroutine_fn backup_run(Job *job, Error **errp)
>>   {
>>   BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>> -BlockDriverState *bs = blk_bs(s->common.blk);
>>   int ret = 0;
>>   
>> -QLIST_INIT(&s->inflight_reqs);
>> -qemu_co_rwlock_init(&s->flush_rwlock);
>> -
>> -backup_init_copy_bitmap(s);
>> -
>> -s->before_write.notify = backup_before_write_notify;
>> -bdrv_add_before_write_notifier(bs, &s->before_write);
>> -
>>   if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
>>   int64_t offset = 0;
>>   int64_t count;
>> @@ -572,6 +576,7 @@ static int coroutine_fn backup_run(Job *job, Error 
>> **errp)
>>   
>>out:
>>   notifier_with_return_remove(&s->before_write);
>> +s->before_write.notify = NULL;
>>   
>>   /* wait until pending backup_do_cow() calls have completed */
>>   qemu_co_rwlock_wrlock(&s->flush_rwlock);
>> @@ -767,6 +772,15 @@ BlockJob *backup_job_create(const char *job_id, 
>> BlockDriverState *bs,
>>  &error_abort);
>>   job->len = len;
>>   
>> +/* Finally, install a write notifier that takes effect after 
>> job_start() */
>> +backup_init_copy_bitmap(job);
>> +
>> +QLIST_INIT(&job->inflight_reqs);
>> +qemu_co_rwlock_init(&job->flush_rwlock);
>> +
>> +job->before_write.notify = backup_before_write_notify;
>> +bdrv_add_before_write_notifier(bs, &job->before_write);
>> +
>>   return &job->common;
>>   
>>error:
>> diff --git a/include/qemu/job.h b/include/qemu/job

Re: [Qemu-block] Broken aarch64 by qcow2: skip writing zero buffers to empty COW areas [v2]

[Max Reitz]

On 21.08.19 16:14, Lukáš Doktor wrote:
> Hello guys,
> 
> First attempt was rejected due to zip attachment, let's try it again with 
> just Avocado-vt debug.log and serial console log files attached.
> 
> I bisected a regression on aarch64 all the way to this commit: "qcow2: skip 
> writing zero buffers to empty COW areas" 
> c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. Would you please have a look at it?

I think I can see the issue on my x64 system (I don’t see the XFS
corruption, but the installation fails because of some segfaults).

I haven’t found a simpler way to reproduce the problem yet, though,
which is a pain... :-/

It looks like the problem disappears when I configure qemu with
“--disable-xfsctl”.  Can you try that?

Max

Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/8] iotests: Allow skipping test cases

[Andrey Shinkevich]

On 19/08/2019 23:18, Max Reitz wrote:
> case_notrun() does not actually skip the current test case.  It just
> adds a "notrun" note and then returns to the caller, who manually has to
> skip the test.  Generally, skipping a test case is as simple as
> returning from the current function, but not always: For example, this
> model does not allow skipping tests already in the setUp() function.
> 
> Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
> and then self.skipTest().  To make this work, we need to filter the
> information on how many test cases were skipped from the unittest
> output.
> 
> Signed-off-by: Max Reitz 
> ---
>   tests/qemu-iotests/iotests.py | 21 ++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 84438e837c..2f53baf633 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -802,6 +802,11 @@ class QMPTestCase(unittest.TestCase):
>   return self.pause_wait(job_id)
>   return result
>   
> +def case_skip(self, reason):
> +'''Skip this test case'''
> +case_notrun(reason)
> +self.skipTest(reason)
> +
>   
>   def notrun(reason):
>   '''Skip this test suite'''
> @@ -813,7 +818,10 @@ def notrun(reason):
>   sys.exit(0)
>   
>   def case_notrun(reason):
> -'''Skip this test case'''
> +'''Mark this test case as not having been run, but do not actually
> +skip it; that is left to the caller.  See QMPTestCase.case_skip()

The clause "do not actually skip it" sounds like a prescription. I would 
like the comment to be clearer for a reader that the method is a 
notifier only.

Andrey

> +for a variant that actually skips the current test case.'''
> +
>   # Each test in qemu-iotests has a number ("seq")
>   seq = os.path.basename(sys.argv[0])
>   
> @@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
>   unittest.main(testRunner=runner)
>   finally:
>   if not debug:
> -sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
> -r'Ran \1 tests', output.getvalue()))
> +out = output.getvalue()
> +out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', 
> out)
> +
> +# Hide skipped tests from the reference output
> +out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
> +out_first_line, out_rest = out.split('\n', 1)
> +out = out_first_line.replace('s', '.') + '\n' + out_rest
> +
> +sys.stderr.write(out)
>   
>   def execute_test(test_function=None,
>supported_fmts=[], supported_oses=['linux'],
> 

Reviewed-by: Andrey Shinkevich 
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/4] iotests: use python logging

[John Snow]

On 8/20/19 8:10 PM, no-re...@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190820235243.26092-1-js...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PATCH v3 0/4] iotests: use python logging
> Message-id: 20190820235243.26092-1-js...@redhat.com
> 

I have to remember that apparently git-publish does not seem to "save"
my setting for adding my signed-off-by if I don't explicitly request it.

Sorry about that. You may assume:

Signed-off-by: John Snow 

for all patches in this series.

Re: [Qemu-block] [PATCH v8 2/3] block/nbd: nbd reconnect

[Eric Blake]

On 8/21/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Implement reconnect. To achieve this:
> 
> 1. add new modes:
>connecting-wait: means, that reconnecting is in progress, and there
>  were small number of reconnect attempts, so all requests are
>  waiting for the connection.
>connecting-nowait: reconnecting is in progress, there were a lot of
>  attempts of reconnect, all requests will return errors.
> 
>two old modes are used too:
>connected: normal state
>quit: exiting after fatal error or on close
> 
> Possible transitions are:
> 
>* -> quit
>connecting-* -> connected
>connecting-wait -> connecting-nowait (transition is done after
>   reconnect-delay seconds in connecting-wait mode)
>connected -> connecting-wait
> 
> 2. Implement reconnect in connection_co. So, in connecting-* mode,
> connection_co, tries to reconnect unlimited times.
> 
> 3. Retry nbd queries on channel error, if we are in connecting-wait
> state.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

> +static bool nbd_client_connecting(BDRVNBDState *s)
> +{
> +return s->state == NBD_CLIENT_CONNECTING_WAIT ||
> +s->state == NBD_CLIENT_CONNECTING_NOWAIT;


Indentation looks unusual. I might have done:

return (s->state == NBD_CLIENT_CONNECTING_WAIT ||
s->state == NBD_CLIENT_CONNECTING_NOWAIT);

Or even exploit the enum encoding:

return s->state <= NBD_CLIENT_CONNECTING_NOWAIT

Is s->state updated atomically, or do we risk the case where we might
see two different values of s->state across the || sequence point?  Does
that matter?

> +}
> +
> +static bool nbd_client_connecting_wait(BDRVNBDState *s)
> +{
> +return s->state == NBD_CLIENT_CONNECTING_WAIT;
> +}
> +
> +static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
> +{
> +Error *local_err = NULL;
> +
> +if (!nbd_client_connecting(s)) {
> +return;
> +}
> +assert(nbd_client_connecting(s));

This assert adds nothing given the condition beforehand.

> +
> +/* Wait for completion of all in-flight requests */
> +
> +qemu_co_mutex_lock(&s->send_mutex);
> +
> +while (s->in_flight > 0) {
> +qemu_co_mutex_unlock(&s->send_mutex);
> +nbd_recv_coroutines_wake_all(s);
> +s->wait_in_flight = true;
> +qemu_coroutine_yield();
> +s->wait_in_flight = false;
> +qemu_co_mutex_lock(&s->send_mutex);
> +}
> +
> +qemu_co_mutex_unlock(&s->send_mutex);
> +
> +if (!nbd_client_connecting(s)) {
> +return;
> +}
> +
> +/*
> + * Now we are sure that nobody is accessing the channel, and no one will
> + * try until we set the state to CONNECTED.
> + */
> +
> +/* Finalize previous connection if any */
> +if (s->ioc) {
> +nbd_client_detach_aio_context(s->bs);
> +object_unref(OBJECT(s->sioc));
> +s->sioc = NULL;
> +object_unref(OBJECT(s->ioc));
> +s->ioc = NULL;
> +}
> +
> +s->connect_status = nbd_client_connect(s->bs, &local_err);
> +error_free(s->connect_err);
> +s->connect_err = NULL;
> +error_propagate(&s->connect_err, local_err);
> +local_err = NULL;
> +
> +if (s->connect_status < 0) {
> +/* failed attempt */
> +return;
> +}
> +
> +/* successfully connected */
> +s->state = NBD_CLIENT_CONNECTED;
> +qemu_co_queue_restart_all(&s->free_sema);
> +}
> +
> +static coroutine_fn void nbd_reconnect_loop(BDRVNBDState *s)
> +{
> +uint64_t start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +uint64_t delay_ns = s->reconnect_delay * NANOSECONDS_PER_SECOND;
> +uint64_t timeout = 1 * NANOSECONDS_PER_SECOND;
> +uint64_t max_timeout = 16 * NANOSECONDS_PER_SECOND;
> +
> +nbd_reconnect_attempt(s);
> +
> +while (nbd_client_connecting(s)) {
> +if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
> +qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > 
> delay_ns)
> +{
> +s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> +qemu_co_queue_restart_all(&s->free_sema);
> +}
> +
> +qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, timeout,
> + &s->connection_co_sleep_ns_state);
> +if (s->drained) {
> +bdrv_dec_in_flight(s->bs);
> +s->wait_drained_end = true;
> +while (s->drained) {
> +/*
> + * We may be entered once from 
> nbd_client_attach_aio_context_bh
> + * and then from nbd_client_co_drain_end. So here is a loop.
> + */
> +qemu_coroutine_yield();
> +}
> +bdrv_inc_in_flight(s->bs);
> +}
> +if (timeout < max_timeout) {
> +timeout *= 2;
> +}
> +
> +nbd_reconnect_attempt(s);
> +}
>  }
>  
>  static coroutine_fn void nbd_connection_entry(void *opaque)
>  {
> -BDRVNBDState *s = opaque;
> 

Re: [Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests

[Andrey Shinkevich]

On 19/08/2019 12:21, Thomas Huth wrote:
> The python code already contains a possibility to skip tests if the
> corresponding driver is not available in the qemu binary - use it
> in more spots to avoid that the tests are failing if the driver has
> been disabled.
> 
> Signed-off-by: Thomas Huth 
> ---
>   tests/qemu-iotests/030 |  3 +++
>   tests/qemu-iotests/040 |  2 ++
>   tests/qemu-iotests/041 | 14 +-
>   tests/qemu-iotests/245 |  2 ++
>   4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 1b69f318c6..18ad24ccdf 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase):
>   children = []
>   backing = []
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def setUp(self):

Based on the Max's series "iotests: Selfish patches",
this '+' suffice for the whole methods of the class.
So, I can confirm after testing it.

>   opts = ['driver=quorum', 'vote-threshold=2']
>   
> @@ -552,6 +553,7 @@ class TestQuorum(iotests.QMPTestCase):
>   self.vm.add_drive(path = None, opts = ','.join(opts))
>   self.vm.launch()
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def tearDown(self):
>   self.vm.shutdown()
>   for img in self.children:
> @@ -559,6 +561,7 @@ class TestQuorum(iotests.QMPTestCase):
>   for img in self.backing:
>   os.remove(img)
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def test_stream_quorum(self):
>   if not iotests.supports_quorum():
>   return
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index aa0b1847e3..0ec4fb71a9 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -110,6 +110,7 @@ class TestSingleDrive(ImageCommitTestCase):
>   self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 
> 524288', backing_img).find("verification failed"))
>   self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 
> 524288 524288', backing_img).find("verification failed"))
>   
> +@iotests.skip_if_unsupported(['throttle'])
>   def test_commit_with_filter_and_quit(self):
>   result = self.vm.qmp('object-add', qom_type='throttle-group', 
> id='tg')
>   self.assert_qmp(result, 'return', {})
> @@ -129,6 +130,7 @@ class TestSingleDrive(ImageCommitTestCase):
>   self.has_quit = True
>   
>   # Same as above, but this time we add the filter after starting the job
> +@iotests.skip_if_unsupported(['throttle'])
>   def test_commit_plus_filter_and_quit(self):
>   result = self.vm.qmp('object-add', qom_type='throttle-group', 
> id='tg')
>   self.assert_qmp(result, 'return', {})
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 26bf1701eb..f45d20fbe0 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   image_len = 1 * 1024 * 1024 # MB
>   IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def setUp(self):

Also here, as Max wrote. So, this series can be simplified and go after 
the "iotests: Selfish patches".

Andrey

>   self.vm = iotests.VM()
>   
> @@ -841,7 +842,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   result = self.vm.qmp("blockdev-add", **args)
>   self.assert_qmp(result, 'return', {})
>   
> -
> +@iotests.skip_if_unsupported(['quorum'])
>   def tearDown(self):
>   self.vm.shutdown()
>   for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
> @@ -851,6 +852,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   except OSError:
>   pass
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def test_complete(self):
>   if not iotests.supports_quorum():
>   return
> @@ -870,6 +872,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   self.assertTrue(iotests.compare_images(quorum_img2, 
> quorum_repair_img),
>   'target image does not match source after 
> mirroring')
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def test_cancel(self):
>   if not iotests.supports_quorum():
>   return
> @@ -887,6 +890,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   self.assert_has_block_node(None, quorum_img3)
>   self.vm.shutdown()
>   
> +@iotests.skip_if_unsupported(['quorum'])
>   def test_cancel_after_ready(self):
>   if not iotests.supports_quorum():
>   return
> @@ -906,6 +910,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>   self.assertTrue(iotests.compare_images(quorum_img2, 
> quorum_repair_img),
>   'target image does not match source after 
> mirroring')
>   
>

Re: [Qemu-block] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Eric Blake]

On 8/21/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce a function to gracefully wake a coroutine sleeping in
> qemu_co_sleep_ns().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---

I'd like a second reviewer on this, but I'll at least give it a spin.

>  include/qemu/coroutine.h| 17 --
>  block/null.c|  2 +-
>  block/sheepdog.c|  2 +-
>  tests/test-bdrv-drain.c |  6 ++---
>  tests/test-block-iothread.c |  2 +-
>  util/qemu-coroutine-sleep.c | 47 +++--
>  6 files changed, 55 insertions(+), 21 deletions(-)

This merely updates existing callers to pass in NULL for the new
argument.  I'd feel a lot better if one of the two tests/ changes also
added a test passing a non-NULL sleep_state, and demonstrated its use.

> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f5a4..96780a4902 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -274,9 +274,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
>  void qemu_co_rwlock_unlock(CoRwlock *lock);
>  
>  /**
> - * Yield the coroutine for a given duration
> + * Yield the coroutine for a given duration. During this yield @sleep_state 
> (if

s/yield/yield,/

> + * not NULL) is set to opaque pointer, which may be used for

s/to/to an/

> + * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when 
> timer
> + * shoots. Don't save obtained value to other variables and don't call

s/when timer shoots/when the timer fires/

s/save/save/the/

> + * qemu_co_sleep_wake from another aio context.
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
> +   void **sleep_state);
> +
> +/**
> + * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be

s/by/in/
s/Timer/The timer/

> + * deleted. @sleep_state must be the variable which address was given to

s/which/whose/

> + * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
> + * qemu_co_sleep_wake().
> + */
> +void qemu_co_sleep_wake(void *sleep_state);
>  

> +++ b/util/qemu-coroutine-sleep.c
> @@ -17,31 +17,52 @@
>  #include "qemu/timer.h"
>  #include "block/aio.h"
>  
> -static void co_sleep_cb(void *opaque)
> -{
> -Coroutine *co = opaque;
> +const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";

Why is this not marked static?

> +
> +typedef struct QemuCoSleepState {
> +Coroutine *co;
> +QEMUTimer *ts;
> +void **user_state_pointer;
> +} QemuCoSleepState;
>  
> +void qemu_co_sleep_wake(void *sleep_state)
> +{
> +QemuCoSleepState *s = (QemuCoSleepState *)sleep_state;

This is C; the cast is not necessary.

>  /* Write of schedule protected by barrier write in aio_co_schedule */
> -atomic_set(&co->scheduled, NULL);
> -aio_co_wake(co);
> +const char *scheduled = atomic_cmpxchg(&s->co->scheduled,
> +   qemu_co_sleep_ns__scheduled, 
> NULL);
> +
> +assert(scheduled == qemu_co_sleep_ns__scheduled);
> +if (s->user_state_pointer) {
> +*s->user_state_pointer = NULL;
> +}
> +timer_del(s->ts);
> +aio_co_wake(s->co);
>  }
>  
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
> +   void **sleep_state)
>  {
>  AioContext *ctx = qemu_get_current_aio_context();
> -QEMUTimer *ts;
> -Coroutine *co = qemu_coroutine_self();
> +QemuCoSleepState state = {
> +.co = qemu_coroutine_self(),
> +.ts = aio_timer_new(ctx, type, SCALE_NS, qemu_co_sleep_wake, &state),
> +.user_state_pointer = sleep_state,
> +};
>  
> -const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
> +const char *scheduled = atomic_cmpxchg(&state.co->scheduled, NULL,
> +   qemu_co_sleep_ns__scheduled);
>  if (scheduled) {
>  fprintf(stderr,
>  "%s: Co-routine was already scheduled in '%s'\n",
>  __func__, scheduled);
>  abort();
>  }
> -ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
> -timer_mod(ts, qemu_clock_get_ns(type) + ns);
> +
> +if (sleep_state) {
> +*sleep_state = &state;
> +}
> +timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
>  qemu_coroutine_yield();
> -timer_del(ts);
> -timer_free(ts);
> +timer_free(state.ts);
>  }

Grammar changes are trivial; and while it is not the most trivial of
patches, I at least follow what it is doing.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block: gluster: Probe alignment limits

[Max Reitz]

On 17.08.19 23:21, Nir Soffer wrote:
> Implement alignment probing similar to file-posix, by reading from the
> first 4k of the image.
> 
> Before this change, provisioning a VM on storage with sector size of
> 4096 bytes would fail when the installer try to create filesystems. Here
> is an example command that reproduces this issue:
> 
> $ qemu-system-x86_64 -accel kvm -m 2048 -smp 2 \
> -drive file=gluster://gluster1/gv0/fedora29.raw,format=raw,cache=none 
> \
> -cdrom Fedora-Server-dvd-x86_64-29-1.2.iso
> 
> The installer fails in few seconds when trying to create filesystem on
> /dev/mapper/fedora-root. In error report we can see that it failed with
> EINVAL (I could not extract the error from guest).
> 
> Copying disk fails with EINVAL:
> 
> $ qemu-img convert -p -f raw -O raw -t none -T none \
> gluster://gluster1/gv0/fedora29.raw \
> gluster://gluster1/gv0/fedora29-clone.raw
> qemu-img: error while writing sector 4190208: Invalid argument
> 
> This is a fix to same issue fixed in commit a6b257a08e3d (file-posix:
> Handle undetectable alignment) for gluster:// images.
> 
> This fix has the same limit, that the first block of the image should be
> allocated, otherwise we cannot detect the alignment and fallback to a
> safe value (4096) even when using storage with sector size of 512 bytes.
> 
> Signed-off-by: Nir Soffer 
> ---
>  block/gluster.c | 47 +++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index f64dc5b01e..d936240b72 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -52,6 +52,9 @@
>  
>  #define GERR_INDEX_HINT "hint: check in 'server' array index '%d'\n"
>  
> +/* The value is known only on the server side. */
> +#define MAX_ALIGN 4096
> +
>  typedef struct GlusterAIOCB {
>  int64_t size;
>  int ret;
> @@ -902,8 +905,52 @@ out:
>  return ret;
>  }
>  
> +/*
> + * Check if read is allowed with given memory buffer and length.
> + *
> + * This function is used to check O_DIRECT request alignment.
> + */
> +static bool gluster_is_io_aligned(struct glfs_fd *fd, void *buf, size_t len)
> +{
> +ssize_t ret = glfs_pread(fd, buf, len, 0, 0, NULL);
> +return ret >= 0 || errno != EINVAL;

Is glfs_pread() guaranteed to return EINVAL on invalid alignment?
file-posix says this is only the case on Linux (for normal files).  Now
I also don’t know whether the gluster driver works on anything but Linux
anyway.

> +}
> +
> +static void gluster_probe_alignment(BlockDriverState *bs, struct glfs_fd *fd,
> +Error **errp)
> +{
> +char *buf;
> +size_t alignments[] = {1, 512, 1024, 2048, 4096};
> +size_t align;
> +int i;
> +
> +buf = qemu_memalign(MAX_ALIGN, MAX_ALIGN);
> +
> +for (i = 0; i < ARRAY_SIZE(alignments); i++) {
> +align = alignments[i];
> +if (gluster_is_io_aligned(fd, buf, align)) {
> +/* Fallback to safe value. */
> +bs->bl.request_alignment = (align != 1) ? align : MAX_ALIGN;
> +break;
> +}
> +}

I don’t like the fact that the last element of alignments[] should be
the same as MAX_ALIGN, without that ever having been made explicit anywhere.

It’s a bit worse in the file-posix patch, because if getpagesize() is
greater than 4k, max_align will be greater than 4k.  But MAX_BLOCKSIZE
is 4k, too, so I suppose we wouldn’t support any block size beyond that
anyway, which makes the error message appropriate still.

> +
> +qemu_vfree(buf);
> +
> +if (!bs->bl.request_alignment) {
> +error_setg(errp, "Could not find working O_DIRECT alignment");
> +error_append_hint(errp, "Try cache.direct=off\n");
> +}
> +}
> +
>  static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
> +BDRVGlusterState *s = bs->opaque;
> +
> +gluster_probe_alignment(bs, s->fd, errp);
> +
> +bs->bl.min_mem_alignment = bs->bl.request_alignment;

Well, I’ll just trust you that there is no weird system where the memory
alignment is greater than the request alignment.

> +bs->bl.opt_mem_alignment = MAX(bs->bl.request_alignment, MAX_ALIGN);

Isn’t request_alignment guaranteed to not exceed MAX_ALIGN, i.e. isn’t
this always MAX_ALIGN?

>  bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
>  }

file-posix has a check in raw_reopen_prepare() whether we can find a
working alignment for the new FD.  Shouldn’t we do the same in
qemu_gluster_reopen_prepare()?

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v8 3/3] iotests: test nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Add test, which starts backup to nbd target and restarts nbd server
during backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/264| 65 +++
 tests/qemu-iotests/264.out| 12 +++
 tests/qemu-iotests/group  |  1 +
 tests/qemu-iotests/iotests.py |  4 +++
 4 files changed, 82 insertions(+)
 create mode 100755 tests/qemu-iotests/264
 create mode 100644 tests/qemu-iotests/264.out

diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
new file mode 100755
index 00..e70f91c5ca
--- /dev/null
+++ b/tests/qemu-iotests/264
@@ -0,0 +1,65 @@
+#!/usr/bin/env python
+#
+# Test nbd reconnect
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import time
+
+import iotests
+from iotests import qemu_img_create, file_path, qemu_nbd_popen, log
+
+disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
+
+qemu_img_create('-f', iotests.imgfmt, disk_a, '5M')
+qemu_img_create('-f', iotests.imgfmt, disk_b, '5M')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+time.sleep(1)
+
+vm = iotests.VM().add_drive(disk_a)
+vm.launch()
+vm.hmp_qemu_io('drive0', 'write 0 5M')
+
+vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
+   **{'node_name': 'backup0',
+  'driver': 'raw',
+  'file': {'driver': 'nbd',
+   'server': {'type': 'unix', 'path': nbd_sock},
+   'reconnect-delay': 10}})
+vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
+   speed=(1 * 1024 * 1024))
+
+time.sleep(1)
+log('Kill NBD server')
+srv.kill()
+
+jobs = vm.qmp('query-block-jobs')['return']
+if jobs and jobs[0]['offset'] < jobs[0]['len']:
+log('Backup job is still in progress')
+
+time.sleep(1)
+
+log('Start NBD server')
+srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
+
+vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
+e = vm.event_wait('BLOCK_JOB_COMPLETED')
+log('Backup completed: {}'.format(e['data']['offset']))
+
+vm.qmp_log('blockdev-del', node_name='backup0')
+srv.kill()
+vm.shutdown()
diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
new file mode 100644
index 00..4a2f4aa509
--- /dev/null
+++ b/tests/qemu-iotests/264.out
@@ -0,0 +1,12 @@
+{"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": 
"nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", 
"type": "unix"}}, "node-name": "backup0"}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 
1048576, "sync": "full", "target": "backup0"}}
+{"return": {}}
+Kill NBD server
+Backup job is still in progress
+Start NBD server
+{"execute": "block-job-set-speed", "arguments": {"device": "drive0", "speed": 
0}}
+{"return": {}}
+Backup completed: 5242880
+{"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
+{"return": {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..34c2b89108 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,3 +274,4 @@
 257 rw
 258 rw quick
 262 rw quick migration
+264 rw quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 84438e837c..ab2f8f7d1b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -229,6 +229,10 @@ def qemu_nbd_early_pipe(*args):
 else:
 return exitcode, subp.communicate()[0]
 
+def qemu_nbd_popen(*args):
+'''Run qemu-nbd in daemon mode and return the parent's exit code'''
+return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
 '''Return True if two image files are identical'''
 return qemu_img('compare', '-f', fmt1,
-- 
2.18.0

[Qemu-block] [PATCH v8 2/3] block/nbd: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

Implement reconnect. To achieve this:

1. add new modes:
   connecting-wait: means, that reconnecting is in progress, and there
 were small number of reconnect attempts, so all requests are
 waiting for the connection.
   connecting-nowait: reconnecting is in progress, there were a lot of
 attempts of reconnect, all requests will return errors.

   two old modes are used too:
   connected: normal state
   quit: exiting after fatal error or on close

Possible transitions are:

   * -> quit
   connecting-* -> connected
   connecting-wait -> connecting-nowait (transition is done after
  reconnect-delay seconds in connecting-wait mode)
   connected -> connecting-wait

2. Implement reconnect in connection_co. So, in connecting-* mode,
connection_co, tries to reconnect unlimited times.

3. Retry nbd queries on channel error, if we are in connecting-wait
state.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 335 ++--
 1 file changed, 271 insertions(+), 64 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index beed46fb34..f272154d4b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Block driver for  NBD
  *
+ * Copyright (c) 2019 Virtuozzo International GmbH.
  * Copyright (C) 2016 Red Hat, Inc.
  * Copyright (C) 2008 Bull S.A.S.
  * Author: Laurent Vivier 
@@ -55,6 +56,8 @@ typedef struct {
 } NBDClientRequest;
 
 typedef enum NBDClientState {
+NBD_CLIENT_CONNECTING_WAIT,
+NBD_CLIENT_CONNECTING_NOWAIT,
 NBD_CLIENT_CONNECTED,
 NBD_CLIENT_QUIT
 } NBDClientState;
@@ -67,8 +70,14 @@ typedef struct BDRVNBDState {
 CoMutex send_mutex;
 CoQueue free_sema;
 Coroutine *connection_co;
+void *connection_co_sleep_ns_state;
+bool drained;
+bool wait_drained_end;
 int in_flight;
 NBDClientState state;
+int connect_status;
+Error *connect_err;
+bool wait_in_flight;
 
 NBDClientRequest requests[MAX_NBD_REQUESTS];
 NBDReply reply;
@@ -83,10 +92,21 @@ typedef struct BDRVNBDState {
 char *x_dirty_bitmap;
 } BDRVNBDState;
 
-/* @ret will be used for reconnect in future */
+static int nbd_client_connect(BlockDriverState *bs, Error **errp);
+
 static void nbd_channel_error(BDRVNBDState *s, int ret)
 {
-s->state = NBD_CLIENT_QUIT;
+if (ret == -EIO) {
+if (s->state == NBD_CLIENT_CONNECTED) {
+s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
+NBD_CLIENT_CONNECTING_NOWAIT;
+}
+} else {
+if (s->state == NBD_CLIENT_CONNECTED) {
+qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+s->state = NBD_CLIENT_QUIT;
+}
 }
 
 static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
@@ -129,7 +149,13 @@ static void nbd_client_attach_aio_context(BlockDriverState 
*bs,
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 
-qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
+/*
+ * s->connection_co is either yielded from nbd_receive_reply or from
+ * nbd_reconnect_loop()
+ */
+if (s->state == NBD_CLIENT_CONNECTED) {
+qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
+}
 
 bdrv_inc_in_flight(bs);
 
@@ -140,29 +166,157 @@ static void 
nbd_client_attach_aio_context(BlockDriverState *bs,
 aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
 }
 
+static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
+{
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 
-static void nbd_teardown_connection(BlockDriverState *bs)
+s->drained = true;
+if (s->connection_co_sleep_ns_state) {
+qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
+}
+}
+
+static void coroutine_fn nbd_client_co_drain_end(BlockDriverState *bs)
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 
-assert(s->ioc);
+s->drained = false;
+if (s->wait_drained_end) {
+s->wait_drained_end = false;
+aio_co_wake(s->connection_co);
+}
+}
+
 
-/* finish any pending coroutines */
-qio_channel_shutdown(s->ioc,
- QIO_CHANNEL_SHUTDOWN_BOTH,
- NULL);
+static void nbd_teardown_connection(BlockDriverState *bs)
+{
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+
+if (s->state == NBD_CLIENT_CONNECTED) {
+/* finish any pending coroutines */
+assert(s->ioc);
+qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+s->state = NBD_CLIENT_QUIT;
+if (s->connection_co) {
+if (s->connection_co_sleep_ns_state) {
+qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
+}
+}
 BDRV_POLL_WHILE(bs, s->connection_co);
+}
 
-nbd_client_detach_aio_context(bs);
-object_unref(OBJECT(s->sioc));
-s->sioc = NULL;
-object_unref(OBJECT(s->ioc));
-s->ioc = NULL;
+

[Qemu-block] [PATCH v8 0/3] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Hi all!
Here is NBD reconnect. Previously, if connection failed all current
and future requests will fail. After the series, nbd-client driver
will try to reconnect unlimited times. During first @reconnect-delay
seconds of reconnecting all requests will wait for the connection,
and if it is established requests will be resent. After
@reconnect-delay period all requests will be failed (until successful
reconnect).

v8:
preparations are already merged [thx to Eric], old 07 with SI_* constants
dropped [Peter]
02: - use NANOSECONDS_PER_SECOND
03: - move to tests/qemu-iotests/264
- limit job speed, otherwise it fails on ramfs as backup finishes too early

v7:
almost all: rebased on merged nbd.c and nbd-client.c (including patch subject)
01-04: add Eric's r-b
04: wording
05: new
06: rewrite to remove timer earlier
07: new
08:
 - rebase on 05 and 07
 - drop "All rights reserved"
 - handle drain
 - improve handling aio context attach
09: move 249 -> 257

Vladimir Sementsov-Ogievskiy (3):
  qemu-coroutine-sleep: introduce qemu_co_sleep_wake
  block/nbd: nbd reconnect
  iotests: test nbd reconnect

 include/qemu/coroutine.h  |  17 +-
 block/nbd.c   | 335 +++---
 block/null.c  |   2 +-
 block/sheepdog.c  |   2 +-
 tests/test-bdrv-drain.c   |   6 +-
 tests/test-block-iothread.c   |   2 +-
 util/qemu-coroutine-sleep.c   |  47 +++--
 tests/qemu-iotests/264|  65 +++
 tests/qemu-iotests/264.out|  12 ++
 tests/qemu-iotests/group  |   1 +
 tests/qemu-iotests/iotests.py |   4 +
 11 files changed, 408 insertions(+), 85 deletions(-)
 create mode 100755 tests/qemu-iotests/264
 create mode 100644 tests/qemu-iotests/264.out

-- 
2.18.0

[Qemu-block] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

Introduce a function to gracefully wake a coroutine sleeping in
qemu_co_sleep_ns().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/qemu/coroutine.h| 17 --
 block/null.c|  2 +-
 block/sheepdog.c|  2 +-
 tests/test-bdrv-drain.c |  6 ++---
 tests/test-block-iothread.c |  2 +-
 util/qemu-coroutine-sleep.c | 47 +++--
 6 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9801e7f5a4..96780a4902 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -274,9 +274,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
 void qemu_co_rwlock_unlock(CoRwlock *lock);
 
 /**
- * Yield the coroutine for a given duration
+ * Yield the coroutine for a given duration. During this yield @sleep_state (if
+ * not NULL) is set to opaque pointer, which may be used for
+ * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when timer
+ * shoots. Don't save obtained value to other variables and don't call
+ * qemu_co_sleep_wake from another aio context.
  */
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
+void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
+   void **sleep_state);
+
+/**
+ * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
+ * deleted. @sleep_state must be the variable which address was given to
+ * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
+ * qemu_co_sleep_wake().
+ */
+void qemu_co_sleep_wake(void *sleep_state);
 
 /**
  * Yield until a file descriptor becomes readable
diff --git a/block/null.c b/block/null.c
index 699aa295cb..1e3f26b07e 100644
--- a/block/null.c
+++ b/block/null.c
@@ -109,7 +109,7 @@ static coroutine_fn int null_co_common(BlockDriverState *bs)
 BDRVNullState *s = bs->opaque;
 
 if (s->latency_ns) {
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns, NULL);
 }
 return 0;
 }
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 773dfc6ab1..3a7ef2f209 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -743,7 +743,7 @@ static coroutine_fn void reconnect_to_sdog(void *opaque)
 if (s->fd < 0) {
 trace_sheepdog_reconnect_to_sdog();
 error_report_err(local_err);
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10ULL);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10ULL, NULL);
 }
 };
 
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 374bef6bb2..2f53a7add5 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -43,7 +43,7 @@ static void coroutine_fn 
bdrv_test_co_drain_begin(BlockDriverState *bs)
 BDRVTestState *s = bs->opaque;
 s->drain_count++;
 if (s->sleep_in_drain_begin) {
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10, NULL);
 }
 }
 
@@ -74,7 +74,7 @@ static int coroutine_fn bdrv_test_co_preadv(BlockDriverState 
*bs,
  * it to complete. We need to sleep a while as bdrv_drain_invoke() comes
  * first and polls its result, too, but it shouldn't accidentally complete
  * this request yet. */
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 10, NULL);
 
 if (s->bh_indirection_ctx) {
 aio_bh_schedule_oneshot(s->bh_indirection_ctx, co_reenter_bh,
@@ -829,7 +829,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
 /* Avoid job_sleep_ns() because it marks the job as !busy. We want to
  * emulate some actual activity (probably some I/O) here so that drain
  * has to wait for this activity to stop. */
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100, NULL);
 
 job_pause_point(&s->common.job);
 }
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index 926577b1f9..a1ac5efcaa 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -381,7 +381,7 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
  * emulate some actual activity (probably some I/O) here so that the
  * drain involved in AioContext switches has to wait for this activity
  * to stop. */
-qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100);
+qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100, NULL);
 
 job_pause_point(&s->common.job);
 }
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 4bfdd30cbf..48a64bb8d8 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -17,31 +17,52 @@
 #include "qemu/timer.h"
 #include "block/aio.h"
 
-static void co_sleep_cb(void *opaque)
-{
-Coroutine *co = opaque;
+const char *qemu_co_sleep_ns__scheduled = "qemu_

[Qemu-block] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change

[Damien Hedde]

Provide a temporary device_legacy_reset function doing what
device_reset does to prepare for the transition with Resettable
API.

All occurrence of device_reset in the code tree are also replaced
by device_legacy_reset.

The new resettable API has different prototype and semantics
(resetting child buses as well as the specified device). Subsequent
commits will make the changeover for each call site individually; once
that is complete device_legacy_reset() will be removed.

Signed-off-by: Damien Hedde 
Reviewed-by: Peter Maydell 
---
Cc: Gerd Hoffmann 
Cc: Paolo Bonzini 
Cc: "Daniel P. Berrangé" 
Cc: Eduardo Habkost 
Cc: Richard Henderson 
Cc: "Michael S. Tsirkin" 
Cc: Marcel Apfelbaum 
Cc: John Snow 
Cc: "Cédric Le Goater" 
Cc: Collin Walling 
Cc: Cornelia Huck 
Cc: David Hildenbrand 
Cc: Halil Pasic 
Cc: Christian Borntraeger 
Cc: Dmitry Fleytman 
Cc: Fam Zheng 
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-s3...@nongnu.org
Cc: qemu-...@nongnu.org
---
 hw/audio/intel-hda.c | 2 +-
 hw/core/qdev.c   | 6 +++---
 hw/hyperv/hyperv.c   | 2 +-
 hw/i386/pc.c | 2 +-
 hw/ide/microdrive.c  | 8 
 hw/intc/spapr_xive.c | 2 +-
 hw/ppc/pnv_psi.c | 2 +-
 hw/ppc/spapr_pci.c   | 2 +-
 hw/ppc/spapr_vio.c   | 2 +-
 hw/s390x/s390-pci-inst.c | 2 +-
 hw/scsi/vmw_pvscsi.c | 2 +-
 hw/sd/omap_mmc.c | 2 +-
 hw/sd/pl181.c| 2 +-
 include/hw/qdev-core.h   | 4 ++--
 14 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6ecd383540..27b71c57cf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1087,7 +1087,7 @@ static void intel_hda_reset(DeviceState *dev)
 QTAILQ_FOREACH(kid, &d->codecs.qbus.children, sibling) {
 DeviceState *qdev = kid->child;
 cdev = HDA_CODEC_DEVICE(qdev);
-device_reset(DEVICE(cdev));
+device_legacy_reset(DEVICE(cdev));
 d->state_sts |= (1 << cdev->cad);
 }
 intel_hda_update_irq(d);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 60d66c2f39..a95d4fa87d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -257,7 +257,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
-device_reset(dev);
+device_legacy_reset(dev);
 
 return 0;
 }
@@ -865,7 +865,7 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 }
 }
 if (dev->hotplugged) {
-device_reset(dev);
+device_legacy_reset(dev);
 }
 dev->pending_deleted_event = false;
 
@@ -1087,7 +1087,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
 dc->unrealize = dev_unrealize;
 }
 
-void device_reset(DeviceState *dev)
+void device_legacy_reset(DeviceState *dev)
 {
 DeviceClass *klass = DEVICE_GET_CLASS(dev);
 
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 6ebf31c310..cd9db3cb5c 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
 SynICState *synic = get_synic(cs);
 
 if (synic) {
-device_reset(DEVICE(synic));
+device_legacy_reset(DEVICE(synic));
 }
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3ab4bcb3ca..f33a8de42f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2826,7 +2826,7 @@ static void pc_machine_reset(MachineState *machine)
 cpu = X86_CPU(cs);
 
 if (cpu->apic_state) {
-device_reset(cpu->apic_state);
+device_legacy_reset(cpu->apic_state);
 }
 }
 }
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index b0272ea14b..6b30e36ed8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t 
at, uint8_t value)
 case 0x00: /* Configuration Option Register */
 s->opt = value & 0xcf;
 if (value & OPT_SRESET) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
 }
 md_interrupt_update(s);
 break;
@@ -316,7 +316,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t 
at, uint16_t value)
 case 0xe:  /* Device Control */
 s->ctrl = value;
 if (value & CTRL_SRST) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
 }
 md_interrupt_update(s);
 break;
@@ -541,7 +541,7 @@ static int dscm1_attach(PCMCIACardState *card)
 md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
 md->io_base = 0x0;
 
-device_reset(DEVICE(md));
+device_legacy_reset(DEVICE(md));
 md_interrupt_update(md);
 
 return 0;
@@ -551,7 +551,7 @@ static int dscm1_detach(PCMCIACardState *card)
 {
 MicroDriveState *md = MICRODRIVE(card);
 
-device_reset(DEVICE(md));
+device_legacy_reset(DEVICE(md));
 return 0;
 }
 
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_x

Re: [Qemu-block] Broken aarch64 by qcow2: skip writing zero buffers to empty COW areas [v2]

[Lukáš Doktor]

Dne 21. 08. 19 v 17:49 Anton Nefedov napsal(a):
> On 21/8/2019 5:14 PM, Lukáš Doktor wrote:
>> Hello guys,
>>
>> First attempt was rejected due to zip attachment, let's try it again with 
>> just Avocado-vt debug.log and serial console log files attached.
>>
>> I bisected a regression on aarch64 all the way to this commit: "qcow2: skip 
>> writing zero buffers to empty COW areas" 
>> c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. Would you please have a look at it?
>>
>> My reproducer is running kickstart installation of RHEL-8 from DVD on 
>> aarch64 gicv3 machine, which never finishes since this commit, where 
>> anaconda complains about package installation, occasionally there are also 
>> XFS metadata corruption messages on serial console:
>>
> 
> hi,
> 
> this looks scary :( I doubt that it can have anything to do with aarch64
> but rather a really tricky timing (or, possibly, a broken environment
> like broken fallocate() on a host? who knows..)
> 
> Is it always the same machine you observe this issue on? Did you try
> others?
> 
> I just wonder if it's worth to try to reproduce it on my machine
> (and I don't have aarch64 on hand now). I can probably come up with
> some torture test that will continuously write to qcow2 with random
> offsets/sizes and verify the result.
> 
> If you could kindly reproduce it again then we can probably start with
> enabling qemu traces by appending
>   " -trace bdrv* -trace qcow2* -trace file=/some_huge_partition/qemu.log"
> to the command line.
> 
> Beware that it's going to produce a huge amount of logs.
> 
> Also, the corrupted image and the serial log will be required for
> investigation.
> 
> thanks,
> 
> /Anton
> 

Hello Anton,

I have only tried that on a single machine, but colleague of mine reported 
similar issues even on TCG installing Fedora using x86_64 host. I'll try to 
reproduce it on my x86_64 box which should simplify the debugging.

Lukáš



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] Broken aarch64 by qcow2: skip writing zero buffers to empty COW areas [v2]

[Anton Nefedov]

On 21/8/2019 5:14 PM, Lukáš Doktor wrote:
> Hello guys,
> 
> First attempt was rejected due to zip attachment, let's try it again with 
> just Avocado-vt debug.log and serial console log files attached.
> 
> I bisected a regression on aarch64 all the way to this commit: "qcow2: skip 
> writing zero buffers to empty COW areas" 
> c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. Would you please have a look at it?
> 
> My reproducer is running kickstart installation of RHEL-8 from DVD on aarch64 
> gicv3 machine, which never finishes since this commit, where anaconda 
> complains about package installation, occasionally there are also XFS 
> metadata corruption messages on serial console:
> 

hi,

this looks scary :( I doubt that it can have anything to do with aarch64
but rather a really tricky timing (or, possibly, a broken environment
like broken fallocate() on a host? who knows..)

Is it always the same machine you observe this issue on? Did you try
others?

I just wonder if it's worth to try to reproduce it on my machine
(and I don't have aarch64 on hand now). I can probably come up with
some torture test that will continuously write to qcow2 with random
offsets/sizes and verify the result.

If you could kindly reproduce it again then we can probably start with
enabling qemu traces by appending
  " -trace bdrv* -trace qcow2* -trace file=/some_huge_partition/qemu.log"
to the command line.

Beware that it's going to produce a huge amount of logs.

Also, the corrupted image and the serial log will be required for
investigation.

thanks,

/Anton

Re: [Qemu-block] [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring

[Daniel P . Berrangé]

On Wed, Aug 14, 2019 at 11:22:07PM +0300, Maxim Levitsky wrote:
> * rename the write_func to create_write_func,
>   and init_func to create_init_func
>   this is  preparation for other write_func that will
>   be used to update the encryption keys.
> 
> No functional changes
> 
> Signed-off-by: Maxim Levitsky 
> ---
>  block/crypto.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 8237424ae6..42a3f0898b 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -51,7 +51,6 @@ static int block_crypto_probe_generic(QCryptoBlockFormat 
> format,
>  }
>  }
>  
> -

Unrelated whitespace change

>  static ssize_t block_crypto_read_func(QCryptoBlock *block,
>size_t offset,
>uint8_t *buf,
> @@ -77,7 +76,7 @@ struct BlockCryptoCreateData {
>  };
>  
>  
> -static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
> size_t offset,
> const uint8_t *buf,
> size_t buflen,

Re-indent.

> @@ -95,8 +94,7 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
>  return ret;
>  }
>  
> -

Unrelated whitespace

> -static ssize_t block_crypto_init_func(QCryptoBlock *block,
> +static ssize_t block_crypto_create_init_func(QCryptoBlock *block,
>size_t headerlen,
>void *opaque,
>Error **errp)

Re-indent.

> @@ -108,7 +106,8 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
>  return -EFBIG;
>  }
>  
> -/* User provided size should reflect amount of space made
> +/*
> + * User provided size should reflect amount of space made

Unrelated whitespace

>   * available to the guest, so we must take account of that
>   * which will be used by the crypto header
>   */
> @@ -117,6 +116,8 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
>  }
>  
>  
> +
> +

Unrelated whitespace

>  static QemuOptsList block_crypto_runtime_opts_luks = {
>  .name = "crypto",
>  .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head),
> @@ -272,8 +273,8 @@ static int 
> block_crypto_co_create_generic(BlockDriverState *bs,
>  };
>  
>  crypto = qcrypto_block_create(opts, NULL,
> -  block_crypto_init_func,
> -  block_crypto_write_func,
> +  block_crypto_create_init_func,
> +  block_crypto_create_write_func,
>&data,
>errp);

With the whitespace changes removed & indent fixed

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-block] [Qemu-devel] [PATCH] nbd: Tolerate more errors to structured reply request

[Eric Blake]

On 8/19/19 12:57 PM, Eric Blake wrote:
> A server may have a reason to reject a request for structured replies,
> beyond just not recognizing them as a valid request.  It doesn't hurt
> us to continue talking to such a server; otherwise 'qemu-nbd --list'
> of such a server fails to display all possible details about the
> export.
> 
> Encountered when temporarily tweaking nbdkit to reply with
> NBD_REP_ERR_POLICY.  Present since structured reply support was first
> added (commit d795299b reused starttls handling, but starttls has to
> reject all errors).
> 
> Signed-off-by: Eric Blake 
> ---
>  nbd/client.c | 39 +++
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 

> -/* If reply represents success, return 1 without further action.
> - * If reply represents an error, consume the optional payload of
> - * the packet on ioc.  Then return 0 for unsupported (so the client
> - * can fall back to other approaches), or -1 with errp set for other
> - * errors.
> +/*
> + * If reply represents success, return 1 without further action.  If
> + * reply represents an error, consume the optional payload of the
> + * packet on ioc.  Then return 0 for unsupported (so the client can
> + * fall back to other approaches), where @strict determines if only
> + * ERR_UNSUP or all errors fit that category, or -1 with errp set for
> + * other errors.
>   */
>  static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> -Error **errp)
> +bool strict, Error **errp)
>  {
>  char *msg = NULL;
> -int result = -1;
> +int result = strict ? -1 : 0;
> 
>  if (!(reply->type & (1 << 31))) {
>  return 1;
> @@ -162,6 +164,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>  error_setg(errp, "server error %" PRIu32
> " (%s) message is too long",
> reply->type, nbd_rep_lookup(reply->type));
> +result = -1;
>  goto cleanup;
>  }
>  msg = g_malloc(reply->length + 1);
> @@ -169,6 +172,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> NBDOptionReply *reply,
>  error_prepend(errp, "Failed to read option error %" PRIu32
>" (%s) message: ",
>reply->type, nbd_rep_lookup(reply->type));
> +result = -1;
>  goto cleanup;
>  }
>  msg[reply->length] = '\0';

Previously - nbd_handle_reply_err() left errp unchanged when returning
0, now if strict=false and return is 0, errp may be set.

Doesn't affect callers that pass strict=true, but...


> -static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
> +static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
> + Error **errp)
>  {
>  NBDOptionReply reply;
>  int error;
> @@ -562,7 +569,7 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
> opt, Error **errp)
>  if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
>  return -1;
>  }
> -error = nbd_handle_reply_err(ioc, &reply, errp);
> +error = nbd_handle_reply_err(ioc, &reply, strict, errp);
>  if (error <= 0) {
>  return error;
>  }

> @@ -950,7 +957,7 @@ static int nbd_start_negotiate(AioContext *aio_context, 
> QIOChannel *ioc,
>  if (structured_reply) {
>  result = nbd_request_simple_option(ioc,
> NBD_OPT_STRUCTURED_REPLY,
> -   errp);
> +   false, errp);
>  if (result < 0) {
>  return -EINVAL;
>  }

...this now can leave errp set, which can wreck callers.  I'll need to
post v2.

Also, I suspect that nbd_negotiate_simple_meta_context() should consider
the use of a non-strict error check (STARTTLS is really the only case
where if the server fails with an unexpected error, we really can't
continue on with some sane fallback regardless of the error).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 0/9] NBD reconnect

[Eric Blake]

On 8/21/19 6:41 AM, Vladimir Sementsov-Ogievskiy wrote:
> Should I resend with 07 dropped?
> 

At this point, the earlier patches in the series are now in-tree, and
the later patches need rebasing again...


>>> v7:
>>> almost all: rebased on merged nbd.c and nbd-client.c (including patch 
>>> subject)
>>> 01-04: add Eric's r-b
>>> 04: wording
>>> 05: new
>>> 06: rewrite to remove timer earlier
>>> 07: new
>>> 08:
>>>   - rebase on 05 and 07
>>>   - drop "All rights reserved"
>>>   - handle drain
>>>   - improve handling aio context attach
>>> 09: move 249 -> 257

257 snuck into the tree for a different test, so you'll get to move it
again.

But yes, dropping patch 7 (with controversial SI unit addition) in favor
of more discernable constants locally (such as NANOSECONDS_PER_SECOND)
as part of the rebase is a good idea.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/backup: install notifier during creation

[Vladimir Sementsov-Ogievskiy]

09.08.2019 23:13, John Snow wrote:
> Backup jobs may yield prior to installing their handler, because of the
> job_co_entry shim which guarantees that a job won't begin work until
> we are ready to start an entire transaction.
> 
> Unfortunately, this makes proving correctness about transactional
> points-in-time for backup hard to reason about. Make it explicitly clear
> by moving the handler registration to creation time, and changing the
> write notifier to a no-op until the job is started.
> 
> Reported-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: John Snow 
> ---
>   block/backup.c | 32 +++-
>   include/qemu/job.h |  5 +
>   job.c  |  2 +-
>   3 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 07d751aea4..4df5b95415 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -344,6 +344,13 @@ static int coroutine_fn backup_before_write_notify(
>   assert(QEMU_IS_ALIGNED(req->offset, BDRV_SECTOR_SIZE));
>   assert(QEMU_IS_ALIGNED(req->bytes, BDRV_SECTOR_SIZE));
>   
> +/* The handler is installed at creation time; the actual point-in-time
> + * starts at job_start(). Transactions guarantee those two points are
> + * the same point in time. */
> +if (!job_started(&job->common.job)) {
> +return 0;
> +}

Hmm, sorry if it is a stupid question, I'm not good in multiprocessing and in
Qemu iothreads..

job_started just reads job->co. If bs runs in iothread, and therefore 
write-notifier
is in iothread, when job_start is called from main thread.. Is it guaranteed 
that
write-notifier will see job->co variable change early enough to not miss guest 
write?
Should not job->co be volatile for example or something like this?

If not think about this patch looks good for me.

> +
>   return backup_do_cow(job, req->offset, req->bytes, NULL, true);
>   }
>   
> @@ -398,6 +405,12 @@ static void backup_clean(Job *job)
>   BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>   BlockDriverState *bs = blk_bs(s->common.blk);
>   
> +/* cancelled before job_start: remove write_notifier */
> +if (s->before_write.notify) {
> +notifier_with_return_remove(&s->before_write);
> +s->before_write.notify = NULL;
> +}
> +
>   if (s->copy_bitmap) {
>   bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
>   s->copy_bitmap = NULL;
> @@ -527,17 +540,8 @@ static void backup_init_copy_bitmap(BackupBlockJob *job)
>   static int coroutine_fn backup_run(Job *job, Error **errp)
>   {
>   BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> -BlockDriverState *bs = blk_bs(s->common.blk);
>   int ret = 0;
>   
> -QLIST_INIT(&s->inflight_reqs);
> -qemu_co_rwlock_init(&s->flush_rwlock);
> -
> -backup_init_copy_bitmap(s);
> -
> -s->before_write.notify = backup_before_write_notify;
> -bdrv_add_before_write_notifier(bs, &s->before_write);
> -
>   if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
>   int64_t offset = 0;
>   int64_t count;
> @@ -572,6 +576,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>   
>out:
>   notifier_with_return_remove(&s->before_write);
> +s->before_write.notify = NULL;
>   
>   /* wait until pending backup_do_cow() calls have completed */
>   qemu_co_rwlock_wrlock(&s->flush_rwlock);
> @@ -767,6 +772,15 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  &error_abort);
>   job->len = len;
>   
> +/* Finally, install a write notifier that takes effect after job_start() 
> */
> +backup_init_copy_bitmap(job);
> +
> +QLIST_INIT(&job->inflight_reqs);
> +qemu_co_rwlock_init(&job->flush_rwlock);
> +
> +job->before_write.notify = backup_before_write_notify;
> +bdrv_add_before_write_notifier(bs, &job->before_write);
> +
>   return &job->common;
>   
>error:
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 9e7cd1e4a0..733afb696b 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -394,6 +394,11 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job));
>*/
>   void job_start(Job *job);
>   
> +/**
> + * job_started returns true if the @job has started.
> + */
> +bool job_started(Job *job);
> +
>   /**
>* @job: The job to enter.
>*
> diff --git a/job.c b/job.c
> index 28dd48f8a5..745af659ff 100644
> --- a/job.c
> +++ b/job.c
> @@ -243,7 +243,7 @@ bool job_is_completed(Job *job)
>   return false;
>   }
>   
> -static bool job_started(Job *job)
> +bool job_started(Job *job)
>   {
>   return job->co;
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] QEMU bitmap backup usability FAQ

[Vladimir Sementsov-Ogievskiy]

[CC Nikolay]

21.08.2019 1:25, John Snow wrote:
> Hi, downstream here at Red Hat I've been fielding some questions about
> the usability and feature readiness of Bitmaps (and related features) in
> QEMU.
> 
> Here are some questions I answered internally that I am copying to the
> list for two reasons:
> 
> (1) To make sure my answers are actually correct, and
> (2) To share this pseudo-reference with the community at large.
> 
> This is long, and mostly for reference. There's a summary at the bottom
> with some todo items and observations about the usability of the feature
> as it exists in QEMU.
> 
> Before too long, I intend to send a more summarized "roadmap" mail which
> details all of the current and remaining work to be done in and around
> the bitmaps feature in QEMU.
> 
> 
> Questions:
> 
>> "What format(s) is/are required for this functionality?"
> 
>  From the QEMU API, any format can be used to create and author
> incremental backups. The only known format limitations are:
> 
> 1. Persistent bitmaps cannot be created on any format except qcow2,
> although there are hooks to add support to other formats at a later date
> if desired.
> 
> DANGER CAVEAT #1: Adding bitmaps to QEMU by default creates transient
> bitmaps instead of persistent ones.
> 
> Possible TODO: Allow users to 'upgrade' transient bitmaps to persistent
> ones in case they made a mistake.

I doubt, as in my opinion real users of Qemu are not people but libvirt, which
should never make such mistake.

> 
> 
> 2. When using push backups (blockdev-backup, drive-backup), you may use
> any format as a target format. >
> DANGER CAVEAT #2: without backing file and/or filesystem-less sparse
> support, these images will be unusable.

You mean incremental backups of course, as the whole document is about bitmaps.

> 
> EXAMPLE: Backing up to a raw file loses allocation information, so we
> can no longer distinguish between zeroes and unallocated regions. The
> cluster size is also lost. This file will not be usable without
> additional metadata recorded elsewhere.*
> 
> (* This is complicated, but it is in theory possible to do a push backup
> to e.g. an NBD target with custom server code that saves allocation
> information to a metadata file, which would allow you to reconstruct
> backups. For instance, recording in a .json file which extents were
> written out would allow you to -- with a custom binary -- write this
> information on top of a base file to reconstruct a backup.)
> 
> 
> 3. Any format can be used for either shared storage or live storage
> migrations. There are TWO distinct mechanisms for migrating bitmaps:
> 
> A) The bitmap is flushed to storage and re-opened on the destination.
> This is only supported for qcow2 and shared-storage migrations.

cons: flushing/reopening is done during migration downtime, so if you have
a lot of bitmap data (for example, 64k granulared bitmap for 16tb disk is
~30MB, and there may be several bitmaps) downtime will become long.

> 
> B) The bitmap is live-migrated to the destination. This is supported for
> any format and can be used for either shared storage or live storage
> migrations.
> 
> DANGER CAVEAT #3: The second bitmap migration technique there is an
> optional migration capability that must be enabled explicitly.
> Otherwise, some migration combinations may drop bitmaps.

Also, bad thing may happen if we try to migrate persistent bitmap to not qcow2.

Also, with enabled capability, flushing/reopening is automatically disabled.

> 
> Matrix:
> 
>> migrate = migrate_capability or (persistent and shared_storage)
> 
> Enumerated:
> 
> live storage + raw : transient + no-capability: Dropped
> live-storage + raw : transient + bm-capability: Migrated
> live-storage + qcow2 : transient + no-capability: Dropped
> live-storage + qcow2 : transient + bm-capability: Migrated
> live-storage + qcow2 : persistent + no-capability: Dropped (!)
> live-storage + qcow2 : persistent + bm-capability: Migrated
> 
> shared-storage + raw : transient - no-capability: Dropped
> shared-storage + raw : transient + bm-capability: Migrated
> shared-storage + qcow2 : transient + no-capability: Migrated

Dropped you mean

> shared-storage + qcow2 : transient + bm-capability: Migrated
> shared-storage + qcow2 : persistent + no-capability: Migrated
> shared-storage + qcow2 : persistent + bm-capability: Migrated
> 
> Enabling the bitmap migration capability will ALWAYS migrate the bitmap.
> If it's disabled, we will only migrate the bitmaps for shared storage
> migrations where the bitmap is persistent, which is a qcow2-only case.
> 
> There is no warning or error if you attempt to migrate in a manner that
> loses your bitmaps.
> 
> (I might be persuaded to add a case for when you are doing a live
> storage migration of qcow2 with persistent bitmaps, which is somewhat a
> conflicting case: you've asked for the bitmap to be persistent, but it
> seems likely that if this ever happens in practice, it's because you
> have 

Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Maxim Levitsky]

On Wed, 2019-08-21 at 13:31 +0200, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
> > > On 8/15/19 9:44 AM, Maxim Levitsky wrote:
> > > 
> > > > > > > Does the idea of a union type with a default value for the 
> > > > > > > discriminator
> > > > > > > help?  Maybe we have a discriminator which defaults to 'auto', 
> > > > > > > and add a
> > > > > > > union branch 'auto':'any'.  During creation, if the 
> > > > > > > "driver":"auto"
> > > > > > > branch is selected (usually implicitly by omitting "driver", but 
> > > > > > > also
> > > > > > > possible explicitly), the creation attempt is rejected as invalid
> > > > > > > regardless of the contents of the remaining 'any'.  But during 
> > > > > > > amend
> > > > > > > usage, if the 'auto' branch is selected, we then add in the proper
> > > > > > > "driver":"xyz" and reparse the QAPI object to determine if the 
> > > > > > > remaining
> > > > > > > fields in 'any' still meet the specification for the required 
> > > > > > > driver branch.
> > > > > > > 
> > > > > > > This idea may still require some tweaks to the QAPI generator, 
> > > > > > > but it's
> > > > > > > the best I can come up with for a way to parse an arbitrary JSON 
> > > > > > > object
> > > > > > > with unknown validation, then reparse it again after adding more
> > > > > > > information that would constrain the parse differently.
> > > > > > 
> > > > > > Feels like this would be a lot of code just to allow the client to 
> > > > > > omit
> > > > > > passing a value that it knows anyway. If this were a human 
> > > > > > interface, I
> > > > > > could understand the desire to make commands less verbose, but for 
> > > > > > QMP I
> > > > > > honestly don't see the point when it's not trivial.
> > > > > 
> > > > > Seconded.
> > > > 
> > > > 
> > > > But what about my suggestion of adding something like:
> > > > 
> > > > { 'union': 'BlockdevAmendOptions',
> > > > 
> > > >   'base': {
> > > >   'node-name': 'str' },
> > > > 
> > > >   'discriminator': { 'get_block_driver(node-name)' } ,
> > > 
> > > Not worth it. It makes the QAPI generator more complex (to invoke
> > > arbitrary code instead of a fixed name) just to avoid a little bit of
> > > complexity in the caller (which is assumed to be a computer, and thus
> > > shouldn't have a hard time providing a sane 'driver' unconditionally).
> > > An HMP wrapper around the QMP command can do whatever magic it needs to
> > > omit driver, but making driver mandatory for QMP is just fine.
> > 
> > All right! I kind of not agree with that, since I think even though QMP is 
> > a machine language,
> > it still should be consistent since humans still use it, even if this is 
> > humans that code some
> > tool that use it.
> > 
> > I won't argue with you though, let it be like that.
> 
> Software's fundamental limit is complexity.  We need to pick what we use
> it for.  Sometimes, that means saying no to things that would be nice to
> have.

I fully agree with that and that is usually the exact reason I argue about such 
things:
Sometimes avoiding complexity in one place, and/or breaking consistency can 
introduce complexity in other place (like libvirt).

In this particular case, I won't argue about this, but still it kind of feels 
like
it is a precedent of requiring the user to supply redundant data.

Of all issues all of you pointed out (thanks!!) this is probably the least 
important one that I should be arguing about, 
so let it be like you say.

Best regards,
Maxim Levitsky

Re: [Qemu-block] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

On 21/8/2019 2:21 PM, Max Reitz wrote:
> On 21.08.19 13:00, Anton Nefedov wrote:
>> On 12/8/2019 10:04 PM, Max Reitz wrote:
>>> On 16.05.19 16:33, Anton Nefedov wrote:
 A block driver can provide a callback to report driver-specific
 statistics.

 file-posix driver now reports discard statistics

 Signed-off-by: Anton Nefedov 
 Reviewed-by: Vladimir Sementsov-Ogievskiy 
 Acked-by: Markus Armbruster 
 ---
qapi/block-core.json  | 38 ++
include/block/block.h |  1 +
include/block/block_int.h |  1 +
block.c   |  9 +
block/file-posix.c| 38 +++---
block/qapi.c  |  5 +
6 files changed, 89 insertions(+), 3 deletions(-)
>>>
>>>
 diff --git a/qapi/block-core.json b/qapi/block-core.json
 index 55194f84ce..368e09ae37 100644
 --- a/qapi/block-core.json
 +++ b/qapi/block-core.json
 @@ -956,6 +956,41 @@
   '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
   '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }

 +##
 +# @BlockStatsSpecificFile:
 +#
 +# File driver statistics
 +#
 +# @discard-nb-ok: The number of successful discard operations performed by
 +# the driver.
 +#
 +# @discard-nb-failed: The number of failed discard operations performed by
 +# the driver.
 +#
 +# @discard-bytes-ok: The number of bytes discarded by the driver.
 +#
 +# Since: 4.1
 +##
 +{ 'struct': 'BlockStatsSpecificFile',
 +  'data': {
 +  'discard-nb-ok': 'uint64',
 +  'discard-nb-failed': 'uint64',
 +  'discard-bytes-ok': 'uint64' } }
 +
 +##
 +# @BlockStatsSpecific:
 +#
 +# Block driver specific statistics
 +#
 +# Since: 4.1
 +##
 +{ 'union': 'BlockStatsSpecific',
 +  'base': { 'driver': 'BlockdevDriver' },
 +  'discriminator': 'driver',
 +  'data': {
 +  'file': 'BlockStatsSpecificFile',
 +  'host_device': 'BlockStatsSpecificFile' } }
>>>
>>> I would like to use these chance to complain that I find this awkward.
>>> My problem is that I don’t know how any management application is
>>> supposed to reasonably consume this.  It feels weird to potentially have
>>> to recognize the result for every block driver.
>>>
>>> I would now like to note that I’m clearly not in a position to block
>>> this at this point, because I’ve had a year to do so, I didn’t, so it
>>> would be unfair to do it now.
>>>
>>> (Still, I feel like if I have a concern, I should raise it, even if it’s
>>> too late.)
>>>
>>> I know Markus has proposed this, but I don’t understand why.  He set
>>> ImageInfoSpecific as a precedence, but that has a different reasoning
>>> behind it.  The point for that is that it simply doesn’t work any other
>>> way, because it is clearly format-specific information that cannot be
>>> shared between drivers.  Anything that can be shared is put into
>>> ImageInfo (like the cluster size).
>>>
>>> We have the same constellation here, BlockStats contains common stuff,
>>> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
>>> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
>>> duplicates fields that already exist in BlockDeviceStats.
>>>
>>>
>>> (Furthermore, most of ImageInfoSpecific is actually not useful to
>>> management software, but only as an information for humans (and having
>>> such a structure for that is perfectly fine).  But these stats don’t
>>> really look like something for immediate human consumption.)
>>>
>>>
>>> So I wonder why you don’t just put this information into
>>> BlockDeviceStats.  From what I can tell looking at
>>> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
>>> currently completely 0 if @query-nodes is true.
>>>
>>> (Furthermore, I wonder whether it would make sense to re-add
>>> BlockAcctStats to each BDS and then let the generic block code do the
>>> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
>>> care about node-level information at the time, but maybe it’s time to
>>> reconsider.)
>>>
>>>
>>> Anyway, as I’ve said, I fully understand that complaining about a design
>>> decision is just unfair at this point, so this is not a veto.
>>>
>>
>> hi!
>>
>> Having both "unmap" and "discard" stats in the same list feels weird.
>> The intention is that unmap belongs to the device level, and discard is
>> from the driver level.
> 
> Sorry, what I meant wasn’t adding a separate “discard” group, but just
> filling in the existing “unmap” fields.  As far as I understand, if we
> had BlockAcctStats for each BDS, the file node’s unmap stats would be
> the same as its discard stats, wouldn’t it?
> 

So, you mean count it all on BDS level _instead_ of SCSI/IDE le

Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Markus Armbruster]

Maxim Levitsky  writes:

> On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
>> On 8/15/19 9:44 AM, Maxim Levitsky wrote:
>> 
>> > > > > Does the idea of a union type with a default value for the 
>> > > > > discriminator
>> > > > > help?  Maybe we have a discriminator which defaults to 'auto', and 
>> > > > > add a
>> > > > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
>> > > > > branch is selected (usually implicitly by omitting "driver", but also
>> > > > > possible explicitly), the creation attempt is rejected as invalid
>> > > > > regardless of the contents of the remaining 'any'.  But during amend
>> > > > > usage, if the 'auto' branch is selected, we then add in the proper
>> > > > > "driver":"xyz" and reparse the QAPI object to determine if the 
>> > > > > remaining
>> > > > > fields in 'any' still meet the specification for the required driver 
>> > > > > branch.
>> > > > > 
>> > > > > This idea may still require some tweaks to the QAPI generator, but 
>> > > > > it's
>> > > > > the best I can come up with for a way to parse an arbitrary JSON 
>> > > > > object
>> > > > > with unknown validation, then reparse it again after adding more
>> > > > > information that would constrain the parse differently.
>> > > > 
>> > > > Feels like this would be a lot of code just to allow the client to omit
>> > > > passing a value that it knows anyway. If this were a human interface, I
>> > > > could understand the desire to make commands less verbose, but for QMP 
>> > > > I
>> > > > honestly don't see the point when it's not trivial.
>> > > 
>> > > Seconded.
>> > 
>> > 
>> > But what about my suggestion of adding something like:
>> > 
>> > { 'union': 'BlockdevAmendOptions',
>> > 
>> >   'base': {
>> >   'node-name': 'str' },
>> > 
>> >   'discriminator': { 'get_block_driver(node-name)' } ,
>> 
>> Not worth it. It makes the QAPI generator more complex (to invoke
>> arbitrary code instead of a fixed name) just to avoid a little bit of
>> complexity in the caller (which is assumed to be a computer, and thus
>> shouldn't have a hard time providing a sane 'driver' unconditionally).
>> An HMP wrapper around the QMP command can do whatever magic it needs to
>> omit driver, but making driver mandatory for QMP is just fine.
>
> All right! I kind of not agree with that, since I think even though QMP is a 
> machine language,
> it still should be consistent since humans still use it, even if this is 
> humans that code some
> tool that use it.
>
> I won't argue with you though, let it be like that.

Software's fundamental limit is complexity.  We need to pick what we use
it for.  Sometimes, that means saying no to things that would be nice to
have.

Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)

[Markus Armbruster]

Maxim Levitsky  writes:

> This adds:
>
> * x-blockdev-update-encryption and x-blockdev-erase-encryption qmp commands
>   Both commands take the QCryptoKeyManageOptions
>   the x-blockdev-update-encryption is meant for non destructive addition
>   of key slots / whatever the encryption driver supports in the future
>
>   x-blockdev-erase-encryption is meant for destructive encryption key erase,
>   in some cases even without way to recover the data.
>
>
> * bdrv_setup_encryption callback in the block driver
>   This callback does both the above functions with 'action' parameter
>
> * QCryptoKeyManageOptions with set of options that drivers can use for 
> encryption managment
>   Currently it has all the options that LUKS needs, and later it can be 
> extended
>   (via union) to support more encryption drivers if needed
>
> * blk_setup_encryption / bdrv_setup_encryption - the usual block layer 
> wrappers.
>   Note that bdrv_setup_encryption takes BlockDriverState and not BdrvChild,
>   for the ease of use from the qmp code. It is not expected that this function
>   will be used by anything but qmp and qemu-img code
>
>
> Signed-off-by: Maxim Levitsky 
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..53ed411eed 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5327,3 +5327,39 @@
>'data' : { 'node-name': 'str',
>   'iothread': 'StrOrNull',
>   '*force': 'bool' } }
> +
> +
> +##
> +# @x-blockdev-update-encryption:
> +#
> +# Update the encryption keys for an encrypted block device
> +#
> +# @node-name:  Name of the blockdev to operate on
> +# @force: Disable safety checks (use with care)

What checks excactly are disabled?

> +# @options:   Driver specific options
> +#
> +
> +# Since: 4.2
> +##
> +{ 'command': 'x-blockdev-update-encryption',
> +  'data': { 'node-name' : 'str',
> +'*force' : 'bool',
> +'options': 'QCryptoEncryptionSetupOptions' } }
> +
> +##
> +# @x-blockdev-erase-encryption:
> +#
> +# Erase the encryption keys for an encrypted block device
> +#
> +# @node-name:  Name of the blockdev to operate on
> +# @force: Disable safety checks (use with care)

Likewise.

> +# @options:   Driver specific options
> +#
> +# Returns: @QCryptoKeyManageResult

Doc comment claims the command returns something, even though it
doesn't.  Please fix.  Sadly, the doc generator fails to flag that.

> +#
> +# Since: 4.2
> +##
> +{ 'command': 'x-blockdev-erase-encryption',
> +  'data': { 'node-name' : 'str',
> +'*force' : 'bool',
> +'options': 'QCryptoEncryptionSetupOptions' } }
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index b2a4cff683..69e8b086db 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -309,3 +309,29 @@
>'base': 'QCryptoBlockInfoBase',
>'discriminator': 'format',
>'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> +
> +
> +##
> +# @QCryptoEncryptionSetupOptions:
> +#
> +# Driver specific options for encryption key management.

Specific to which driver?

> +#
> +# @key-secret: the ID of a QCryptoSecret object providing the password
> +#  to add or to erase (optional for erase)
> +#
> +# @old-key-secret: the ID of a QCryptoSecret object providing the password
> +#  that can currently unlock the image
> +#
> +# @slot: Key slot to update/erase
> +#(optional, for update will select a free slot,
> +#for erase will erase all slots that match the password)
> +#
> +# @iter-time: number of milliseconds to spend in
> +# PBKDF passphrase processing. Currently defaults to 2000
> +# Since: 4.2
> +##
> +{ 'struct': 'QCryptoEncryptionSetupOptions',
> +  'data': { '*key-secret': 'str',
> +'*old-key-secret': 'str',
> +'*slot': 'int',
> +'*iter-time': 'int' } }

The two new commands have identical arguments.  Some of them you factor
out into their own struct.  Can you explain what makes them special?

The extra nesting on the wire is kind of ugly.  We can talk about how to
avoid it once I understand why we want the extra struct.

Re: [Qemu-block] [PATCH v7 0/9] NBD reconnect

[Vladimir Sementsov-Ogievskiy]

Should I resend with 07 dropped?

25.07.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
> ping
> 
> 18.06.2019 14:43, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>> Here is NBD reconnect. Previously, if connection failed all current
>> and future requests will fail. After the series, nbd-client driver
>> will try to reconnect unlimited times. During first @reconnect-delay
>> seconds of reconnecting all requests will wait for the connection,
>> and if it is established requests will be resent. After
>> @reconnect-delay period all requests will be failed (until successful
>> reconnect).
>>
>> v7:
>> almost all: rebased on merged nbd.c and nbd-client.c (including patch 
>> subject)
>> 01-04: add Eric's r-b
>> 04: wording
>> 05: new
>> 06: rewrite to remove timer earlier
>> 07: new
>> 08:
>>   - rebase on 05 and 07
>>   - drop "All rights reserved"
>>   - handle drain
>>   - improve handling aio context attach
>> 09: move 249 -> 257
>>
>> Vladimir Sementsov-Ogievskiy (9):
>>    block/nbd: split connection_co start out of nbd_client_connect
>>    block/nbd: use non-blocking io channel for nbd negotiation
>>    block/nbd: move from quit to state
>>    block/nbd: add cmdline and qapi parameter reconnect-delay
>>    block/nbd: refactor nbd connection parameters
>>    qemu-coroutine-sleep: introduce qemu_co_sleep_wake
>>    qemu/units: add SI decimal units
>>    block/nbd: nbd reconnect
>>    iotests: test nbd reconnect
>>
>>   qapi/block-core.json  |  11 +-
>>   include/block/nbd.h   |   3 +-
>>   include/qemu/coroutine.h  |  17 +-
>>   include/qemu/units.h  |   7 +
>>   block/nbd.c   | 531 +-
>>   block/null.c  |   2 +-
>>   block/sheepdog.c  |   2 +-
>>   nbd/client.c  |  16 +-
>>   qemu-nbd.c    |   2 +-
>>   tests/test-bdrv-drain.c   |   6 +-
>>   tests/test-block-iothread.c   |   2 +-
>>   util/qemu-coroutine-sleep.c   |  47 ++-
>>   tests/qemu-iotests/257    |  63 
>>   tests/qemu-iotests/257.out    |  10 +
>>   tests/qemu-iotests/group  |   1 +
>>   tests/qemu-iotests/iotests.py |   4 +
>>   16 files changed, 551 insertions(+), 173 deletions(-)
>>   create mode 100755 tests/qemu-iotests/257
>>   create mode 100644 tests/qemu-iotests/257.out
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations

[Anton Nefedov]

On 12/8/2019 9:16 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   hw/ide/core.c | 12 
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6afadf894f..3a7ac93777 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -441,6 +441,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);
> 
> Hmm, in other places (ide_handle_rw_error() here or
> scsi_handle_rw_error() in scsi-disk) only report this with
> BLOCK_ERROR_ACTION_REPORT.
> 
> So I’m wondering whether the same should be done here.
> 

Many other places do not check the action:
scsi_{dma|read|write}_complete(), hw/ide/atapi.c calls

That feels somewhat weird to me, to account the operation complete when
it's not.
(But I don't really know the use cases of BLOCK_ERROR_ACTION_IGNORE).

Can it be that the error action is only checked so that the operation is
not accounted twice (as there might be block_acct_done() later in the
common path with ACTION_IGNORE)

/Anton

Re: [Qemu-block] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Max Reitz]

On 21.08.19 13:00, Anton Nefedov wrote:
> On 12/8/2019 10:04 PM, Max Reitz wrote:
>> On 16.05.19 16:33, Anton Nefedov wrote:
>>> A block driver can provide a callback to report driver-specific
>>> statistics.
>>>
>>> file-posix driver now reports discard statistics
>>>
>>> Signed-off-by: Anton Nefedov 
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>> Acked-by: Markus Armbruster 
>>> ---
>>>   qapi/block-core.json  | 38 ++
>>>   include/block/block.h |  1 +
>>>   include/block/block_int.h |  1 +
>>>   block.c   |  9 +
>>>   block/file-posix.c| 38 +++---
>>>   block/qapi.c  |  5 +
>>>   6 files changed, 89 insertions(+), 3 deletions(-)
>>
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 55194f84ce..368e09ae37 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -956,6 +956,41 @@
>>>  '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>>  '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>>   
>>> +##
>>> +# @BlockStatsSpecificFile:
>>> +#
>>> +# File driver statistics
>>> +#
>>> +# @discard-nb-ok: The number of successful discard operations performed by
>>> +# the driver.
>>> +#
>>> +# @discard-nb-failed: The number of failed discard operations performed by
>>> +# the driver.
>>> +#
>>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>>> +#
>>> +# Since: 4.1
>>> +##
>>> +{ 'struct': 'BlockStatsSpecificFile',
>>> +  'data': {
>>> +  'discard-nb-ok': 'uint64',
>>> +  'discard-nb-failed': 'uint64',
>>> +  'discard-bytes-ok': 'uint64' } }
>>> +
>>> +##
>>> +# @BlockStatsSpecific:
>>> +#
>>> +# Block driver specific statistics
>>> +#
>>> +# Since: 4.1
>>> +##
>>> +{ 'union': 'BlockStatsSpecific',
>>> +  'base': { 'driver': 'BlockdevDriver' },
>>> +  'discriminator': 'driver',
>>> +  'data': {
>>> +  'file': 'BlockStatsSpecificFile',
>>> +  'host_device': 'BlockStatsSpecificFile' } }
>>
>> I would like to use these chance to complain that I find this awkward.
>> My problem is that I don’t know how any management application is
>> supposed to reasonably consume this.  It feels weird to potentially have
>> to recognize the result for every block driver.
>>
>> I would now like to note that I’m clearly not in a position to block
>> this at this point, because I’ve had a year to do so, I didn’t, so it
>> would be unfair to do it now.
>>
>> (Still, I feel like if I have a concern, I should raise it, even if it’s
>> too late.)
>>
>> I know Markus has proposed this, but I don’t understand why.  He set
>> ImageInfoSpecific as a precedence, but that has a different reasoning
>> behind it.  The point for that is that it simply doesn’t work any other
>> way, because it is clearly format-specific information that cannot be
>> shared between drivers.  Anything that can be shared is put into
>> ImageInfo (like the cluster size).
>>
>> We have the same constellation here, BlockStats contains common stuff,
>> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
>> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
>> duplicates fields that already exist in BlockDeviceStats.
>>
>>
>> (Furthermore, most of ImageInfoSpecific is actually not useful to
>> management software, but only as an information for humans (and having
>> such a structure for that is perfectly fine).  But these stats don’t
>> really look like something for immediate human consumption.)
>>
>>
>> So I wonder why you don’t just put this information into
>> BlockDeviceStats.  From what I can tell looking at
>> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
>> currently completely 0 if @query-nodes is true.
>>
>> (Furthermore, I wonder whether it would make sense to re-add
>> BlockAcctStats to each BDS and then let the generic block code do the
>> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
>> care about node-level information at the time, but maybe it’s time to
>> reconsider.)
>>
>>
>> Anyway, as I’ve said, I fully understand that complaining about a design
>> decision is just unfair at this point, so this is not a veto.
>>
> 
> hi!
> 
> Having both "unmap" and "discard" stats in the same list feels weird.
> The intention is that unmap belongs to the device level, and discard is
> from the driver level.

Sorry, what I meant wasn’t adding a separate “discard” group, but just
filling in the existing “unmap” fields.  As far as I understand, if we
had BlockAcctStats for each BDS, the file node’s unmap stats would be
the same as its discard stats, wouldn’t it?

> Now we have a separate structure named "driver-
> specific". Could also be called "driver-stats".
> 
> We could make this structure non-optional, present for all driver
> types, as indeed there is nothing special about discard stats. But 

[Qemu-block] Broken aarch64 by qcow2: skip writing zero buffers to empty COW areas

[Lukáš Doktor]

Hello guys,

I bisected a regression on aarch64 all the way to this commit: "qcow2: skip 
writing zero buffers to empty COW areas" 
c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a. Would you please have a look at it?

My reproducer is running kickstart installation of RHEL-8 from DVD on aarch64 
gicv3 machine, which never finishes since this commit, where anaconda complains 
about package installation, occasionally there are also XFS metadata corruption 
messages on serial console:


2019-08-18 19:36:11: Installing check.aarch64 (359/1180)[  
439.335155] XFS (dm-3): Metadata corruption detected at 
xfs_inode_buf_verify+0x174/0x190 [xfs], xfs_inode block 0xb1d960 
xfs_inode_buf_verify
2019-08-18 19:36:11: [  439.345295] XFS (dm-3): Unmount and run xfs_repair
2019-08-18 19:36:11: [  439.349167] XFS (dm-3): First 128 bytes of corrupted 
metadata buffer:
2019-08-18 19:36:11: 
2019-08-18 19:36:11: Installing libidn.aarch64 (360/1180)[ 
 439.354063] : 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  

2019-08-18 19:36:11: [  439.365513] 0010: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.371929] 0020: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.378175] 0030: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.384242] 0040: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.390603] 0050: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.396784] 0060: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.402838] 0070: 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00  
2019-08-18 19:36:11: [  439.409314] XFS (dm-3): metadata I/O error in 
"xfs_trans_read_buf_map" at daddr 0xb1d960 len 32 error 117
2019-08-18 19:36:11: [  439.416866] XFS (dm-3): xfs_imap_to_bp: 
xfs_trans_read_buf() returned error -117.
2019-08-18 19:36:12: [  440.341560] systemd-journald[1550]: Journal effective 
settings seal=yes compress=yes compress_threshold_bytes=512B
2019-08-18 19:36:12: 

2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
Error[
  440.371152] systemd-journald[1550]: Journal effective settings seal=yes 
compress=yes compress_threshold_bytes=512B
2019-08-18 19:36:12: [  440.381579] systemd-journald[1550]: Journal effective 
settings seal=yes compress=yes compress_threshold_bytes=512B
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12: 
2019-08-18 19:36:12:    The following error occurred while installing.  
This is a fatal error and
2019-08-18 19:36:12:installation will be aborted.
2019-08-18 19:36:12: 
2019-08-18 19:36:12:DNF error: Error unpacking rpm package 
libidn-1.34-5.el8.aarch64Press ENTER to exit: [  
440.401675] systemd-journald[1550]: Journal effective settings seal=yes 
compress=yes compress_threshold_bytes=512B

It reproduces 100%, usually at different stage (different RPM package). With 
Avocado-vt you can reproduce it by putting RHEL-8 DVD image to RHEL.7.devel iso 
location and executing something like this:

avocado --show all run --vt-guest-os RHEL.7.devel --vt-extra-params 
store_vm_register=no take_regular_screendumps=no monitor_type=human 
--vt-qemu-bin /home/jenkins/aarch64/qemu-master-build/bin/qemu-system-aarch64 
-- unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads

Note: I'm still using RHEL.7.devel profile although the provided DVD is RHEL8, 
it doesn't really matter
Note: The "--vt-extra-params" are workarounds of current issue where frequent 
use of QMP monitor results in bad things on aarch64

But it should reproduce even without Avocado-vt. The used qemu cmd was:

MALLOC_PERTURB_=1  
/home/jenkins/aarch64/qemu-master-build/bin/qemu-system-aarch64 \
-S  \
-name 'avocado-vt-vm1'  \
-sandbox off  \
-drive 
file=/usr/share/AAVMF/AAVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \
-drive 
file=/var/lib/libvirt/images/avocado/avocado-vt/images/rhel7devel-aarch64_AAVMF_VARS.fd,if=pflash,format=raw,unit=1
 \
-machine virt,gic-version=host  \
-nodefaults \
-device virtio-gpu-pci,bus=pcie.0,addr=0x1  \
-chardev 
socket,id=hmp_id_qmpmonitor1,path=/var/tmp/avocado_kurmsdn9/monitor-qmpmonitor1-20190821-065917-I0T8uXz4,server,nowait
 \
-mon chardev=hmp_id_qmpmonitor1,mode=readline  \
-chardev 
socket,id=hmp_id_catch_monitor,path=/var/tmp/avocado_kurmsdn9/mon

Re: [Qemu-block] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct

[Anton Nefedov]

On 12/8/2019 8:58 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> it allows to report it in the error handler
>>
>> Signed-off-by: Anton Nefedov 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>> Reviewed-by: Alberto Garcia 
>> ---
>>   hw/scsi/scsi-disk.c | 12 +---
>>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> (Sorry for the late reply :-/)
> 
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index e7e865ab3b..b43254103c 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData 
>> *data, int ret)
>>   {
>>   SCSIDiskReq *r = data->r;
>>   SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> -uint64_t sector_num;
>> -uint32_t nb_sectors;
>>   
>>   assert(r->req.aiocb == NULL);
>>   if (scsi_disk_req_check_error(r, ret, false)) {
>> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData 
>> *data, int ret)
>>   }
>>   
>>   if (data->count > 0) {
>> -sector_num = ldq_be_p(&data->inbuf[0]);
>> -nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xULL;
>> -if (!check_lba_range(s, sector_num, nb_sectors)) {
>> +r->sector = ldq_be_p(&data->inbuf[0]);
>> +r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xULL;
>> +if (!check_lba_range(s, r->sector, r->sector_count)) {
>>   scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>>   goto done;
>>   }
>>   
>>   r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>> -sector_num * s->qdev.blocksize,
>> -nb_sectors * s->qdev.blocksize,
>> +r->sector * s->qdev.blocksize,
>> +r->sector_count * s->qdev.blocksize,
> 
> This looks to me like these are not necessarily in terms of 512-byte
> sectors.  It doesn’t seem to make anything technically wrong, because
> patch 7 takes that into account.
> 
> But it’s still weird if everything else in this file treats these fields
> as being in terms of 512 byte sectors (and they are actually defined
> this way in SCSIDiskReq).
> 

Nice that you caught this, thanks! I guess variable names misled me

Re: [Qemu-block] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

On 12/8/2019 10:04 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> A block driver can provide a callback to report driver-specific
>> statistics.
>>
>> file-posix driver now reports discard statistics
>>
>> Signed-off-by: Anton Nefedov 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>> Acked-by: Markus Armbruster 
>> ---
>>   qapi/block-core.json  | 38 ++
>>   include/block/block.h |  1 +
>>   include/block/block_int.h |  1 +
>>   block.c   |  9 +
>>   block/file-posix.c| 38 +++---
>>   block/qapi.c  |  5 +
>>   6 files changed, 89 insertions(+), 3 deletions(-)
> 
> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 55194f84ce..368e09ae37 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -956,6 +956,41 @@
>>  '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>  '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>   
>> +##
>> +# @BlockStatsSpecificFile:
>> +#
>> +# File driver statistics
>> +#
>> +# @discard-nb-ok: The number of successful discard operations performed by
>> +# the driver.
>> +#
>> +# @discard-nb-failed: The number of failed discard operations performed by
>> +# the driver.
>> +#
>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'struct': 'BlockStatsSpecificFile',
>> +  'data': {
>> +  'discard-nb-ok': 'uint64',
>> +  'discard-nb-failed': 'uint64',
>> +  'discard-bytes-ok': 'uint64' } }
>> +
>> +##
>> +# @BlockStatsSpecific:
>> +#
>> +# Block driver specific statistics
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'union': 'BlockStatsSpecific',
>> +  'base': { 'driver': 'BlockdevDriver' },
>> +  'discriminator': 'driver',
>> +  'data': {
>> +  'file': 'BlockStatsSpecificFile',
>> +  'host_device': 'BlockStatsSpecificFile' } }
> 
> I would like to use these chance to complain that I find this awkward.
> My problem is that I don’t know how any management application is
> supposed to reasonably consume this.  It feels weird to potentially have
> to recognize the result for every block driver.
> 
> I would now like to note that I’m clearly not in a position to block
> this at this point, because I’ve had a year to do so, I didn’t, so it
> would be unfair to do it now.
> 
> (Still, I feel like if I have a concern, I should raise it, even if it’s
> too late.)
> 
> I know Markus has proposed this, but I don’t understand why.  He set
> ImageInfoSpecific as a precedence, but that has a different reasoning
> behind it.  The point for that is that it simply doesn’t work any other
> way, because it is clearly format-specific information that cannot be
> shared between drivers.  Anything that can be shared is put into
> ImageInfo (like the cluster size).
> 
> We have the same constellation here, BlockStats contains common stuff,
> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
> duplicates fields that already exist in BlockDeviceStats.
> 
> 
> (Furthermore, most of ImageInfoSpecific is actually not useful to
> management software, but only as an information for humans (and having
> such a structure for that is perfectly fine).  But these stats don’t
> really look like something for immediate human consumption.)
> 
> 
> So I wonder why you don’t just put this information into
> BlockDeviceStats.  From what I can tell looking at
> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
> currently completely 0 if @query-nodes is true.
> 
> (Furthermore, I wonder whether it would make sense to re-add
> BlockAcctStats to each BDS and then let the generic block code do the
> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
> care about node-level information at the time, but maybe it’s time to
> reconsider.)
> 
> 
> Anyway, as I’ve said, I fully understand that complaining about a design
> decision is just unfair at this point, so this is not a veto.
> 

hi!

Having both "unmap" and "discard" stats in the same list feels weird.
The intention is that unmap belongs to the device level, and discard is
from the driver level. Now we have a separate structure named "driver-
specific". Could also be called "driver-stats".

We could make this structure non-optional, present for all driver
types, as indeed there is nothing special about discard stats. But then
we need some way to distinguish
  - discard_nb_ok == 0 as no request reached the driver level
  - discard_nb_ok == 0 as the driver does not support the accounting

Yes, putting the accounting in the generic code would help, but do we
really want to burden it with accounting too? Tracking that each and
every case is covered with all those block_acct_done() invalid() and
failed() can really be a pain.

And wh

Re: [Qemu-block] [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

On 20.08.19 23:32, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> null-aio may not be whitelisted.  Skip all test cases that require it.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/093 | 12 +---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 50c1e7f2ec..f03fa24a07 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -24,7 +24,7 @@ import iotests
>>  nsec_per_sec = 10
>>  
>>  class ThrottleTestCase(iotests.QMPTestCase):
>> -test_img = "null-aio://"
>> +test_driver = "null-aio"
>>  max_drives = 3
>>  
>>  def blockstats(self, device):
>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>  return stat['rd_bytes'], stat['rd_operations'], 
>> stat['wr_bytes'], stat['wr_operations']
>>  raise Exception("Device not found for blockstats: %s" % device)
>>  
>> +def required_drivers(self):
>> +return [self.test_driver]
>> +
>> +@iotests.skip_if_unsupported(required_drivers)
> 
> Oh, I see why you're passing args[0] back to the callback now. Why not
> just pass self.required_drivers and call it with no arguments instead?
> 
> You can get a bound version that way that doesn't need additional
> arguments, and then the callback is free to take generic callables of
> any kind.

That would be nicer indeed.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method

[Max Reitz]

On 20.08.19 23:31, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> This lets tests use skip_if_unsupported() with a potentially variable
>> list of required formats.
>>
>> Suggested-by: Kevin Wolf 
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 726f904f50..8f315538e9 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[], 
>> read_only=False):
>> Runs the test if all the required formats are whitelisted'''
>>  def skip_test_decorator(func):
>>  def func_wrapper(*args, **kwargs):
>> -usf_list = list(set(required_formats) -
>> -set(supported_formats(read_only)))
>> +if callable(required_formats):
>> +fmts = required_formats(args[0])
>> +else:
>> +fmts = required_formats
>> +
>> +usf_list = list(set(fmts) - set(supported_formats(read_only)))
>>  if usf_list:
>>  args[0].case_skip('{}: formats {} are not 
>> whitelisted'.format(
>>  args[0], usf_list))
>>
> 
> I am required to inform you that this is in direct violation of the
> pythonista treaty of 2007; which mandates that you try to call and fail
> instead of attempting to gracefully check ahead of time.
> 
> Luckily, I am not fond of such rules.

:-)

I blame Kevin’s proposal.  (We should always have someone on PTO to
blame for everything.)


Thanks for reviewing.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()

[Max Reitz]

On 20.08.19 23:27, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> skip_if_unsupported() should use the stronger variant case_skip(),
>> because this allows it to be used even with setUp() (in a meaningful
>> way).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 2f53baf633..726f904f50 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[], 
>> read_only=False):
>>  usf_list = list(set(required_formats) -
>>  set(supported_formats(read_only)))
>>  if usf_list:
>> -case_notrun('{}: formats {} are not whitelisted'.format(
>> +args[0].case_skip('{}: formats {} are not 
>> whitelisted'.format(
>>  args[0], usf_list))
>>  else:
>>  return func(*args, **kwargs)
>>
> 
> Should we promote args[0] to a named argument here, because we depend on
> it having a specific type? It's not truly as polymorphic as we're making
> it appear.
> 
> That type here is iotests.QMPTestCase because we're relying on case_skip
> being present.
> 
> def test_wrapper(test_case, *args, **kwargs):
> ...
> return func(test_case, *args, **kwargs)

That sounds good to me indeed.

(I didn’t feel too bad about it because we already use args[0] for the
skip reason, but it really should be named, yes.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 2/7] hw/dma/omap_dma: Move switch 'fall through' comment to correct place

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
> 
> CC  hw/dma/omap_dma.o
>   hw/dma/omap_dma.c: In function ‘omap_dma_write’:
>   hw/dma/omap_dma.c:1532:12: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
>1532 | if (s->model <= omap_dma_3_1)
> |^
>   hw/dma/omap_dma.c:1534:5: note: here
>1534 | case 0x400:
> | ^~~~
>   cc1: all warnings being treated as errors
> 
> Correctly place the 'fall through' comment.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/dma/omap_dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
> index eab83c5c3a..6677237d42 100644
> --- a/hw/dma/omap_dma.c
> +++ b/hw/dma/omap_dma.c
> @@ -1531,8 +1531,8 @@ static void omap_dma_write(void *opaque, hwaddr addr,
>  case 0x404 ... 0x4fe:
>  if (s->model <= omap_dma_3_1)
>  break;
> +/* fall through */
>  case 0x400:
> -/* Fall through. */
>  if (omap_dma_sys_write(s, addr, value))
>  break;
>  return;
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 1/7] json: Move switch 'fall through' comment to correct place

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
> 
>   qobject/json-parser.c: In function ‘parse_literal’:
>   qobject/json-parser.c:492:24: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
> 492 | case JSON_INTEGER: {
> |^
>   qobject/json-parser.c:524:5: note: here
> 524 | case JSON_FLOAT:
> | ^~~~
> 
> Correctly place the 'fall through' comment.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  qobject/json-parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 7d23e12e33..d083810d37 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -519,8 +519,8 @@ static QObject *parse_literal(JSONParserContext *ctxt)
>  }
>  assert(ret == -ERANGE);
>  }
> -/* fall through to JSON_FLOAT */
>  }
> +/* fall through to JSON_FLOAT */
>  case JSON_FLOAT:
>  /* FIXME dependent on locale; a pervasive issue in QEMU */
>  /* FIXME our lexer matches RFC 8259 in forbidding Inf or NaN,
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 6/7] vl: Rewrite a fall through comment

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
>   vl.c: In function ‘qemu_ref_timedate’:
>   vl.c:773:15: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
> 773 | value -= rtc_realtime_clock_offset;
> | ~~^~~~
>   vl.c:775:5: note: here
> 775 | case QEMU_CLOCK_VIRTUAL:
> | ^~~~
>   cc1: all warnings being treated as errors
> 
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index a5808f9a02..f5cf71e3b4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -771,7 +771,7 @@ static time_t qemu_ref_timedate(QEMUClockType clock)
>  switch (clock) {
>  case QEMU_CLOCK_REALTIME:
>  value -= rtc_realtime_clock_offset;
> -/* no break */
> +/* fall through */
>  case QEMU_CLOCK_VIRTUAL:
>  value += rtc_ref_start_datetime;
>  break;
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 7/7] spapr_events: Rewrite a fall through comment

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
> CC  ppc64-softmmu/hw/ppc/spapr_rtc.o
>   hw/ppc/spapr_events.c: In function ‘rtas_event_log_to_source’:
>   hw/ppc/spapr_events.c:312:12: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
> 312 | if (spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT)) {
> |^
>   hw/ppc/spapr_events.c:317:5: note: here
> 317 | case RTAS_LOG_TYPE_EPOW:
> | ^~~~
>   cc1: all warnings being treated as errors
> 
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ppc/spapr_events.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index ae0f093f59..0a98894ad6 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -313,7 +313,7 @@ rtas_event_log_to_source(SpaprMachineState *spapr, int 
> log_type)
>  g_assert(source->enabled);
>  break;
>  }
> -/* fall back to epow for legacy hotplug interrupt source */
> +/* fall through back to epow for legacy hotplug interrupt source */
>  case RTAS_LOG_TYPE_EPOW:
>  source = spapr_event_sources_get_source(spapr->event_sources,
>  EVENT_CLASS_EPOW);
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 5/7] target/ppc: Rewrite a fall through comment

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
>   target/ppc/mmu_helper.c: In function ‘dump_mmu’:
>   target/ppc/mmu_helper.c:1349:12: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
>1349 | if (ppc64_v3_radix(env_archcpu(env))) {
> |^
>   target/ppc/mmu_helper.c:1356:5: note: here
>1356 | default:
> | ^~~
>   cc1: all warnings being treated as errors
> 
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/ppc/mmu_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 261a8fe707..862824b073 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -98,7 +98,7 @@ static int pp_check(int key, int pp, int nx)
>  case 0x1:
>  case 0x2:
>  access |= PAGE_WRITE;
> -/* No break here */
> +/* fall through */
>  case 0x3:
>  access |= PAGE_READ;
>  break;
> @@ -706,7 +706,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env, 
> mmu_ctx_t *ctx,
>  if (pr != 0) {
>  goto check_perms;
>  }
> -/* No break here */
> +/* fall through */
>  case 0x3:
>  /* All accesses granted */
>  ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> @@ -720,7 +720,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env, 
> mmu_ctx_t *ctx,
>  ret = -2;
>  break;
>  }
> -/* No break here */
> +/* fall through */
>  case 0x1:
>  check_perms:
>  /* Check from TLB entry */
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-block] [Qemu-trivial] [PATCH-for-4.1? 4/7] hw/ipmi: Rewrite a fall through comment

[Laurent Vivier]

Le 19/07/2019 à 15:14, Philippe Mathieu-Daudé a écrit :
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
>   hw/ipmi/ipmi_bmc_extern.c: In function ‘addchar’:
>   hw/ipmi/ipmi_bmc_extern.c:178:12: error: this statement may fall through 
> [-Werror=implicit-fallthrough=]
> 178 | ch |= 0x10;
> | ~~~^~~
>   hw/ipmi/ipmi_bmc_extern.c:181:5: note: here
> 181 | default:
> | ^~~
>   cc1: all warnings being treated as errors
>   make: *** [rules.mak:69: hw/ipmi/ipmi_bmc_extern.o] Error 1
> 
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
> 
> Reported-by: Stefan Weil 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ipmi/ipmi_bmc_extern.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index c0a8dac346..d4cbd210c4 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -176,8 +176,7 @@ static void addchar(IPMIBmcExtern *ibe, unsigned char ch)
>  ibe->outbuf[ibe->outlen] = VM_ESCAPE_CHAR;
>  ibe->outlen++;
>  ch |= 0x10;
> -/* No break */
> -
> +/* fall through */
>  default:
>  ibe->outbuf[ibe->outlen] = ch;
>  ibe->outlen++;
> 

Applied to my trivial-patches branch.

Thanks,
Laurent