Re: [PATCH v2] docs: Mark "gluster" support in QEMU as deprecated

[Daniel P . Berrangé]

On Wed, Sep 25, 2024 at 09:15:14AM +0200, Thomas Huth wrote:
> According to https://marc.info/?l=fedora-devel-list&m=171934833215726
> the GlusterFS development effectively ended. Thus mark it as deprecated
> in QEMU, so we can remove it in a future release if the project does
> not gain momentum again.
> 
> Acked-by: Niels de Vos 
> Signed-off-by: Thomas Huth 
> ---
>  v2: Mark it as deprecated in the QAPI and print a warning once, too
> 
>  docs/about/deprecated.rst | 9 +
>  qapi/block-core.json  | 7 ++-
>  block/gluster.c   | 2 ++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ed31d4b0b2..b231aa3948 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -395,6 +395,15 @@ Specifying the iSCSI password in plain text on the 
> command line using the
>  used instead, to refer to a ``--object secret...`` instance that provides
>  a password via a file, or encrypted.
>  
> +``gluster`` backend (since 9.2)
> +^^^
> +
> +According to https://marc.info/?l=fedora-devel-list&m=171934833215726
> +the GlusterFS development effectively ended. Unless the development
> +gains momentum again, the QEMU project might remove the gluster backend
> +in a future release.

I'd suggest the second half of the sentance can be simplified:

   ", the QEMU project will remove the gluster backend/"

since marking something as deprecated is a stronger than "might".
We /will/ remove it, unless new informaton comes to light that
makes us re-evaluate the plans.


With 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: [PATCH v2 00/22] -Werror=maybe-uninitialized fixes

[Daniel P . Berrangé]

On Tue, Sep 24, 2024 at 05:05:31PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Hi,
> 
> Depending on -Doptimization=, GCC (14.2.1 here) produces different
> maybe-uninitialized warnings:
> - g: produces -Werror=maybe-uninitialized errors
> - 0: clean build
> - 1: produces -Werror=maybe-uninitialized errors
> - 2: clean build
> - 3: produces few -Werror=maybe-uninitialized errors
> - s: produces -Werror=maybe-uninitialized errors
> 
> Most are false-positive, because prior LOCK_GUARD should guarantee an
> initialization path. Few of them are a bit trickier. Finally, I found
> a potential related memory leak.

In addition we now build with "-ftrivial-auto-var-init=zero", so
any case which is missing an "= NULL" or "= 0"  initialization is
protected.

With 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: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib

[Daniel P . Berrangé]

On Mon, Sep 23, 2024 at 11:03:08AM -0500, Eric Blake wrote:
> On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
> > On Thu, Mar 28, 2024 at 02:13:42PM +, Richard W.M. Jones wrote:
> > > On Thu, Mar 28, 2024 at 03:06:03PM +0100, Thomas Huth wrote:
> > > > Since version 2.66, glib has useful URI parsing functions, too.
> > > > Use those instead of the QEMU-internal ones to be finally able
> > > > to get rid of the latter. The g_uri_get_host() also takes care
> > > > of removing the square brackets from IPv6 addresses, so we can
> > > > drop that part of the QEMU code now, too.
> > > > 
> 
> > > >  
> > > > -p = uri->path ? uri->path : "";
> > > > +p = g_uri_get_path(uri) ?: "";
> > > >  if (p[0] == '/') {
> > > >  p++;
> > > >  }
> 
> > > Looks ok,
> > >
> > > Reviewed-by: Richard W.M. Jones 
> > 
> > Or maybe not.  This caused a regression in the nbdkit test suite (when
> > we use qemu-img from 9.1).  It seems the exportname part of the NBD
> > URI gets munged:
> > 
> > https://gitlab.com/qemu-project/qemu/-/issues/2584
> 
> To be more specific, it looks like
> g_uri_get_path("./name//with//..//slashes") is getting munged to
> "name/slashes".  That is, glib is blindly assuming that ./ and XXX/../
> can be dropped, and // can be simplified to /, which may be true for
> arbitrary file names but not true for abitrary URIs (since URIs have
> application-specific semantics, which may not match path name
> traversal semantics).  Looks like we need to report a bug to glib,
> and/or see if glib's URI functions have a flag for turning off this
> unwanted munging.

The source code indicates it is doing some normalization
based on this:

  https://datatracker.ietf.org/doc/html/rfc3986#section-5.2.4

With 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 :|

[PULL 10/10] crypto: Introduce x509 utils

[Daniel P . Berrangé]

From: Dorjoy Chowdhury 

An utility function for getting fingerprint from X.509 certificate
has been introduced. Implementation only provided using gnutls.

Signed-off-by: Dorjoy Chowdhury 
[DB: fixed missing gnutls_x509_crt_deinit in success path]
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/meson.build  |  4 ++
 crypto/x509-utils.c | 76 +
 include/crypto/x509-utils.h | 22 +++
 3 files changed, 102 insertions(+)
 create mode 100644 crypto/x509-utils.c
 create mode 100644 include/crypto/x509-utils.h

diff --git a/crypto/meson.build b/crypto/meson.build
index c46f9c22a7..735635de1f 100644
--- a/crypto/meson.build
+++ b/crypto/meson.build
@@ -24,6 +24,10 @@ crypto_ss.add(files(
   'rsakey.c',
 ))
 
+if gnutls.found()
+  crypto_ss.add(files('x509-utils.c'))
+endif
+
 if nettle.found()
   crypto_ss.add(nettle, files('hash-nettle.c', 'hmac-nettle.c', 
'pbkdf-nettle.c'))
   if hogweed.found()
diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
new file mode 100644
index 00..6e157af76b
--- /dev/null
+++ b/crypto/x509-utils.c
@@ -0,0 +1,76 @@
+/*
+ * X.509 certificate related helpers
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "crypto/x509-utils.h"
+#include 
+#include 
+#include 
+
+static const int qcrypto_to_gnutls_hash_alg_map[QCRYPTO_HASH_ALG__MAX] = {
+[QCRYPTO_HASH_ALG_MD5] = GNUTLS_DIG_MD5,
+[QCRYPTO_HASH_ALG_SHA1] = GNUTLS_DIG_SHA1,
+[QCRYPTO_HASH_ALG_SHA224] = GNUTLS_DIG_SHA224,
+[QCRYPTO_HASH_ALG_SHA256] = GNUTLS_DIG_SHA256,
+[QCRYPTO_HASH_ALG_SHA384] = GNUTLS_DIG_SHA384,
+[QCRYPTO_HASH_ALG_SHA512] = GNUTLS_DIG_SHA512,
+[QCRYPTO_HASH_ALG_RIPEMD160] = GNUTLS_DIG_RMD160,
+};
+
+int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
+  QCryptoHashAlgorithm alg,
+  uint8_t *result,
+  size_t *resultlen,
+  Error **errp)
+{
+int ret = -1;
+int hlen;
+gnutls_x509_crt_t crt;
+gnutls_datum_t datum = {.data = cert, .size = size};
+
+if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
+error_setg(errp, "Unknown hash algorithm");
+return -1;
+}
+
+if (result == NULL) {
+error_setg(errp, "No valid buffer given");
+return -1;
+}
+
+gnutls_x509_crt_init(&crt);
+
+if (gnutls_x509_crt_import(crt, &datum, GNUTLS_X509_FMT_PEM) != 0) {
+error_setg(errp, "Failed to import certificate");
+goto cleanup;
+}
+
+hlen = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
+if (*resultlen < hlen) {
+error_setg(errp,
+   "Result buffer size %zu is smaller than hash %d",
+   *resultlen, hlen);
+goto cleanup;
+}
+
+if (gnutls_x509_crt_get_fingerprint(crt,
+qcrypto_to_gnutls_hash_alg_map[alg],
+result, resultlen) != 0) {
+error_setg(errp, "Failed to get fingerprint from certificate");
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+gnutls_x509_crt_deinit(crt);
+return ret;
+}
diff --git a/include/crypto/x509-utils.h b/include/crypto/x509-utils.h
new file mode 100644
index 00..4210dfbcfc
--- /dev/null
+++ b/include/crypto/x509-utils.h
@@ -0,0 +1,22 @@
+/*
+ * X.509 certificate related helpers
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#ifndef QCRYPTO_X509_UTILS_H
+#define QCRYPTO_X509_UTILS_H
+
+#include "crypto/hash.h"
+
+int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
+  QCryptoHashAlgorithm hash,
+  uint8_t *result,
+  size_t *resultlen,
+  Error **errp);
+
+#endif
-- 
2.45.2

[PULL 08/10] crypto: Define macros for hash algorithm digest lengths

[Daniel P . Berrangé]

From: Dorjoy Chowdhury 

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Dorjoy Chowdhury 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/hash.c | 14 +++---
 include/crypto/hash.h |  8 
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/crypto/hash.c b/crypto/hash.c
index b0f8228bdc..8087f5dae6 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -23,13 +23,13 @@
 #include "hashpriv.h"
 
 static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALG__MAX] = {
-[QCRYPTO_HASH_ALG_MD5] = 16,
-[QCRYPTO_HASH_ALG_SHA1] = 20,
-[QCRYPTO_HASH_ALG_SHA224] = 28,
-[QCRYPTO_HASH_ALG_SHA256] = 32,
-[QCRYPTO_HASH_ALG_SHA384] = 48,
-[QCRYPTO_HASH_ALG_SHA512] = 64,
-[QCRYPTO_HASH_ALG_RIPEMD160] = 20,
+[QCRYPTO_HASH_ALG_MD5]   = QCRYPTO_HASH_DIGEST_LEN_MD5,
+[QCRYPTO_HASH_ALG_SHA1]  = QCRYPTO_HASH_DIGEST_LEN_SHA1,
+[QCRYPTO_HASH_ALG_SHA224]= QCRYPTO_HASH_DIGEST_LEN_SHA224,
+[QCRYPTO_HASH_ALG_SHA256]= QCRYPTO_HASH_DIGEST_LEN_SHA256,
+[QCRYPTO_HASH_ALG_SHA384]= QCRYPTO_HASH_DIGEST_LEN_SHA384,
+[QCRYPTO_HASH_ALG_SHA512]= QCRYPTO_HASH_DIGEST_LEN_SHA512,
+[QCRYPTO_HASH_ALG_RIPEMD160] = QCRYPTO_HASH_DIGEST_LEN_RIPEMD160,
 };
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgorithm alg)
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 54d87aa2a1..a113cc3b04 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -23,6 +23,14 @@
 
 #include "qapi/qapi-types-crypto.h"
 
+#define QCRYPTO_HASH_DIGEST_LEN_MD5   16
+#define QCRYPTO_HASH_DIGEST_LEN_SHA1  20
+#define QCRYPTO_HASH_DIGEST_LEN_SHA22428
+#define QCRYPTO_HASH_DIGEST_LEN_SHA25632
+#define QCRYPTO_HASH_DIGEST_LEN_SHA38448
+#define QCRYPTO_HASH_DIGEST_LEN_SHA51264
+#define QCRYPTO_HASH_DIGEST_LEN_RIPEMD160 20
+
 /* See also "QCryptoHashAlgorithm" defined in qapi/crypto.json */
 
 /**
-- 
2.45.2

[PULL 09/10] crypto: Support SHA384 hash when using glib

[Daniel P . Berrangé]

From: Dorjoy Chowdhury 

QEMU requires minimum glib version 2.66.0 as per the root meson.build
file and per glib documentation[1] G_CHECKSUM_SHA384 is available since
2.51.

[1] https://docs.gtk.org/glib/enum.ChecksumType.html

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Dorjoy Chowdhury 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/hash-glib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 82de9db705..18e64faa9c 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -29,7 +29,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALG__MAX] = {
 [QCRYPTO_HASH_ALG_SHA1] = G_CHECKSUM_SHA1,
 [QCRYPTO_HASH_ALG_SHA224] = -1,
 [QCRYPTO_HASH_ALG_SHA256] = G_CHECKSUM_SHA256,
-[QCRYPTO_HASH_ALG_SHA384] = -1,
+[QCRYPTO_HASH_ALG_SHA384] = G_CHECKSUM_SHA384,
 [QCRYPTO_HASH_ALG_SHA512] = G_CHECKSUM_SHA512,
 [QCRYPTO_HASH_ALG_RIPEMD160] = -1,
 };
-- 
2.45.2

[PULL 04/10] tests/unit: always build the pbkdf crypto unit test

[Daniel P . Berrangé]

The meson rules were excluding the pbkdf crypto test when gnutls was the
crypto backend. It was then excluded again in #if statements in the test
file.

Rather than update these conditions, remove them all, and use the result
of the qcrypto_pbkdf_supports() function to determine whether to skip
test registration.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/unit/meson.build |  4 +---
 tests/unit/test-crypto-pbkdf.c | 13 -
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 490ab8182d..972d792883 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -121,9 +121,7 @@ if have_block
   if config_host_data.get('CONFIG_REPLICATION')
 tests += {'test-replication': [testblock]}
   endif
-  if nettle.found() or gcrypt.found()
-tests += {'test-crypto-pbkdf': [io]}
-  endif
+  tests += {'test-crypto-pbkdf': [io]}
 endif
 
 if have_system
diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
index 43c417f6b4..241e1c2cf0 100644
--- a/tests/unit/test-crypto-pbkdf.c
+++ b/tests/unit/test-crypto-pbkdf.c
@@ -25,8 +25,7 @@
 #include 
 #endif
 
-#if ((defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) && \
- (defined(_WIN32) || defined(RUSAGE_THREAD)))
+#if defined(_WIN32) || defined(RUSAGE_THREAD)
 #include "crypto/pbkdf.h"
 
 typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
@@ -394,7 +393,7 @@ static void test_pbkdf(const void *opaque)
 }
 
 
-static void test_pbkdf_timing(void)
+static void test_pbkdf_timing_sha256(void)
 {
 uint8_t key[32];
 uint8_t salt[32];
@@ -422,14 +421,18 @@ int main(int argc, char **argv)
 g_assert(qcrypto_init(NULL) == 0);
 
 for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
+if (!qcrypto_pbkdf2_supports(test_data[i].hash)) {
+continue;
+}
+
 if (!test_data[i].slow ||
 g_test_slow()) {
 g_test_add_data_func(test_data[i].path, &test_data[i], test_pbkdf);
 }
 }
 
-if (g_test_slow()) {
-g_test_add_func("/crypt0/pbkdf/timing", test_pbkdf_timing);
+if (g_test_slow() && qcrypto_pbkdf2_supports(QCRYPTO_HASH_ALG_SHA256)) {
+g_test_add_func("/crypt0/pbkdf/timing/sha256", 
test_pbkdf_timing_sha256);
 }
 
 return g_test_run();
-- 
2.45.2

[PULL 05/10] tests/unit: build pbkdf test on macOS

[Daniel P . Berrangé]

Add CONFIG_DARWIN to the pbkdf test build condition, since we have a way
to measure CPU time on this platform since commit bf98afc75efedf1.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/unit/test-crypto-pbkdf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
index 241e1c2cf0..39264cb662 100644
--- a/tests/unit/test-crypto-pbkdf.c
+++ b/tests/unit/test-crypto-pbkdf.c
@@ -25,7 +25,7 @@
 #include 
 #endif
 
-#if defined(_WIN32) || defined(RUSAGE_THREAD)
+#if defined(_WIN32) || defined(RUSAGE_THREAD) || defined(CONFIG_DARWNI)
 #include "crypto/pbkdf.h"
 
 typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
-- 
2.45.2

[PULL 06/10] crypto: avoid leak of ctx when bad cipher mode is given

[Daniel P . Berrangé]

Fixes: Coverity CID 1546884
Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/cipher-nettle.c.inc | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/crypto/cipher-nettle.c.inc b/crypto/cipher-nettle.c.inc
index 42b39e18a2..766de036ba 100644
--- a/crypto/cipher-nettle.c.inc
+++ b/crypto/cipher-nettle.c.inc
@@ -734,16 +734,19 @@ static QCryptoCipher 
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
 #ifdef CONFIG_CRYPTO_SM4
 case QCRYPTO_CIPHER_ALG_SM4:
 {
-QCryptoNettleSm4 *ctx = g_new0(QCryptoNettleSm4, 1);
+QCryptoNettleSm4 *ctx;
+const QCryptoCipherDriver *drv;
 
 switch (mode) {
 case QCRYPTO_CIPHER_MODE_ECB:
-ctx->base.driver = &qcrypto_nettle_sm4_driver_ecb;
+drv = &qcrypto_nettle_sm4_driver_ecb;
 break;
 default:
 goto bad_cipher_mode;
 }
 
+ctx = g_new0(QCryptoNettleSm4, 1);
+ctx->base.driver = drv;
 sm4_set_encrypt_key(&ctx->key[0], key);
 sm4_set_decrypt_key(&ctx->key[1], key);
 
-- 
2.45.2

[PULL 07/10] crypto: use consistent error reporting pattern for unsupported cipher modes

[Daniel P . Berrangé]

Not all paths in qcrypto_cipher_ctx_new() were correctly distinguishing
between valid user input for cipher mode (which should report a user
facing error), vs program logic errors (which should assert).

Reported-by: Peter Maydell 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/cipher-nettle.c.inc | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/crypto/cipher-nettle.c.inc b/crypto/cipher-nettle.c.inc
index 766de036ba..2654b439c1 100644
--- a/crypto/cipher-nettle.c.inc
+++ b/crypto/cipher-nettle.c.inc
@@ -525,8 +525,10 @@ static QCryptoCipher 
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
 case QCRYPTO_CIPHER_MODE_CTR:
 drv = &qcrypto_nettle_des_driver_ctr;
 break;
-default:
+case QCRYPTO_CIPHER_MODE_XTS:
 goto bad_cipher_mode;
+default:
+g_assert_not_reached();
 }
 
 ctx = g_new0(QCryptoNettleDES, 1);
@@ -551,8 +553,10 @@ static QCryptoCipher 
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
 case QCRYPTO_CIPHER_MODE_CTR:
 drv = &qcrypto_nettle_des3_driver_ctr;
 break;
-default:
+case QCRYPTO_CIPHER_MODE_XTS:
 goto bad_cipher_mode;
+default:
+g_assert_not_reached();
 }
 
 ctx = g_new0(QCryptoNettleDES3, 1);
@@ -663,8 +667,10 @@ static QCryptoCipher 
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
 case QCRYPTO_CIPHER_MODE_CTR:
 drv = &qcrypto_nettle_cast128_driver_ctr;
 break;
-default:
+case QCRYPTO_CIPHER_MODE_XTS:
 goto bad_cipher_mode;
+default:
+g_assert_not_reached();
 }
 
 ctx = g_new0(QCryptoNettleCAST128, 1);
@@ -741,8 +747,12 @@ static QCryptoCipher 
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
 case QCRYPTO_CIPHER_MODE_ECB:
 drv = &qcrypto_nettle_sm4_driver_ecb;
 break;
-default:
+case QCRYPTO_CIPHER_MODE_CBC:
+case QCRYPTO_CIPHER_MODE_CTR:
+case QCRYPTO_CIPHER_MODE_XTS:
 goto bad_cipher_mode;
+default:
+g_assert_not_reached();
 }
 
 ctx = g_new0(QCryptoNettleSm4, 1);
-- 
2.45.2

[PULL 03/10] crypto: check gnutls & gcrypt support the requested pbkdf hash

[Daniel P . Berrangé]

Both gnutls and gcrypt can be configured to exclude support for certain
algorithms via a runtime check against system crypto policies. Thus it
is not sufficient to have a compile time test for hash support in their
pbkdf implementations.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/pbkdf-gcrypt.c | 2 +-
 crypto/pbkdf-gnutls.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/pbkdf-gcrypt.c b/crypto/pbkdf-gcrypt.c
index a8d8e64f4d..bc0719c831 100644
--- a/crypto/pbkdf-gcrypt.c
+++ b/crypto/pbkdf-gcrypt.c
@@ -33,7 +33,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
 case QCRYPTO_HASH_ALG_SHA384:
 case QCRYPTO_HASH_ALG_SHA512:
 case QCRYPTO_HASH_ALG_RIPEMD160:
-return true;
+return qcrypto_hash_supports(hash);
 default:
 return false;
 }
diff --git a/crypto/pbkdf-gnutls.c b/crypto/pbkdf-gnutls.c
index 2dfbbd382c..911b565bea 100644
--- a/crypto/pbkdf-gnutls.c
+++ b/crypto/pbkdf-gnutls.c
@@ -33,7 +33,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
 case QCRYPTO_HASH_ALG_SHA384:
 case QCRYPTO_HASH_ALG_SHA512:
 case QCRYPTO_HASH_ALG_RIPEMD160:
-return true;
+return qcrypto_hash_supports(hash);
 default:
 return false;
 }
-- 
2.45.2

[PULL 01/10] iotests: fix expected output from gnutls

[Daniel P . Berrangé]

Error reporting from gnutls was improved by:

  commit 57941c9c86357a6a642f9ee3279d881df4043b6d
  Author: Daniel P. Berrangé 
  Date:   Fri Mar 15 14:07:58 2024 +

crypto: push error reporting into TLS session I/O APIs

This has the effect of changing the output from one of the NBD
tests.

Reported-by: Thomas Huth 
Signed-off-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/233.out | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 1910f7df20..d498d55e0e 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -69,8 +69,8 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == check TLS with authorization ==
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: Software caused 
connection abort
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: Software caused 
connection abort
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: The TLS connection 
was non-properly terminated.
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: The TLS connection 
was non-properly terminated.
 
 == check TLS fail over UNIX with no hostname ==
 qemu-img: Could not open 
'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': No hostname for 
certificate validation
@@ -103,14 +103,14 @@ qemu-img: Could not open 
'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0'
 qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.
 
 == final server log ==
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
 qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
 qemu-nbd: option negotiation failed: TLS handshake failed: An illegal 
parameter has been received.
 qemu-nbd: option negotiation failed: TLS handshake failed: An illegal 
parameter has been received.
 *** done
-- 
2.45.2

[PULL 02/10] crypto: run qcrypto_pbkdf2_count_iters in a new thread

[Daniel P . Berrangé]

From: Tiago Pasqualini 

CPU time accounting in the kernel has been demonstrated to have a
sawtooth pattern[1][2]. This can cause the getrusage system call to
not be as accurate as we are expecting, which can cause this calculation
to stall.

The kernel discussions shows that this inaccuracy happens when CPU time
gets big enough, so this patch changes qcrypto_pbkdf2_count_iters to run
in a fresh thread to avoid this inaccuracy. It also adds a sanity check
to fail the process if CPU time is not accounted.

[1] 
https://lore.kernel.org/lkml/159231011694.16989.16351419333851309713.tip-bot2@tip-bot2/
[2] 
https://lore.kernel.org/lkml/20221226031010.4079885-1-maxing@bytedance.com/t/#m1c7f2fdc0ea742776a70fd1aa2a2e414c437f534

Resolves: #2398
Signed-off-by: Tiago Pasqualini 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/pbkdf.c | 53 +++---
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 8d198c152c..d1c06ef3ed 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/thread.h"
 #include "qapi/error.h"
 #include "crypto/pbkdf.h"
 #ifndef _WIN32
@@ -85,12 +86,28 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long 
*val_ms,
 #endif
 }
 
-uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
-const uint8_t *key, size_t nkey,
-const uint8_t *salt, size_t nsalt,
-size_t nout,
-Error **errp)
+typedef struct CountItersData {
+QCryptoHashAlgorithm hash;
+const uint8_t *key;
+size_t nkey;
+const uint8_t *salt;
+size_t nsalt;
+size_t nout;
+uint64_t iterations;
+Error **errp;
+} CountItersData;
+
+static void *threaded_qcrypto_pbkdf2_count_iters(void *data)
 {
+CountItersData *iters_data = (CountItersData *) data;
+QCryptoHashAlgorithm hash = iters_data->hash;
+const uint8_t *key = iters_data->key;
+size_t nkey = iters_data->nkey;
+const uint8_t *salt = iters_data->salt;
+size_t nsalt = iters_data->nsalt;
+size_t nout = iters_data->nout;
+Error **errp = iters_data->errp;
+
 uint64_t ret = -1;
 g_autofree uint8_t *out = g_new(uint8_t, nout);
 uint64_t iterations = (1 << 15);
@@ -114,7 +131,10 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm 
hash,
 
 delta_ms = end_ms - start_ms;
 
-if (delta_ms > 500) {
+if (delta_ms == 0) { /* sanity check */
+error_setg(errp, "Unable to get accurate CPU usage");
+goto cleanup;
+} else if (delta_ms > 500) {
 break;
 } else if (delta_ms < 100) {
 iterations = iterations * 10;
@@ -129,5 +149,24 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm 
hash,
 
  cleanup:
 memset(out, 0, nout);
-return ret;
+iters_data->iterations = ret;
+return NULL;
+}
+
+uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
+const uint8_t *key, size_t nkey,
+const uint8_t *salt, size_t nsalt,
+size_t nout,
+Error **errp)
+{
+CountItersData data = {
+hash, key, nkey, salt, nsalt, nout, 0, errp
+};
+QemuThread thread;
+
+qemu_thread_create(&thread, "pbkdf2", threaded_qcrypto_pbkdf2_count_iters,
+   &data, QEMU_THREAD_JOINABLE);
+qemu_thread_join(&thread);
+
+return data.iterations;
 }
-- 
2.45.2

[PULL 00/10] Crypto fixes patches

[Daniel P . Berrangé]

The following changes since commit f2aee60305a1e40374b2fc1093e4d04404e780ee:

  Merge tag 'pull-request-2024-09-08' of https://gitlab.com/huth/qemu into 
staging (2024-09-09 10:47:24 +0100)

are available in the Git repository at:

  https://gitlab.com/berrange/qemu tags/crypto-fixes-pull-request

for you to fetch changes up to 10a1d34fc0d4dfe0dd6f5ec73f62dc1afa04af6c:

  crypto: Introduce x509 utils (2024-09-09 15:13:38 +0100)


Various crypto fixes

 * Support sha384 with glib crypto backend
 * Improve error reporting for unsupported cipher modes
 * Avoid memory leak when bad cipher mode is given
 * Run pbkdf tests on macOS
 * Runtime check for pbkdf hash impls with gnutls & gcrypt
 * Avoid hangs counter pbkdf iterations on some Linux kernels
   by using a throwaway thread for benchmarking performance
 * Fix iotests expected output from gnutls errors

--------

Daniel P. Berrangé (6):
  iotests: fix expected output from gnutls
  crypto: check gnutls & gcrypt support the requested pbkdf hash
  tests/unit: always build the pbkdf crypto unit test
  tests/unit: build pbkdf test on macOS
  crypto: avoid leak of ctx when bad cipher mode is given
  crypto: use consistent error reporting pattern for unsupported cipher
modes

Dorjoy Chowdhury (3):
  crypto: Define macros for hash algorithm digest lengths
  crypto: Support SHA384 hash when using glib
  crypto: Introduce x509 utils

Tiago Pasqualini (1):
  crypto: run qcrypto_pbkdf2_count_iters in a new thread

 crypto/cipher-nettle.c.inc | 25 ---
 crypto/hash-glib.c |  2 +-
 crypto/hash.c  | 14 +++
 crypto/meson.build |  4 ++
 crypto/pbkdf-gcrypt.c  |  2 +-
 crypto/pbkdf-gnutls.c  |  2 +-
 crypto/pbkdf.c | 53 
 crypto/x509-utils.c| 76 ++
 include/crypto/hash.h  |  8 
 include/crypto/x509-utils.h| 22 ++
 tests/qemu-iotests/233.out | 12 +++---
 tests/unit/meson.build |  4 +-
 tests/unit/test-crypto-pbkdf.c | 13 +++---
 13 files changed, 200 insertions(+), 37 deletions(-)
 create mode 100644 crypto/x509-utils.c
 create mode 100644 include/crypto/x509-utils.h

-- 
2.45.2

Re: [PATCH v2 01/19] qapi: Smarter camel_to_upper() to reduce need for 'prefix'

[Daniel P . Berrangé]

On Thu, Sep 05, 2024 at 07:59:13AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Wed, Sep 04, 2024 at 01:18:18PM +0200, Markus Armbruster wrote:
> >> camel_to_upper() converts its argument from camel case to upper case
> >> with '_' between words.  Used for generated enumeration constant
> >> prefixes.
> >
> >
> >> 
> >> Signed-off-by: Markus Armbruster 
> >> Reviewed-by: Daniel P. Berrang?? 
> >
> > The accent in my name is getting mangled in this series.
> 
> Uh-oh!
> 
> Checking...  Hmm.  It's correct in git, correct in output of
> git-format-patch, correct in the copy I got from git-send-email --bcc
> armbru via localhost MTA, and the copy I got from --to
> qemu-de...@nongnu.org, correct in lore.kernel.org[*], correct in an mbox
> downloaded from patchew.
> 
> Could the culprit be on your side?

I compared my received mail vs the mbox archive on nongnu.org for
qemu-devel.

In both cases the actual mail body seems to be valid UTF-8 and is
identical. The message in the nongnu.org archive, however, has

  Content-Type: text/plain; charset=UTF-8

while the copy I got in my inbox has merely

  Content-Type: text/plain

What I can't determine is whether your original sent message
had "charset=UTF-8" which then got stripped by redhat's incoming
mail server, or whether your original lacked 'charset=UTF8' and
it got added by mailman when saving the message to the mbox archives ?


With 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: [PATCH v2 01/19] qapi: Smarter camel_to_upper() to reduce need for 'prefix'

[Daniel P . Berrangé]

On Wed, Sep 04, 2024 at 01:18:18PM +0200, Markus Armbruster wrote:
> camel_to_upper() converts its argument from camel case to upper case
> with '_' between words.  Used for generated enumeration constant
> prefixes.


> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Daniel P. Berrang?? 

The accent in my name is getting mangled in this series.

IIRC your mail client (git send-email ?) needs to be explicitly
setting a chardset eg

  Content-type: text/plain; charset=utf8

so that mail clients & intermediate servers know how to interpret
the 8bit data.

With 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: [PATCH] raw-format: Fix error message for invalid offset/size

[Daniel P . Berrangé]

On Thu, Aug 29, 2024 at 08:55:27PM +0200, Kevin Wolf wrote:
> s->offset and s->size are only set at the end of the function and still
> contain the old values when formatting the error message. Print the
> parameters with the new values that we actually checked instead.
> 
> Fixes: 500e2434207d ('raw-format: Split raw_read_options()')
> Signed-off-by: Kevin Wolf 
> ---
>  block/raw-format.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 

With 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: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Daniel P . Berrangé]

On Wed, Aug 28, 2024 at 10:32:15AM +0200, Thomas Huth wrote:
> On 27/08/2024 09.05, Markus Armbruster wrote:
> > Daniel P. Berrangé  writes:
> > 
> > > On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
> > > > On 24/07/2024 11.47, Daniel P. Berrangé wrote:
> > > > > The current TLS session I/O APIs just return a synthetic errno
> > > > > value on error, which has been translated from a gnutls error
> > > > > value. This looses a large amount of valuable information that
> > > > > distinguishes different scenarios.
> > > > > 
> > > > > Pushing population of the "Error *errp" object into the TLS
> > > > > session I/O APIs gives more detailed error information.
> > > > > 
> > > > > Reviewed-by: Philippe Mathieu-Daudé 
> > > > > Signed-off-by: Daniel P. Berrangé 
> > > > > ---
> > > > 
> > > >   Hi Daniel!
> > > > 
> > > > iotest 233 is failing for me with -raw now, and bisection
> > > > points to this commit. Output is:
> > > > 
> > > > --- .../qemu/tests/qemu-iotests/233.out
> > > > +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
> > > > @@ -69,8 +69,8 @@
> > > >   1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > > 
> > > >   == check TLS with authorization ==
> > > > -qemu-img: Could not open 
> > > > 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read 
> > > > option reply: Cannot read from TLS channel: Software caused connection 
> > > > abort
> > > > -qemu-img: Could not open 
> > > > 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read 
> > > > option reply: Cannot read from TLS channel: Software caused connection 
> > > > abort
> > > > +qemu-img: Could not open 
> > > > 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read 
> > > > option reply: Cannot read from TLS channel: The TLS connection was 
> > > > non-properly terminated.
> > > > +qemu-img: Could not open 
> > > > 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read 
> > > > option reply: Cannot read from TLS channel: The TLS connection was 
> > > > non-properly terminated.
> > > 
> > > This is an expected change. Previously squashed the real GNUTLS error
> > > into ECONNABORTED:
> > > 
> > > -case GNUTLS_E_PREMATURE_TERMINATION:
> > > -errno = ECONNABORTED;
> > > -break;
> > > 
> > > 
> > > now we report the original gnutls root cause.
> > > 
> > > IOW, we need to update the expected output files.
> > 
> > Has this been done?
> 
> No, I think the problem still persists.

I've just cc'd you both on a patch that fixes this.

With 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 :|

[PATCH] iotests: fix expected output from gnutls in NBD test

[Daniel P . Berrangé]

Error reporting from gnutls was improved by:

  commit 57941c9c86357a6a642f9ee3279d881df4043b6d
  Author: Daniel P. Berrangé 
  Date:   Fri Mar 15 14:07:58 2024 +

crypto: push error reporting into TLS session I/O APIs

This has the effect of changing the output from one of the NBD
tests.

Reported-by: Thomas Huth 
Signed-off-by: Daniel P. Berrangé 
---

NB, wrt qemu-stable this will be for the 9.1 stable branch once
that is created, no earlier releases will need this.

 tests/qemu-iotests/233.out | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 1910f7df20..d498d55e0e 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -69,8 +69,8 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == check TLS with authorization ==
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: Software caused 
connection abort
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: Software caused 
connection abort
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: The TLS connection 
was non-properly terminated.
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': 
Failed to read option reply: Cannot read from TLS channel: The TLS connection 
was non-properly terminated.
 
 == check TLS fail over UNIX with no hostname ==
 qemu-img: Could not open 
'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': No hostname for 
certificate validation
@@ -103,14 +103,14 @@ qemu-img: Could not open 
'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0'
 qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.
 
 == final server log ==
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
 qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read 
from TLS channel: The TLS connection was non-properly terminated.
 qemu-nbd: option negotiation failed: TLS handshake failed: An illegal 
parameter has been received.
 qemu-nbd: option negotiation failed: TLS handshake failed: An illegal 
parameter has been received.
 *** done
-- 
2.45.2

Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Daniel P . Berrangé]

On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
> On 24/07/2024 11.47, Daniel P. Berrangé wrote:
> > The current TLS session I/O APIs just return a synthetic errno
> > value on error, which has been translated from a gnutls error
> > value. This looses a large amount of valuable information that
> > distinguishes different scenarios.
> > 
> > Pushing population of the "Error *errp" object into the TLS
> > session I/O APIs gives more detailed error information.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> 
>  Hi Daniel!
> 
> iotest 233 is failing for me with -raw now, and bisection
> points to this commit. Output is:
> 
> --- .../qemu/tests/qemu-iotests/233.out
> +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
> @@ -69,8 +69,8 @@
>  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> 
>  == check TLS with authorization ==
> -qemu-img: Could not open 
> 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option 
> reply: Cannot read from TLS channel: Software caused connection abort
> -qemu-img: Could not open 
> 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option 
> reply: Cannot read from TLS channel: Software caused connection abort
> +qemu-img: Could not open 
> 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option 
> reply: Cannot read from TLS channel: The TLS connection was non-properly 
> terminated.
> +qemu-img: Could not open 
> 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option 
> reply: Cannot read from TLS channel: The TLS connection was non-properly 
> terminated.

This is an expected change. Previously squashed the real GNUTLS error
into ECONNABORTED:

-case GNUTLS_E_PREMATURE_TERMINATION:
-errno = ECONNABORTED;
-break;


now we report the original gnutls root cause.

IOW, we need to update the expected output files.


With 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: [PATCH 1/2] util: Refactor json-writer's string sanitizer to be public

[Daniel P . Berrangé]

On Fri, Aug 02, 2024 at 02:26:05PM -0500, Eric Blake wrote:
> My next patch needs to convert text from an untrusted input into an
> output representation that is suitable for display on a terminal is
> useful to more than just the json-writer; the text should normally be
> UTF-8, but blindly allowing all Unicode code points (including ASCII
> ESC) through to a terminal risks remote-code-execution attacks on some
> terminals.  Extract the existing body of json-writer's quoted_strinto
> a new helper routine mod_utf8_sanitize, and generalize it to also work
> on data that is length-limited rather than NUL-terminated.  [I was
> actually surprised that glib does not have such a sanitizer already -
> Google turns up lots of examples of rolling your own string
> sanitizer.]
> 
> If desired in the future, we may want to tweak whether the output is
> guaranteed to be ASCII (using lots of \u escape sequences, including
> surrogate pairs for code points outside the BMP) or if we are okay
> passing printable Unicode through (we still need to escape control
> characters).  But for now, I went for minimal code churn, including
> the fact that the resulting function allows a non-UTF-8 2-byte synonym
> for U+.
> 
> Signed-off-by: Eric Blake 
> ---
>  include/qemu/unicode.h |  3 ++
>  qobject/json-writer.c  | 47 +--
>  util/unicode.c | 84 ++
>  3 files changed, 88 insertions(+), 46 deletions(-)

I was going to ask for a unit test, but "escaped_string" in
test-qjson.c  looks like it will be covering this sufficiently
well already, that we don't need to test it in isolation.

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 2/2] qemu-img: CVE-XXX Sanitize untrusted output from NBD server

[Daniel P . Berrangé]

On Fri, Aug 02, 2024 at 10:03:05PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 02, 2024 at 02:26:06PM -0500, Eric Blake wrote:
> > Error messages from an NBD server must be treated as untrusted; a
> > malicious server can inject escape sequences to try and trigger RCE
> > flaws via escape sequences to whatever terminal happens to be running
> > qemu-img.  The easiest solution is to sanitize the output with the
> > same code we use to produce sanitized (pseudo-)JSON over QMP.
> > 
> > Rich Jones originally pointed this flaw out at:
> > https://lists.libguestfs.org/archives/list/gues...@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/
> > 
> > With this patch, and a malicious server run with nbdkit 1.40 as:
> > 
> > $ nbdkit --log=null eval open=' printf \
> >   "EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; 
> > \
> >   exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"'
> > 
> > we now get:
> > 
> > qemu-img: Could not open 'nbd://localhost': Requested export not available
> > server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output 
> > \u001B[31mmess up the output\u001B[m mess up
> > 
> > instead of an attempt to hide the name of the Unix socket and forcing
> > the terminal to render part of the text red.
> > 
> > Note that I did _not_ sanitize the string being sent through
> > trace-events in trace_nbd_server_error_msg; this is because I assume
> > that our trace engines already treat all string strings as untrusted
> > input and apply their own escaping as needed.
> > 
> > Reported-by: "Richard W.M. Jones" 
> > Signed-off-by: Eric Blake 
> > 
> > ---
> > 
> > If my assumption about allowing raw escape bytes through to trace_
> > calls is wrong (such as when tracing to stderr), let me know.  That's
> > a much bigger audit to determine which trace points, if any, should
> > sanitize data before tracing, and/or change the trace engines to
> > sanitize all strings (with possible knock-on effects if trace output
> > changes unexpectedly for a tool expecting something unsanitized).

For nearly all trace backends, QEMU is not emitting anything onthe
console, so there's no escaping QEMU needs to do.

The exception is the "log" backend which calls qemu_log(). IOW, if
that's a concern then the qemu_log() function needs to sanitize the
resulting buffer after printf, rather than the tracing code do anything.

The simpletrace.py script could probably need similar.

I lean towards "don't bother" though, as when tracing I think it is
important to see the raw un-modified output for the sake of accuracy.


> >  nbd/client.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/nbd/client.c b/nbd/client.c
> > index c89c7504673..baa20d10d69 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -23,6 +23,7 @@
> >  #include "trace.h"
> >  #include "nbd-internal.h"
> >  #include "qemu/cutils.h"
> > +#include "qemu/unicode.h"
> > 
> >  /* Definitions for opaque data types */
> > 
> > @@ -230,7 +231,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> > NBDOptionReply *reply,
> >  }
> > 
> >  if (msg) {
> > -error_append_hint(errp, "server reported: %s\n", msg);
> > +g_autoptr(GString) buf = g_string_sized_new(reply->length);
> > +mod_utf8_sanitize(buf, msg);
> > +error_append_hint(errp, "server reported: %s\n", buf->str);
> >  }

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 5/7] nbd/server: CVE-2024-7409: Close stray client sockets at shutdown

[Daniel P . Berrangé]

On Wed, Aug 07, 2024 at 12:43:31PM -0500, Eric Blake wrote:
> A malicious client can attempt to connect to an NBD server, and then
> intentionally delay progress in the handshake, including if it does
> not know the TLS secrets.  Although this behavior can be bounded by
> the max-connections parameter, the QMP nbd-server-start currently
> defaults to unlimited incoming client connections.  Worse, if the
> client waits to close the socket until after the QMP nbd-server-stop
> command is executed, qemu will then SEGV when trying to dereference
> the NULL nbd_server global which is no longer present, which amounts
> to a denial of service attack.  If another NBD server is started
> before the malicious client disconnects, I cannot rule out additional
> adverse effects when the old client interferes with the connection
> count of the new server.
> 
> For environments without this patch, the CVE can be mitigated by
> ensuring (such as via a firewall) that only trusted clients can
> connect to an NBD server.  Note that using frameworks like libvirt
> that ensure that TLS is used and that nbd-server-stop is not executed
> while any trusted clients are still connected will only help if there
> is also no possibility for an untrusted client to open a connection
> but then stall on the NBD handshake.
> 
> Given the previous patches, it would be possible to guarantee that no
> clients remain connected by having nbd-server-stop sleep for longer
> than the default handshake deadline before finally freeing the global
> nbd_server object, but that could make QMP non-responsive for a long
> time.  So intead, this patch fixes the problem by tracking all client
> sockets opened while the server is running, and forcefully closing any
> such sockets remaining without a completed handshake at the time of
> nbd-server-stop, then waiting until the coroutines servicing those
> sockets notice the state change.  nbd-server-stop now has a second
> AIO_WAIT_WHILE_UNLOCKED (the first is indirectly through the
> blk_exp_close_all_type() that disconnects all clients that completed
> handshakes), but forced socket shutdown is enough to progress the
> coroutines and quickly tear down all clients before the server is
> freed, thus finally fixing the CVE.
> 
> This patch relies heavily on the fact that nbd/server.c guarantees
> that it only calls nbd_blockdev_client_closed() from the main loop
> (see the assertion in nbd_client_put() and the hoops used in
> nbd_client_put_nonzero() to achieve that); if we did not have that
> guarantee, we would also need a mutex protecting our accesses of the
> list of connections to survive re-entrancy from independent iothreads.
> 
> Although I did not actually try to test old builds, it looks like this
> problem has existed since at least commit 862172f45c (v2.12.0, 2017) -
> even back when that patch started using a QIONetListener to handle
> listening on multiple sockets, nbd_server_free() was already unaware
> that the nbd_blockdev_client_closed callback can be reached later by a
> client thread that has not completed handshakes (and therefore the
> client's socket never got added to the list closed in
> nbd_export_close_all), despite that patch intentionally tearing down
> the QIONetListener to prevent new clients.
> 
> Reported-by: Alexander Ivanov 
> Fixes: CVE-2024-7409
> Signed-off-by: Eric Blake 
> ---
>  blockdev-nbd.c | 35 ++-
>  1 file changed, 34 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 4/7] nbd/server: CVE-2024-7409: Drop non-negotiating clients

[Daniel P . Berrangé]

On Wed, Aug 07, 2024 at 12:43:30PM -0500, Eric Blake wrote:
> A client that opens a socket but does not negotiate is merely hogging
> qemu's resources (an open fd and a small amount of memory); and a
> malicious client that can access the port where NBD is listening can
> attempt a denial of service attack by intentionally opening and
> abandoning lots of unfinished connections.  The previous patch put a
> default bound on the number of such ongoing connections, but once that
> limit is hit, no more clients can connect (including legitimate ones).
> The solution is to insist that clients complete handshake within a
> reasonable time limit, defaulting to 10 seconds.  A client that has
> not successfully completed NBD_OPT_GO by then (including the case of
> where the client didn't know TLS credentials to even reach the point
> of NBD_OPT_GO) is wasting our time and does not deserve to stay
> connected.  Later patches will allow fine-tuning the limit away from
> the default value (including disabling it for doing integration
> testing of the handshake process itself).
> 
> Note that this patch in isolation actually makes it more likely to see
> qemu SEGV after nbd-server-stop, as any client socket still connected
> when the server shuts down will now be closed after 10 seconds rather
> than at the client's whims.  That will be addressed in the next patch.
> 
> For a demo of this patch in action:
> $ qemu-nbd -f raw -r -t -e 10 file &
> $ nbdsh --opt-mode -c '
> H = list()
> for i in range(20):
>   print(i)
>   H.insert(i, nbd.NBD())
>   H[i].set_opt_mode(True)
>   H[i].connect_uri("nbd://localhost")
> '
> 
> where later connections get to start progressing once earlier ones are
> forcefully dropped for taking too long, rather than hanging.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Eric Blake 
> ---
>  nbd/server.c | 31 ++-
>  nbd/trace-events |  1 +
>  2 files changed, 31 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 3/7] nbd/server: CVE-2024-7409: Change default max-connections to 100

[Daniel P . Berrangé]

On Wed, Aug 07, 2024 at 12:43:29PM -0500, Eric Blake wrote:
> Allowing an unlimited number of clients to any web service is a recipe
> for a rudimentary denial of service attack: the client merely needs to
> open lots of sockets without closing them, until qemu no longer has
> any more fds available to allocate.
> 
> For qemu-nbd, we default to allowing only 1 connection unless more are
> explicitly asked for (-e or --shared); this was historically picked as
> a nice default (without an explicit -t, a non-persistent qemu-nbd goes
> away after a client disconnects, without needing any additional
> follow-up commands), and we are not going to change that interface now
> (besides, someday we want to point people towards qemu-storage-daemon
> instead of qemu-nbd).
> 
> But for qemu proper, the QMP nbd-server-start command has historically
> had a default of unlimited number of connections, in part because
> unlike qemu-nbd it is inherently persistent.  Allowing multiple client
> sockets is particularly useful for clients that can take advantage of
> MULTI_CONN (creating parallel sockets to increase throughput),
> although known clients that do so (such as libnbd's nbdcopy) typically
> use only 8 or 16 connections (the benefits of scaling diminish once
> more sockets are competing for kernel attention).  Picking a number
> large enough for typical use cases, but not unlimited, makes it
> slightly harder for a malicious client to perform a denial of service
> merely by opening lots of connections withot progressing through the
> handshake.
> 
> This change does not eliminate CVE-2024-7409 on its own, but reduces
> the chance for fd exhaustion or unlimited memory usage as an attack
> surface.  On the other hand, by itself, it makes it more obvious that
> with a finite limit, we have the problem of an unauthenticated client
> holding 100 fds opened as a way to block out a legitimate client from
> being able to connect; thus, later patches will further add timeouts
> to reject clients that are not making progress.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Eric Blake 
> ---
>  qapi/block-export.json | 4 ++--
>  include/block/nbd.h| 7 +++
>  block/monitor/block-hmp-cmds.c | 3 ++-
>  blockdev-nbd.c | 8 
>  4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 665d5fd0262..ce33fe378df 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -28,7 +28,7 @@
>  # @max-connections: The maximum number of connections to allow at the
>  # same time, 0 for unlimited.  Setting this to 1 also stops the
>  # server from advertising multiple client support (since 5.2;
> -# default: 0)
> +# default: 100)
>  #
>  # Since: 4.2
>  ##
> @@ -63,7 +63,7 @@
>  # @max-connections: The maximum number of connections to allow at the
>  # same time, 0 for unlimited.  Setting this to 1 also stops the
>  # server from advertising multiple client support (since 5.2;
> -# default: 0).
> +# default: 100).
>  #
>  # Errors:
>  # - if the server is already running

This is considered a backwards compatibility break.
An intentional one.

Our justification is that we believe it is the least worst option
to mitigate the DoS vector. Lets explore the reasoning for that
belief...

An alternative would be to deprecate the absence of 'max-connections',
and after the deprecation period, make it in into a mandatory
parameter, forcing apps to make a decision. This would be strictly
compliant with our QAPI change policy.

How does that differ in impact from changing the defaults...

  * Changed default
 - Small risk of breaking app if needing > 100 concurrent conns
 - Immediately closes the DoS hole for all apps using QEMU

  * Deprecation & eventual change to mandatory
 - Zero risk of breaking apps today
 - Forces all apps to be changed to pass max-connections
   in 3 releases time
 - Does NOT close the DoS hole until the 3rd future release
   from now.

If we took the deprecation route, arguably any app which isn't
already setting max-connections would need to have a CVE filed
against it *today*, and thus effectively apps would be forced
into immediately setting max-connections, despite our deprecaiton
grace period supposedly giving them 3 releases to adjust.

If we took the changed default route and broke an app, it would
again have to be changed to set max-connections to a value that
satisfies its use case.


So

If we take the deprecation route, we create work for /all/ app
developers using NBD to update their code now to fix the CVE.

If we take the changed defaults route, and our 100 value choice
is sufficient, then n

Re: [PATCH v4 2/7] nbd/server: Plumb in new args to nbd_client_add()

[Daniel P . Berrangé]

On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> Upcoming patches to fix a CVE need to track an opaque pointer passed
> in by the owner of a client object, as well as reequest for a time
> limit on how fast negotiation must complete.  Prepare for that by
> changing the signature of nbd_client_new() and adding an accessor to
> get at the opaque pointer, although for now the two servers
> (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Eric Blake 
> ---
>  include/block/nbd.h | 11 ++-
>  blockdev-nbd.c  |  6 --
>  nbd/server.c| 20 +---
>  qemu-nbd.c  |  4 +++-
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..5fe14786414 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> 
>  extern const BlockExportDriver blk_exp_nbd;
> 
> +/*
> + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> + */
> +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10

Suggest

s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/


> +
>  /* Handshake phase structs - this struct is passed on the wire */
> 
>  typedef struct NBDOption {
> @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
>  NBDExport *nbd_export_find(const char *name);
> 
>  void nbd_client_new(QIOChannelSocket *sioc,
> +uint32_t handshake_limit,

s/handshake_limit/handshake_max_secs/

to make the units of the parameter self-documenting.

Since this is a non-functional suggestion

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 1/7] nbd: Minor style fixes

[Daniel P . Berrangé]

in $subject  s/style/style & typo/

On Wed, Aug 07, 2024 at 12:43:27PM -0500, Eric Blake wrote:
> Touch up a comment with the wrong type name, and an over-long line,
> both noticed while working on upcoming patches.
> 
> Signed-off-by: Eric Blake 
> ---
>  nbd/server.c | 2 +-
>  qemu-nbd.c   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Daniel P . Berrangé]

On Mon, Aug 05, 2024 at 09:21:36PM -0500, Eric Blake wrote:
> Since an NBD server may be long-living, serving clients that
> repeatedly connect and disconnect, it can be more efficient to clean
> up after each client disconnects, rather than storing a list of
> resources to clean up when the server exits.  Rewrite the list of
> known clients to be double-linked so that we can get O(1) deletion to
> keep the list pruned to size as clients exit.  This in turn requires
> each client to track an opaque pointer of owner information (although
> qemu-nbd doesn't need to refer to it).

I tend to feel that this needs to be squashed into the previous
patch.  The previous patch effectively creates unbounded memory
usage in the NBD server. ie consider a client that connects and
immediately disconnects, not sending any data, in a tight loop.
It will "leak" NBDConn & QIOChanelSocket pointers for each
iteration of the loop, only to be cleaned up when the NBD Server
is shutdown.

Especially given that we tagged the previous commit with a CVE
and not this commit,  people could be misled into backporting
only the former commit leaving them open to the leak.

> 
> Signed-off-by: Eric Blake 
> ---
>  include/block/nbd.h |  4 +++-
>  blockdev-nbd.c  | 27 ---
>  nbd/server.c| 15 ---
>  qemu-nbd.c  |  2 +-
>  4 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..7dce9b9c35b 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -405,7 +405,9 @@ NBDExport *nbd_export_find(const char *name);
>  void nbd_client_new(QIOChannelSocket *sioc,
>  QCryptoTLSCreds *tlscreds,
>  const char *tlsauthz,
> -void (*close_fn)(NBDClient *, bool));
> +void (*close_fn)(NBDClient *, bool),
> +void *owner);
> +void *nbd_client_owner(NBDClient *client);
>  void nbd_client_get(NBDClient *client);
>  void nbd_client_put(NBDClient *client);
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index b8f00f402c6..660f89d881e 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -23,7 +23,7 @@
> 
>  typedef struct NBDConn {
>  QIOChannelSocket *cioc;
> -QSLIST_ENTRY(NBDConn) next;
> +QLIST_ENTRY(NBDConn) next;
>  } NBDConn;
> 
>  typedef struct NBDServerData {
> @@ -32,10 +32,11 @@ typedef struct NBDServerData {
>  char *tlsauthz;
>  uint32_t max_connections;
>  uint32_t connections;
> -QSLIST_HEAD(, NBDConn) conns;
> +QLIST_HEAD(, NBDConn) conns;
>  } NBDServerData;
> 
>  static NBDServerData *nbd_server;
> +static uint32_t nbd_cookie; /* Generation count of nbd_server */
>  static int qemu_nbd_connections = -1; /* Non-negative if this is qemu-nbd */
> 
>  static void nbd_update_server_watch(NBDServerData *s);
> @@ -57,10 +58,16 @@ int nbd_server_max_connections(void)
> 
>  static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>  {
> +NBDConn *conn = nbd_client_owner(client);
> +
>  assert(qemu_in_main_thread() && nbd_server);
> 
> +object_unref(OBJECT(conn->cioc));
> +QLIST_REMOVE(conn, next);
> +g_free(conn);
> +
>  nbd_client_put(client);
> -assert(nbd_server->connections > 0);
> +assert(nbd_server && nbd_server->connections > 0);
>  nbd_server->connections--;
>  nbd_update_server_watch(nbd_server);
>  }
> @@ -74,12 +81,12 @@ static void nbd_accept(QIONetListener *listener, 
> QIOChannelSocket *cioc,
>  nbd_server->connections++;
>  object_ref(OBJECT(cioc));
>  conn->cioc = cioc;
> -QSLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
> +QLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
>  nbd_update_server_watch(nbd_server);
> 
>  qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
>  nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
> -   nbd_blockdev_client_closed);
> +   nbd_blockdev_client_closed, conn);
>  }
> 
>  static void nbd_update_server_watch(NBDServerData *s)
> @@ -93,6 +100,8 @@ static void nbd_update_server_watch(NBDServerData *s)
> 
>  static void nbd_server_free(NBDServerData *server)
>  {
> +NBDConn *conn, *tmp;
> +
>  if (!server) {
>  return;
>  }
> @@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
>   */
>  qio_net_listener_disconnect(server->listener);
>  object_unref(OBJECT(server->listener));
> -while (!QSLIST_EMPTY(&server->conns)) {
> -NBDConn *conn = QSLIST_FIRST(&server->conns);
> -
> +QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
>  qio_channel_shutdown(QIO_CHANNEL(conn->cioc), 
> QIO_CHANNEL_SHUTDOWN_BOTH,
>   NULL);
> -object_unref(OBJECT(conn->cioc));
> -QSLIST_REMOVE_HEAD(&server->conns, next);
> -g_free(conn);
>  }
> 
>  AIO_WAIT_WHILE_UNLOCKED(NULL, server->con

Re: [PATCH v3 1/2] nbd: CVE-2024-7409: Close stray client sockets at server shutdown

[Daniel P . Berrangé]

On Mon, Aug 05, 2024 at 09:21:35PM -0500, Eric Blake wrote:
> A malicious client can attempt to connect to an NBD server, and then
> intentionally delay progress in the handshake, including if it does
> not know the TLS secrets.  Although this behavior can be bounded by
> the max-connections parameter, the QMP nbd-server-start currently
> defaults to unlimited incoming client connections.  Worse, if the
> client waits to close the socket until after the QMP nbd-server-stop
> command is executed, qemu will then SEGV when trying to dereference
> the NULL nbd_server global whish is no longer present, which amounts
> to a denial of service attack.  If another NBD server is started
> before the malicious client disconnects, I cannot rule out additional
> adverse effects when the old client interferes with the connection
> count of the new server.
> 
> For environments without this patch, the CVE can be mitigated by
> ensuring (such as via a firewall) that only trusted clients can
> connect to an NBD server.  Note that using frameworks like libvirt
> that ensure that TLS is used and that nbd-server-stop is not executed
> while any trusted clients are still connected will only help if there
> is also no possibility for an untrusted client to open a connection
> but then stall on the NBD handshake.
> 
> This patch fixes the problem by tracking all client sockets opened
> while the server is running, and forcefully closing any such sockets
> remaining without a completed handshake at the time of
> nbd-server-stop, then waiting until the coroutines servicing those
> sockets notice the state change.  nbd-server-stop may now take
> slightly longer to execute, but the extra time is independent of
> client response behaviors, and is generally no worse than the time
> already taken by the blk_exp_close_all_type() that disconnects all
> clients that completed handshakes (since that code also has an
> AIO_WAIT_WHILE_UNLOCKED).  For a long-running server with lots of
> clients rapidly connecting and disconnecting, the memory used to track
> all client sockets can result in some memory overhead, but it is not a
> leak; the next patch will further optimize that by cleaning up memory
> as clients go away.  At any rate, this patch in isolation is
> sufficient to fix the CVE.
> 
> This patch relies heavily on the fact that nbd/server.c guarantees
> that it only calls nbd_blockdev_client_closed() from the main loop
> (see the assertion in nbd_client_put() and the hoops used in
> nbd_client_put_nonzero() to achieve that); if we did not have that
> guarantee, we would also need a mutex protecting our accesses of the
> list of connections to survive re-entrancy from independent iothreads.
> 
> Although I did not actually try to test old builds, it looks like this
> problem has existed since at least commit 862172f45c (v2.12.0, 2017) -
> even back in that patch to start using a QIONetListener to handle
> listening on multiple sockets, nbd_server_free() was already unaware
> that the nbd_blockdev_client_closed callback can be reached later by a
> client thread that has not completed handshakes (and therefore the
> client's socket never got added to the list closed in
> nbd_export_close_all), despite that patch intentionally tearing down
> the QIONetListener to prevent new clients.
> 
> Reported-by: Alexander Ivanov 
> Fixes: CVE-2024-7409
> Signed-off-by: Eric Blake 
> ---
>  blockdev-nbd.c | 30 ++
>  1 file changed, 30 insertions(+)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 11/18] qapi/crypto: Rename QCryptoHashAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 02:26:49PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, Jul 30, 2024 at 10:10:25AM +0200, Markus Armbruster wrote:
> >> QAPI's 'prefix' feature can make the connection between enumeration
> >> type and its constants less than obvious.  It's best used with
> >> restraint.
> >> 
> >> QCryptoHashAlgorithm has a 'prefix' that overrides the generated
> >> enumeration constants' prefix to QCRYPTO_HASH_ALG.
> >> 
> >> We could simply drop 'prefix', but then the prefix becomes
> >> QCRYPTO_HASH_ALGORITHM, which is rather long.
> >> 
> >> We could additionally rename the type to QCryptoHashAlg, but I think
> >> the abbreviation "alg" is less than clear.
> >
> > I would have gone with this, but it is a bit of a bike shed colouring
> > debate so I'm not fussed
> 
> Either solution seems okay, so I went with my personal preference.  Do
> feel free to state yours and ask me to respin!

After reviewing the patches that follow, I'd observe that picking
Algo has made the following patches much larger than if it had
stuck with Alg. Basically changing both the types & constants,
instead of only having to change the types. 

> 
> >> Rename the type to QCryptoHashAlgo instead.  The prefix becomes to
> >> QCRYPTO_HASH_ALGO.
> >> 
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  qapi/crypto.json| 17 +-
> >>  crypto/blockpriv.h  |  2 +-
> >>  crypto/hashpriv.h   |  2 +-
> >>  crypto/hmacpriv.h   |  4 +--
> >>  crypto/ivgenpriv.h  |  2 +-
> >>  include/crypto/afsplit.h|  8 ++---
> >>  include/crypto/block.h  |  2 +-
> >>  include/crypto/hash.h   | 18 +-
> >>  include/crypto/hmac.h   |  6 ++--
> >>  include/crypto/ivgen.h  |  6 ++--
> >>  include/crypto/pbkdf.h  | 10 +++---
> >>  backends/cryptodev-builtin.c|  8 ++---
> >>  backends/cryptodev-lkcf.c   | 10 +++---
> >>  block/parallels-ext.c   |  2 +-
> >>  block/quorum.c  |  4 +--
> >>  crypto/afsplit.c|  6 ++--
> >>  crypto/block-luks.c | 16 -
> >>  crypto/block.c  |  2 +-
> >>  crypto/hash-afalg.c | 26 +++
> >>  crypto/hash-gcrypt.c| 20 +--
> >>  crypto/hash-glib.c  | 20 +--
> >>  crypto/hash-gnutls.c| 20 +--
> >>  crypto/hash-nettle.c| 18 +-
> >>  crypto/hash.c   | 30 -
> >>  crypto/hmac-gcrypt.c| 22 ++---
> >>  crypto/hmac-glib.c  | 22 ++---
> >>  crypto/hmac-gnutls.c| 22 ++---
> >>  crypto/hmac-nettle.c| 22 ++---
> >>  crypto/hmac.c   |  2 +-
> >>  crypto/ivgen.c  |  4 +--
> >>  crypto/pbkdf-gcrypt.c   | 36 ++--
> >>  crypto/pbkdf-gnutls.c   | 36 ++--
> >>  crypto/pbkdf-nettle.c   | 32 +-
> >>  crypto/pbkdf-stub.c |  4 +--
> >>  crypto/pbkdf.c  |  2 +-
> >>  hw/misc/aspeed_hace.c   | 16 -
> >>  io/channel-websock.c|  2 +-
> >>  target/i386/sev.c   |  6 ++--
> >>  tests/bench/benchmark-crypto-akcipher.c | 12 +++
> >>  tests/bench/benchmark-crypto-hash.c | 10 +++---
> >>  tests/bench/benchmark-crypto-hmac.c |  6 ++--
> >>  tests/unit/test-crypto-afsplit.c| 10 +++---
> >>  tests/unit/test-crypto-akcipher.c   |  6 ++--
> >>  tests/unit/test-crypto-block.c  | 16 -
> >>  tests/unit/test-crypto-hash.c   | 42 +++
> >>  tests/unit/test-crypto-hmac.c   | 16 -
> >>  tests/unit/test-crypto-ivgen.c  |  8 ++---
> >>  tests/unit/test-crypto-pbkdf.c  | 44 -
> >>  ui/vnc.c|  2 +-
> >>  util/hbitmap.c  |  2 +-
> >>  crypto/akcipher-gcrypt.c.inc| 14 
> >>  crypto/akcipher-nettle.c.inc| 26 +++
> >>  52 files changed, 350 insertions(+), 351 deletions(-)
> >
> > Acked-by: Daniel P. Berrangé 
> 
> Thanks!
> 

With 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: [PATCH 01/18] qapi: Smarter camel_to_upper() to reduce need for 'prefix'

[Daniel P . Berrangé]

  S390_CPU_POLARIZATION
> CpuS390StateCPUS390_STATE
>   CPU_S390_STATE
>   S390_CPU_STATE
> QAuthZListFormatQ_AUTHZ_LIST_FORMAT
>   QAUTH_Z_LIST_FORMAT
>   QAUTHZ_LIST_FORMAT
> QAuthZListPolicyQ_AUTHZ_LIST_POLICY
>   QAUTH_Z_LIST_POLICY
>   QAUTHZ_LIST_POLICY
> QCryptoAkCipherAlgorithmQ_CRYPTO_AK_CIPHER_ALGORITHM
>   QCRYPTO_AK_CIPHER_ALGORITHM
>   QCRYPTO_AKCIPHER_ALG
> QCryptoAkCipherKeyType  Q_CRYPTO_AK_CIPHER_KEY_TYPE
>   QCRYPTO_AK_CIPHER_KEY_TYPE
>   QCRYPTO_AKCIPHER_KEY_TYPE
> QCryptoCipherAlgorithm  Q_CRYPTO_CIPHER_ALGORITHM
>   QCRYPTO_CIPHER_ALGORITHM
>   QCRYPTO_CIPHER_ALG
> QCryptoHashAlgorithmQ_CRYPTO_HASH_ALGORITHM
>   QCRYPTO_HASH_ALGORITHM
>   QCRYPTO_HASH_ALG
> QCryptoIVGenAlgorithm   Q_CRYPTOIV_GEN_ALGORITHM
>   QCRYPTO_IV_GEN_ALGORITHM
>   QCRYPTO_IVGEN_ALG
> QCryptoRSAPaddingAlgorithm  Q_CRYPTORSA_PADDING_ALGORITHM
>   QCRYPTO_RSA_PADDING_ALGORITHM
>   QCRYPTO_RSA_PADDING_ALG
> QCryptodevBackendAlgTypeQ_CRYPTODEV_BACKEND_ALG_TYPE
>   QCRYPTODEV_BACKEND_ALG_TYPE
>   QCRYPTODEV_BACKEND_ALG
> QCryptodevBackendServiceTypeQ_CRYPTODEV_BACKEND_SERVICE_TYPE
>   QCRYPTODEV_BACKEND_SERVICE_TYPE
>   QCRYPTODEV_BACKEND_SERVICE
> 
> Subsequent commits will tweak things to remove most of these prefixes.
> Only QAUTHZ_LIST_FORMAT and QAUTHZ_LIST_POLICY will remain.

IIUC from above those two result in 

QAUTH_Z_LIST_FORMAT
QAUTH_Z_LIST_POLICY

Is it possible to add a 3rd rule

 *  Single uppercase letter folds into the previous word

or are there valid cases where we have a single uppercase
that we want to preserve ?

It sure would be nice to eliminate the 'prefix' concept,
that we've clearly over-used, if we can kill the only 2
remaining examples.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-core.json |  3 +-
>  qapi/common.json |  1 +
>  qapi/crypto.json |  6 ++--
>  qapi/cryptodev.json  |  1 -
>  qapi/ebpf.json   |  1 +
>  qapi/machine.json|  1 +
>  qapi/migration.json  |  1 +
>  qapi/ui.json |  2 ++
>  scripts/qapi/common.py   | 42 ++++++----------
>  scripts/qapi/schema.py   |  2 +-
>  tests/qapi-schema/alternate-array.out|  1 -
>  tests/qapi-schema/comments.out   |  1 -
>  tests/qapi-schema/doc-good.out   |  1 -
>  tests/qapi-schema/empty.out  |  1 -
>  tests/qapi-schema/include-repetition.out |  1 -
>  tests/qapi-schema/include-simple.out |  1 -
>  tests/qapi-schema/indented-expr.out  |  1 -
>  tests/qapi-schema/qapi-schema-test.json  |  1 +
>  tests/qapi-schema/qapi-schema-test.out   |  2 +-
>  19 files changed, 37 insertions(+), 33 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 09/18] qapi/machine: Rename CpuS390* to S390Cpu, and drop 'prefix'

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:23AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> CpuS390Entitlement has a 'prefix' to change the generated enumeration
> constants' prefix from CPU_S390_POLARIZATION to S390_CPU_POLARIZATION.
> Rename the type to S390CpuEntitlement, so that 'prefix' is not needed.
> 
> Likewise change CpuS390Polarization to S390CpuPolarization, and
> CpuS390State to S390CpuState.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/machine-common.json|  5 ++---
>  qapi/machine-target.json| 11 +--
>  qapi/machine.json   |  9 -
>  qapi/pragma.json|  6 +++---
>  include/hw/qdev-properties-system.h |  2 +-
>  include/hw/s390x/cpu-topology.h |  2 +-
>  target/s390x/cpu.h  |  2 +-
>  hw/core/qdev-properties-system.c|  6 +++---
>  hw/s390x/cpu-topology.c |  6 +++---
>  9 files changed, 23 insertions(+), 26 deletions(-)

Reviewed-by: Daniel P. Berrangé 

With 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: [PATCH 18/18] qapi/cryptodev: Rename QCryptodevBackendAlgType to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:32AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptodevBackendAlgType a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTODEV_BACKEND_ALG.
> 
> We could simply drop 'prefix', but I think the abbreviation "alg" is
> less than clear.
> 
> Additionally rename the type to QCryptodevBackendAlgoType.  The prefix
> becomes QCRYPTODEV_BACKEND_ALGO_TYPE.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/cryptodev.json  |  5 ++---
>  include/sysemu/cryptodev.h   |  2 +-
>  backends/cryptodev-builtin.c |  6 +++---
>  backends/cryptodev-lkcf.c|  4 ++--
>  backends/cryptodev.c |  6 +++---
>  hw/virtio/virtio-crypto.c| 14 +++---
>  6 files changed, 18 insertions(+), 19 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 17/18] qapi/cryptodev: Drop unwanted 'prefix'

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:31AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptodevBackendServiceType has a 'prefix' that overrides the
> generated enumeration constants' prefix to QCRYPTODEV_BACKEND_SERVICE.
> 
> Drop it.  The prefix becomes QCRYPTODEV_BACKEND_SERVICE_TYPE.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/cryptodev.json |  1 -
>  backends/cryptodev-builtin.c|  8 
>  backends/cryptodev-lkcf.c   |  2 +-
>  backends/cryptodev-vhost-user.c |  6 +++---
>  backends/cryptodev.c|  6 +++---
>  hw/virtio/virtio-crypto.c   | 10 +++++-
>  6 files changed, 16 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 16/18] qapi/crypto: Rename QCryptoAFAlg to QCryptoAFAlgo

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:30AM +0200, Markus Armbruster wrote:
> For consistency with other types names *Algo.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  crypto/afalgpriv.h| 14 +++---
>  crypto/hmacpriv.h |  2 +-
>  crypto/afalg.c|  8 
>  crypto/cipher-afalg.c | 12 ++--
>  crypto/hash-afalg.c   | 14 +++---
>  5 files changed, 25 insertions(+), 25 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 15/18] qapi/crypto: Rename QCryptoRSAPaddingAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:29AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoRSAPaddingAlgorithm has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_RSA_PADDING_ALG.
> 
> We could simply drop 'prefix', but then the prefix becomes
> QCRYPTO_RSA_PADDING_ALGORITHM, which is rather long.
> 
> We could additionally rename the type to QCryptoRSAPaddingAlg, but I
> think the abbreviation "alg" is less than clear.
> 
> Rename the type to QCryptoRSAPaddingAlgo instead.  The prefix becomes
> QCRYPTO_RSA_PADDING_ALGO.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json|  9 -
>  backends/cryptodev-builtin.c|  6 +++---
>  backends/cryptodev-lkcf.c   | 10 +-
>  tests/bench/benchmark-crypto-akcipher.c | 12 ++--
>  tests/unit/test-crypto-akcipher.c   | 10 +-
>  crypto/akcipher-gcrypt.c.inc| 18 +-
>  crypto/akcipher-nettle.c.inc| 18 +-
>  7 files changed, 41 insertions(+), 42 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 14/18] qapi/crypto: Rename QCryptoAkCipherAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:28AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoAkCipherAlgorithm has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_AKCIPHER_ALG.
> 
> We could simply drop 'prefix', but then the prefix becomes
> QCRYPTO_AK_CIPHER_ALGORITHM, which is rather long.
> 
> We could additionally rename the type to QCryptoAkCipherAlg, but I
> think the abbreviation "alg" is less than clear.
> 
> Rename the type to QCryptoAkCipherAlgo instead.  The prefix becomes
> QCRYPTO_AK_CIPHER_ALGO.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json|  7 +++
>  crypto/akcipherpriv.h   |  2 +-
>  backends/cryptodev-builtin.c|  4 ++--
>  backends/cryptodev-lkcf.c   |  4 ++--
>  crypto/akcipher.c   |  2 +-
>  tests/bench/benchmark-crypto-akcipher.c |  2 +-
>  tests/unit/test-crypto-akcipher.c   | 10 +-
>  crypto/akcipher-gcrypt.c.inc    |  4 ++--
>  crypto/akcipher-nettle.c.inc|  4 ++--
>  9 files changed, 19 insertions(+), 20 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 13/18] qapi/crypto: Rename QCryptoIVGenAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:27AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoIVGenAlgorithm has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_IVGEN_ALG.
> 
> We could simply drop 'prefix', but then the prefix becomes
> QCRYPTO_IV_GEN_ALGORITHM, which is rather long.
> 
> We could additionally rename the type to QCryptoIVGenAlg, but I think
> the abbreviation "alg" is less than clear.
> 
> Rename the type to QCryptoIVGenAlgo instead.  The prefix becomes
> QCRYPTO_IV_GEN_ALGO.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json   |  9 -
>  crypto/ivgenpriv.h |  2 +-
>  include/crypto/ivgen.h | 14 +++---
>  crypto/block-luks.c| 16 
>  crypto/block-qcow.c|  2 +-
>  crypto/ivgen.c | 10 +-
>  tests/unit/test-crypto-block.c | 14 +++---
>  tests/unit/test-crypto-ivgen.c | 22 +++---
>  8 files changed, 44 insertions(+), 45 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 12/18] qapi/crypto: Rename QCryptoCipherAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:26AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoCipherAlgorithm has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_CIPHER_ALG.
> 
> We could simply drop 'prefix', but then the prefix becomes
> QCRYPTO_CIPHER_ALGORITHM, which is rather long.
> 
> We could additionally rename the type to QCryptoCipherAlg, but I think
> the abbreviation "alg" is less than clear.
> 
> Rename the type to QCryptoCipherAlgo instead.  The prefix becomes
> QCRYPTO_CIPHER_ALGO.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-core.json  |  2 +-
>  qapi/crypto.json  |  9 ++-
>  crypto/blockpriv.h|  4 +-
>  crypto/cipherpriv.h   |  2 +-
>  crypto/ivgenpriv.h|  2 +-
>  include/crypto/cipher.h   | 18 +++---
>  include/crypto/ivgen.h| 10 +--
>  include/crypto/pbkdf.h|  4 +-
>  backends/cryptodev-builtin.c  | 16 ++---
>  block/rbd.c   |  4 +-
>  crypto/block-luks.c   | 92 +--
>  crypto/block-qcow.c   |  4 +-
>  crypto/block.c|  2 +-
>  crypto/cipher-afalg.c | 24 +++
>  crypto/cipher.c   | 72 ++---
>  crypto/ivgen.c|  4 +-
>  crypto/secret_common.c|  2 +-
>  tests/bench/benchmark-crypto-cipher.c | 22 +++
>  tests/unit/test-crypto-block.c| 14 ++--
>  tests/unit/test-crypto-cipher.c   | 66 +--
>  tests/unit/test-crypto-ivgen.c|  8 +--
>  ui/vnc.c  |  4 +-
>  crypto/cipher-builtin.c.inc   | 18 +++---
>  crypto/cipher-gcrypt.c.inc| 56 
>  crypto/cipher-gnutls.c.inc| 38 +--
>  crypto/cipher-nettle.c.inc| 58 -
>  26 files changed, 277 insertions(+), 278 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 11/18] qapi/crypto: Rename QCryptoHashAlgorithm to *Algo, and drop prefix

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:25AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoHashAlgorithm has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_HASH_ALG.
> 
> We could simply drop 'prefix', but then the prefix becomes
> QCRYPTO_HASH_ALGORITHM, which is rather long.
> 
> We could additionally rename the type to QCryptoHashAlg, but I think
> the abbreviation "alg" is less than clear.

I would have gone with this, but it is a bit of a bike shed colouring
debate so I'm not fussed

> 
> Rename the type to QCryptoHashAlgo instead.  The prefix becomes to
> QCRYPTO_HASH_ALGO.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json| 17 +-
>  crypto/blockpriv.h  |  2 +-
>  crypto/hashpriv.h   |  2 +-
>  crypto/hmacpriv.h   |  4 +--
>  crypto/ivgenpriv.h  |  2 +-
>  include/crypto/afsplit.h|  8 ++---
>  include/crypto/block.h  |  2 +-
>  include/crypto/hash.h   | 18 +-
>  include/crypto/hmac.h   |  6 ++--
>  include/crypto/ivgen.h  |  6 ++--
>  include/crypto/pbkdf.h  | 10 +++---
>  backends/cryptodev-builtin.c|  8 ++---
>  backends/cryptodev-lkcf.c   | 10 +++---
>  block/parallels-ext.c   |  2 +-
>  block/quorum.c  |  4 +--
>  crypto/afsplit.c|  6 ++--
>  crypto/block-luks.c | 16 -
>  crypto/block.c  |  2 +-
>  crypto/hash-afalg.c | 26 +++
>  crypto/hash-gcrypt.c| 20 +--
>  crypto/hash-glib.c  | 20 +--
>  crypto/hash-gnutls.c| 20 +--
>  crypto/hash-nettle.c| 18 +-
>  crypto/hash.c   | 30 -
>  crypto/hmac-gcrypt.c| 22 ++---
>  crypto/hmac-glib.c  | 22 ++---
>  crypto/hmac-gnutls.c| 22 ++---
>  crypto/hmac-nettle.c| 22 ++---
>  crypto/hmac.c   |  2 +-
>  crypto/ivgen.c  |  4 +--
>  crypto/pbkdf-gcrypt.c   | 36 ++--
>  crypto/pbkdf-gnutls.c   | 36 ++--
>  crypto/pbkdf-nettle.c   | 32 +-
>  crypto/pbkdf-stub.c |  4 +--
>  crypto/pbkdf.c  |  2 +-
>  hw/misc/aspeed_hace.c   | 16 -
>  io/channel-websock.c|  2 +-
>  target/i386/sev.c   |  6 ++--
>  tests/bench/benchmark-crypto-akcipher.c | 12 +++
>  tests/bench/benchmark-crypto-hash.c | 10 +++---
>  tests/bench/benchmark-crypto-hmac.c |  6 ++--
>  tests/unit/test-crypto-afsplit.c| 10 +++---
>  tests/unit/test-crypto-akcipher.c   |  6 ++--
>  tests/unit/test-crypto-block.c  | 16 -
>  tests/unit/test-crypto-hash.c   | 42 +++
>  tests/unit/test-crypto-hmac.c   | 16 -
>  tests/unit/test-crypto-ivgen.c  |  8 ++---
>  tests/unit/test-crypto-pbkdf.c  | 44 -
>  ui/vnc.c            |  2 +-
>  util/hbitmap.c  |  2 +-
>  crypto/akcipher-gcrypt.c.inc| 14 
>  crypto/akcipher-nettle.c.inc| 26 +++
>  52 files changed, 350 insertions(+), 351 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 10/18] qapi/crypto: Drop unwanted 'prefix'

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:24AM +0200, Markus Armbruster wrote:
> QAPI's 'prefix' feature can make the connection between enumeration
> type and its constants less than obvious.  It's best used with
> restraint.
> 
> QCryptoAkCipherKeyType has a 'prefix' that overrides the generated
> enumeration constants' prefix to QCRYPTO_AKCIPHER_KEY_TYPE.
> 
> Drop it.  The prefix becomes QCRYPTO_AK_CIPHER_KEY_TYPE.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json|  1 -
>  backends/cryptodev-builtin.c|  4 ++--
>  backends/cryptodev-lkcf.c   |  6 +++---
>  tests/bench/benchmark-crypto-akcipher.c |  2 +-
>  tests/unit/test-crypto-akcipher.c   | 28 -
>  crypto/akcipher-gcrypt.c.inc|  8 +++
>  crypto/akcipher-nettle.c.inc|  8 +++
>  crypto/rsakey-builtin.c.inc |  4 ++--
>  crypto/rsakey-nettle.c.inc      |  4 ++--
>  9 files changed, 32 insertions(+), 33 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH 05/18] qapi/crypto: Drop temporary 'prefix'

[Daniel P . Berrangé]

On Tue, Jul 30, 2024 at 10:10:19AM +0200, Markus Armbruster wrote:
> Recent commit "qapi: Smarter camel_to_upper() to reduce need for
> 'prefix'" added two temporary 'prefix' to delay changing the generated
> code.
> 
> Revert them.  This improves QCryptoBlockFormat's generated enumeration
> constant prefix from Q_CRYPTO_BLOCK_FORMAT to QCRYPTO_BLOCK_FORMAT,
> and QCryptoBlockLUKSKeyslotState's from
> Q_CRYPTO_BLOCKLUKS_KEYSLOT_STATE to QCRYPTO_BLOCK_LUKS_KEYSLOT_STATE.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/crypto.json   |  2 --
>  block/crypto.c | 10 +-
>  block/qcow.c   |  2 +-
>  block/qcow2.c  | 10 +-
>  crypto/block-luks.c|  4 ++--
>  crypto/block.c |  4 ++--
>  tests/unit/test-crypto-block.c | 14 +++---
>  7 files changed, 22 insertions(+), 24 deletions(-)

Acked-by: Daniel P. Berrangé 


With 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: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 07:52:00PM +0900, Akihiko Odaki wrote:
> On 2024/07/16 18:53, Daniel P. Berrangé wrote:
> > On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> > > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > > that is appropriate for storing runtime files. It corresponds to "run"
> > > directory in Unix.
> > > 
> > > With a tree-wide search, it was found that there are several cases
> > > where such a functionality is implemented so let's have one as a common
> > > utlity function.
> > > 
> > > A notable feature of qemu_get_runtime_dir() is that it uses
> > > $XDG_RUNTIME_DIR if available. While the function is often called by
> > > executables which requires root privileges, it is still possible that
> > > they are called from a user without privilege to write the system
> > > runtime directory. In fact, I decided to write this patch when I ran
> > > virtiofsd in a Linux namespace created by a normal user and realized
> > > it tries to write the system runtime directory, not writable in this
> > > case. $XDG_RUNTIME_DIR should provide a writable directory in such
> > > cases.
> > > 
> > > This function does not use qemu_get_local_state_dir() or its logic
> > > for Windows. Actually the implementation of qemu_get_local_state_dir()
> > > for Windows seems not right as it calls g_get_system_data_dirs(),
> > > which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> > > "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> > > to provide. Instead, this function try to use the following in order:
> > > - $XDG_RUNTIME_DIR
> > > - LocalAppData folder
> > > - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > > 
> > > This function does not use g_get_user_runtime_dir() either as it
> > > falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> > > available. In the case, we rather use:
> > > get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > > 
> > > Signed-off-by: Akihiko Odaki 
> > > Message-Id: <20230921075425.16738-2-akihiko.od...@daynix.com>
> > > ---
> > >   include/qemu/osdep.h | 12 
> > >   util/oslib-posix.c   | 11 +++
> > >   util/oslib-win32.c   | 26 ++
> > >   3 files changed, 49 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 191916f38e6d..fe8609fc1375 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
> > >*/
> > >   char *qemu_get_local_state_dir(void);
> > > +/**
> > > + * qemu_get_runtime_dir:
> > > + *
> > > + * Return a dynamically allocated directory path that is appropriate for 
> > > storing
> > > + * runtime files. It corresponds to "run" directory in Unix, and uses
> > > + * $XDG_RUNTIME_DIR if available.
> > > + *
> > > + * The caller is responsible for releasing the value returned with 
> > > g_free()
> > > + * after use.
> > > + */
> > > +char *qemu_get_runtime_dir(void);
> > > +
> > >   /**
> > >* qemu_getauxval:
> > >* @type: the auxiliary vector key to lookup
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index e76441695bdc..9599509a9aa7 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
> > >   return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
> > >   }
> > > +char *
> > > +qemu_get_runtime_dir(void)
> > > +{
> > > +char *env = getenv("XDG_RUNTIME_DIR");
> > > +if (env) {
> > > +return g_strdup(env);
> > > +}
> > > +
> > > +return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> > > +}
> > 
> > I'm not convinced this is the correct logic to be following.
> > 
> > In the cover letter you mention not using g_get_user_runtime_dir()
> > because it falls back to XDG_CACHE_HOME, and we need to fallback
> > to LOCALSTATEDIR/run. This is not right for normal users though,
> > where falling back to LOCALSTATEDIR/run is always wrong, as it
> > won't be writable - the g_get_user_runtime_d

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
> 16.07.2024 10:27, Akihiko Odaki wrote:
> > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > that is appropriate for storing runtime files. It corresponds to "run"
> > directory in Unix.
> 
> Since runtime dir is always used with a filename within, how about
> 
>   char *qemu_get_runtime_path(const char *filename)
> 
> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?

Yeah, I agree, every single caller of the function goes on to call
g_build_filename with the result. The helper should just be building
the filename itself.

With 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: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.
> 
> With a tree-wide search, it was found that there are several cases
> where such a functionality is implemented so let's have one as a common
> utlity function.
> 
> A notable feature of qemu_get_runtime_dir() is that it uses
> $XDG_RUNTIME_DIR if available. While the function is often called by
> executables which requires root privileges, it is still possible that
> they are called from a user without privilege to write the system
> runtime directory. In fact, I decided to write this patch when I ran
> virtiofsd in a Linux namespace created by a normal user and realized
> it tries to write the system runtime directory, not writable in this
> case. $XDG_RUNTIME_DIR should provide a writable directory in such
> cases.
> 
> This function does not use qemu_get_local_state_dir() or its logic
> for Windows. Actually the implementation of qemu_get_local_state_dir()
> for Windows seems not right as it calls g_get_system_data_dirs(),
> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> to provide. Instead, this function try to use the following in order:
> - $XDG_RUNTIME_DIR
> - LocalAppData folder
> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> This function does not use g_get_user_runtime_dir() either as it
> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> available. In the case, we rather use:
> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> Signed-off-by: Akihiko Odaki 
> Message-Id: <20230921075425.16738-2-akihiko.od...@daynix.com>
> ---
>  include/qemu/osdep.h | 12 
>  util/oslib-posix.c   | 11 +++
>  util/oslib-win32.c   | 26 ++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 191916f38e6d..fe8609fc1375 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
>   */
>  char *qemu_get_local_state_dir(void);
>  
> +/**
> + * qemu_get_runtime_dir:
> + *
> + * Return a dynamically allocated directory path that is appropriate for 
> storing
> + * runtime files. It corresponds to "run" directory in Unix, and uses
> + * $XDG_RUNTIME_DIR if available.
> + *
> + * The caller is responsible for releasing the value returned with g_free()
> + * after use.
> + */
> +char *qemu_get_runtime_dir(void);
> +
>  /**
>   * qemu_getauxval:
>   * @type: the auxiliary vector key to lookup
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index e76441695bdc..9599509a9aa7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
>  return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +char *env = getenv("XDG_RUNTIME_DIR");
> +if (env) {
> +return g_strdup(env);
> +}
> +
> +return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}

I'm not convinced this is the correct logic to be following.

In the cover letter you mention not using g_get_user_runtime_dir()
because it falls back to XDG_CACHE_HOME, and we need to fallback
to LOCALSTATEDIR/run. This is not right for normal users though,
where falling back to LOCALSTATEDIR/run is always wrong, as it
won't be writable - the g_get_user_runtime_dir() fallback is
desirable for non-root users.

IMHO we should be doing something more like this

#ifndef WIN32
  if (geteuid() == 0) {
 return  get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
  } else {
#endif
 return g_get_user_runtime_dir();
#ifndef WIN32
  }
#endif

> +
>  void qemu_set_tty_echo(int fd, bool echo)
>  {
>  struct termios tty;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b623830d624f..8c5a02ee881d 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -27,6 +27,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include 
> +#include 
>  #include 
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> @@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
>  return g_strdup(data_dirs[0]);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
> +if (size) {
> +char *env = g_malloc(size);
> +GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
> +return env;
> +}
> +
> +PWSTR wpath;
> +const wchar_t *cwpath;
> +if (!SHGetKnownFolderPath(&FOLDERID_LocalAppData, KF_FLAG_DEFAULT, NULL, 
> &wpath)) {
> +cwpath = wpath;
> +size = wcsrtombs(NULL, &cwpath, 0, &(mbstate_t){0}) + 1;
> +char *path = g_malloc(size);
> +wcsrtombs(path, &cwpath, size, &(mbstate_t){0});

Re: [PATCH 3/4] hw/pci: Convert rom_bar into OnOffAuto

[Daniel P . Berrangé]

On Sat, Jul 06, 2024 at 06:29:23PM +0900, Akihiko Odaki wrote:
> rom_bar is tristate but was defined as uint32_t so convert it into
> OnOffAuto to clarify that. For compatibility, a uint32 value set via
> QOM will be converted into OnOffAuto.
> 
> Signed-off-by: Akihiko Odaki 
> ---
>  docs/igd-assign.txt   |  2 +-
>  include/hw/pci/pci_device.h   |  2 +-
>  hw/pci/pci.c  | 57 
> +--
>  hw/vfio/pci-quirks.c  |  2 +-
>  hw/vfio/pci.c | 11 
>  hw/xen/xen_pt_load_rom.c  |  4 +--
>  tests/qtest/virtio-net-failover.c | 32 +++---
>  7 files changed, 81 insertions(+), 29 deletions(-)

> +static void rom_bar_set(Object *obj, Visitor *v, const char *name, void 
> *opaque,
> +Error **errp)
> +{
> +Property *prop = opaque;
> +Error *local_err = NULL;
> +int *ptr = object_field_prop_ptr(obj, prop);
> +uint32_t value;
> +
> +visit_type_enum(v, name, ptr, prop->info->enum_table, &local_err);
> +if (!local_err) {
> +return;
> +}
> +
> +if (visit_type_uint32(v, name, &value, NULL)) {
> +if (value) {
> +*ptr = ON_OFF_AUTO_ON;
> +warn_report("Specifying a number for rombar is deprecated; 
> replace a non-zero value with 'on'");
> +} else {
> +*ptr = ON_OFF_AUTO_OFF;
> +warn_report("Specifying a number for rombar is deprecated; 
> replace 0 with 'off'");
> +}

If you're going to say something is deprecated, you need to update
the deprecated.rst docs to make this visible to users.

With 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: [PATCH v2] Consider discard option when writing zeros

[Daniel P . Berrangé]

On Mon, Jun 24, 2024 at 06:08:26PM +0200, Kevin Wolf wrote:
> Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > Tested using:
> > 
> > Hi Nir,
> > This looks like a good candidate for the qemu-iotests test suite. Adding
> > it to the automated tests will protect against future regressions.
> > 
> > Please add the script and the expected output to
> > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > 
> > See the existing test cases in tests/qemu-iotests/ and
> > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > others are Python. I think shell makes sense for this test case. You
> > can copy the test framework boilerplate from an existing test case.
> 
> 'du' can't be used like this in qemu-iotests because it makes
> assumptions that depend on the filesystem. A test case replicating what
> Nir did manually would likely fail on XFS with its preallocation.
> 
> Maybe we could operate on a file exposed by the FUSE export that is
> backed by qcow2, and then you can use 'qemu-img map' on that qcow2 image
> to verify the allocation status. Somewhat complicated, but I think it
> could work.

A simpler option would be to use 'du' but with a fuzzy range test,
rather than an exact equality test.

For the tests which write 1 MB, check the 'du' usage is "at least 1MB",
for the tests which expect to unmap blocks, check that the 'du' usage
is "less than 256kb". This should be within bounds of xfs speculative
allocation.

With 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: [PATCH 0/2] block/crypto: do not require number of threads upfront

[Daniel P . Berrangé]

On Mon, May 27, 2024 at 11:58:49AM -0400, Stefan Hajnoczi wrote:
> The block layer does not know how many threads will perform I/O. It is 
> possible
> to exceed the number of threads that is given to qcrypto_block_open() and this
> can trigger an assertion failure in qcrypto_block_pop_cipher().
> 
> This patch series removes the n_threads argument and instead handles an
> arbitrary number of threads.
> ---
> Is it secure to store the key in QCryptoBlock? In this series I assumed the
> answer is yes since the QCryptoBlock's cipher state is equally sensitive, but
> I'm not familiar with this code or a crypto expert.

Yes, its a case of   this is undesirable, but we do it everywhere
already, so this isn't making it any worse.

For both patches

Acked-by: Daniel P. Berrangé 



With 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: [PATCH v3 2/2] iotests: test NBD+TLS+iothread

[Daniel P . Berrangé]

On Fri, May 31, 2024 at 01:04:59PM -0500, Eric Blake wrote:
> Prevent regressions when using NBD with TLS in the presence of
> iothreads, adding coverage the fix to qio channels made in the
> previous patch.
> 
> The shell function pick_unused_port() was copied from
> nbdkit.git/tests/functions.sh.in, where it had all authors from Red
> Hat, agreeing to the resulting relicensing from 2-clause BSD to GPLv2.
> 
> CC: qemu-sta...@nongnu.org
> CC: "Richard W.M. Jones" 
> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
>  tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
>  2 files changed, 222 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
>  create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out



> +# pick_unused_port
> +#
> +# Picks and returns an "unused" port, setting the global variable
> +# $port.
> +#
> +# This is inherently racy, but we need it because qemu does not currently
> +# permit NBD+TLS over a Unix domain socket
> +pick_unused_port ()
> +{
> +if ! (ss --version) >/dev/null 2>&1; then
> +_notrun "ss utility required, skipped this test"
> +fi
> +
> +# Start at a random port to make it less likely that two parallel
> +# tests will conflict.
> +port=$(( 5 + (RANDOM%15000) ))
> +while ss -ltn | grep -sqE ":$port\b"; do
> +((port++))
> +if [ $port -eq 65000 ]; then port=5; fi
> +done
> +echo picked unused port
> +}

In retrospect I'd probably have suggested putting this into
common.qemu as its conceptually independant of this test.


That's not a blocker though so

  Reviewed-by: Daniel P. Berrangé 

With 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: [PATCH v5 09/13] osdep: move O_DSYNC and O_DIRECT defines from file-posix

[Daniel P . Berrangé]

On Thu, May 23, 2024 at 04:55:18PM +0200, Stefano Garzarella wrote:
> These defines are also useful for vhost-user-blk when it is compiled
> in some POSIX systems that do not define them, so let's move them to
> “qemu/osdep.h”.
> 
> Suggested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Stefano Garzarella 
> ---
>  include/qemu/osdep.h | 14 ++
>  block/file-posix.c   | 14 --
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f61edcfdc2..e165b5cb1b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -325,6 +325,20 @@ void QEMU_ERROR("code path is reachable")
>  #define ESHUTDOWN 4099
>  #endif
>  
> +/* OS X does not have O_DSYNC */
> +#ifndef O_DSYNC
> +#ifdef O_SYNC
> +#define O_DSYNC O_SYNC
> +#elif defined(O_FSYNC)
> +#define O_DSYNC O_FSYNC
> +#endif
> +#endif
> +
> +/* Approximate O_DIRECT with O_DSYNC if O_DIRECT isn't available */
> +#ifndef O_DIRECT
> +#define O_DIRECT O_DSYNC
> +#endif

Please don't do this - we can't be confident that all code in
QEMU will be OK with O_DIRECT being simulated in this way.

I'm not convinced that the O_DSYNC simulation is a good idea
to do tree-wide either.

With 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: [PATCH v2 2/2] iotests: test NBD+TLS+iothread

[Daniel P . Berrangé]

On Fri, May 17, 2024 at 10:08:08PM -0500, Eric Blake wrote:
> Adding a bit of self-review (in case you want to amend this before
> pushing, instead of waiting for me to get back online),
> 
> On Fri, May 17, 2024 at 09:50:15PM GMT, Eric Blake wrote:
> > Prevent regressions when using NBD with TLS in the presence of
> > iothreads, adding coverage the fix to qio channels made in the
> > previous patch.
> > 
> > CC: qemu-sta...@nongnu.org
> > Signed-off-by: Eric Blake 
> > ---
> >  tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
> >  tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
> >  2 files changed, 224 insertions(+)
> >  create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
> >  create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

> > +
> > +# pick_unused_port
> > +# Copied from nbdkit/tests/functions.sh.in with compatible 2-clause BSD 
> > license
> 
> I'm not sure if I have to include the license text verbatim in this
> file, and/or have this function moved to a helper utility file.  The
> original source file that I borrowed pick_unused_port from has:
> 
> # Copyright Red Hat

I checked most of the relevant history, and this Copyright statement
does indeed appear correct - the code was all written by Richard.

Thus, you could invoke Red Hat's right to re-license and just declare
this copy to be under QEMU's normal GPL license, avoiding the question
of copying the license text.


> > +#
> > +# Picks and returns an "unused" port, setting the global variable
> > +# $port.
> > +#
> > +# This is inherently racy, but we need it because qemu does not currently
> > +# permit NBD+TLS over a Unix domain socket
> > +pick_unused_port ()
> > +{
> > +if ! (ss --version) >/dev/null 2>&1; then
> > +_notrun "ss utility required, skipped this test"
> > +fi
> > +
> > +# Start at a random port to make it less likely that two parallel
> > +# tests will conflict.
> > +port=$(( 5 + (RANDOM%15000) ))
> > +while ss -ltn | grep -sqE ":$port\b"; do
> > +((port++))
> > +if [ $port -eq 65000 ]; then port=5; fi
> 
> Also, common.nbd only probes:
> for ((port = 10809; port <= 10909; port++))
> and nbdkit's choice of starting with a random offset is interesting.

Yes, a random offset is a nice idea, massively reducing risk of
clashes through (un)lucky concurrent execution.



> > +echo
> > +echo === Cleaning up ===
> > +echo
> > +
> > +_send_qemu_cmd $h1 '{"execute":"quit"}' ''
> > +_send_qemu_cmd $h2 '{"execute":"quit"}' ''
> 
> Since the bug was exposed by this point, I didn't bother to do a clean
> shutdown of the mirror job or NBD export.  As is, testing that we shut
> down cleanly despite abandoning a job is probably not a bad idea.

Yeah, perhaps worthwhile, if you can get something that works reliably.
A reliable partial test is better than an unreliable full test, as we'll
just end up  killing the latter.

With 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: [PATCH] qio: Inherit follow_coroutine_ctx across TLS

[Daniel P . Berrangé]

On Wed, May 15, 2024 at 09:14:06PM -0500, Eric Blake wrote:
> Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
> assertion failure:
> 
> qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): 
> Assertion `qemu_get_current_aio_context() == 
> qemu_coroutine_get_aio_context(co)' failed.
> 
> It turns out that when we removed AioContext locking, we did so by
> having NBD tell its qio channels that it wanted to opt in to
> qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
> main channel, we did not opt in on the TLS wrapper channel.
> qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
> no coverage of NBD+TLS+iothread, or we would have noticed this
> regression sooner.  (I'll add that in the next patch)
> 
> But while we could manually opt in to the TLS thread in nbd/server.c,
> it is more generic if all qio channels that wrap other channels
> inherit the follow status, in the same way that they inherit feature
> bits.
> 
> CC: Stefan Hajnoczi 
> CC: Daniel P. Berrangé 
> CC: qemu-sta...@nongnu.org
> Fixes: https://issues.redhat.com/browse/RHEL-34786
> Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()", 
> v8.2.0)
> Signed-off-by: Eric Blake 

Reviewed-by: Daniel P. Berrangé 

> ---
> 
> Maybe we should turn ioc->follow_coroutine_ctx into a
> QIO_CHANNEL_FEATURE_* bit?

The features thus far have all been about properties of the underlying
channel, rather than properties related to the API usage pattern. So
I think it is fine to have it separate.


With 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: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Daniel P . Berrangé]

On Tue, May 07, 2024 at 06:52:50AM +0200, Jinpu Wang wrote:
> Hi Peter, hi Daniel,
> On Mon, May 6, 2024 at 5:29 PM Peter Xu  wrote:
> >
> > On Mon, May 06, 2024 at 12:08:43PM +0200, Jinpu Wang wrote:
> > > Hi Peter, hi Daniel,
> >
> > Hi, Jinpu,
> >
> > Thanks for sharing this test results.  Sounds like a great news.
> >
> > What's your plan next?  Would it then be worthwhile / possible moving QEMU
> > into that direction?  Would that greatly simplify rdma code as Dan
> > mentioned?
> I'm rather not familiar with QEMU migration yet,  from the test
> result, I think it's a possible direction,
> just we need to at least based on a rather recent release like
> rdma-core v33 with proper 'fork' support.
> 
> Maybe Dan or you could give more detail about what you have in mind
> for using rsocket as a replacement for the future.
> We will also look into the implementation details in the meantime.

The migration/socket.c file is the entrypoint for traditional TCP
based migration code. It uses the QIOChannelSocket class which is
written against the traditional sockets APIs, and uses the QAPI
SocketAddress data type to configure it..

My thought was that potentially SocketAddress could be extended to
offer RDMA addressing eg


{ 'union': 'SocketAddress',
  'base': { 'type': 'SocketAddressType' },
  'discriminator': 'type',
  'data': { 'inet': 'InetSocketAddress',
'unix': 'UnixSocketAddress',
'vsock': 'VsockSocketAddress',
'fd': 'FdSocketAddress',
'rdma': 'InetSocketAddress' } }

And then QIOChannelSocket could be also extended to call the
alternative 'rsockets' APIs where needed. That would mean that
existing sockets migration code would almost "just work" with
RDMA. Theoreticaly any other part of QEMU using QIOChannelSocket
would also then magically support RDMA too, with very little (if
any) extra work.

With 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: [PATCH v4 04/12] vhost-user-server: do not set memory fd non-blocking

[Daniel P . Berrangé]

On Wed, May 08, 2024 at 09:44:48AM +0200, Stefano Garzarella wrote:
> In vhost-user-server we set all fd received from the other peer
> in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.)
> it's not really needed, because we don't use these fd with blocking
> operations, but only to map memory.
> 
> In addition, in some systems this operation can fail (e.g. in macOS
> setting an fd returned by shm_open() non-blocking fails with errno
> = ENOTTY).
> 
> So, let's avoid setting fd non-blocking for those messages that we
> know carry memory fd (e.g. VHOST_USER_ADD_MEM_REG,
> VHOST_USER_SET_MEM_TABLE).
> 
> Signed-off-by: Stefano Garzarella 
> ---
> v3:
> - avoiding setting fd non-blocking for messages where we have memory fd
>   (Eric)
> ---
>  util/vhost-user-server.c | 12 ++++
>  1 file changed, 12 insertions(+)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 02/12] libvhost-user: fail vu_message_write() if sendmsg() is failing

[Daniel P . Berrangé]

On Wed, May 08, 2024 at 09:44:46AM +0200, Stefano Garzarella wrote:
> In vu_message_write() we use sendmsg() to send the message header,
> then a write() to send the payload.
> 
> If sendmsg() fails we should avoid sending the payload, since we
> were unable to send the header.
> 
> Discovered before fixing the issue with the previous patch, where
> sendmsg() failed on macOS due to wrong parameters, but the frontend
> still sent the payload which the backend incorrectly interpreted
> as a wrong header.
> 
> Signed-off-by: Stefano Garzarella 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v4 01/12] libvhost-user: set msg.msg_control to NULL when it is empty

[Daniel P . Berrangé]

On Wed, May 08, 2024 at 09:44:45AM +0200, Stefano Garzarella wrote:
> On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
> the `struct msghdr` has the field `msg_controllen` set to 0, but
> `msg_control` is not NULL.
> 
> Reviewed-by: Eric Blake 
> Reviewed-by: David Hildenbrand 
> Reviewed-by: Philippe Mathieu-Daud?? 

Philippe's name has got mangled here

> Signed-off-by: Stefano Garzarella 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c 
> b/subprojects/libvhost-user/libvhost-user.c
> index a879149fef..22bea0c775 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg 
> *vmsg)
>  memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
>  } else {
>  msg.msg_controllen = 0;
> +msg.msg_control = NULL;
>  }
>  
>  do {
> -- 
> 2.45.0
> 

With 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: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Daniel P . Berrangé]

On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote:
> What I worry more is whether this is really what we want to keep rdma in
> qemu, and that's also why I was trying to request for some serious
> performance measurements comparing rdma v.s. nics.  And here when I said
> "we" I mean both QEMU community and any company that will support keeping
> rdma around.
> 
> The problem is if NICs now are fast enough to perform at least equally
> against rdma, and if it has a lower cost of overall maintenance, does it
> mean that rdma migration will only be used by whoever wants to keep them in
> the products and existed already?  In that case we should simply ask new
> users to stick with tcp, and rdma users should only drop but not increase.
> 
> It seems also destined that most new migration features will not support
> rdma: see how much we drop old features in migration now (which rdma
> _might_ still leverage, but maybe not), and how much we add mostly multifd
> relevant which will probably not apply to rdma at all.  So in general what
> I am worrying is a both-loss condition, if the company might be easier to
> either stick with an old qemu (depending on whether other new features are
> requested to be used besides RDMA alone), or do periodic rebase with RDMA
> downstream only.

I don't know much about the originals of RDMA support in QEMU and why
this particular design was taken. It is indeed a huge maint burden to
have a completely different code flow for RDMA with 4000+ lines of
custom protocol signalling which is barely understandable.

I would note that /usr/include/rdma/rsocket.h provides a higher level
API that is a 1-1 match of the normal kernel 'sockets' API. If we had
leveraged that, then QIOChannelSocket class and the QAPI SocketAddress
type could almost[1] trivially have supported RDMA. There would have
been almost no RDMA code required in the migration subsystem, and all
the modern features like compression, multifd, post-copy, etc would
"just work".

I guess the 'rsocket.h' shim may well limit some of the possible
performance gains, but it might still have been a better tradeoff
to have not quite so good peak performance, but with massively
less maint burden.

With regards,
Daniel

[1] "almost" trivially, because the poll() integration for rsockets
requires a bit more magic sauce since rsockets FDs are not
really FDs from the kernel's POV. Still, QIOCHannel likely can
abstract that probme.
-- 
|: 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: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Daniel P . Berrangé]

On Tue, Apr 30, 2024 at 09:15:03AM +0200, Markus Armbruster wrote:
> Peter Xu  writes:
> 
> > On Mon, Apr 29, 2024 at 08:08:10AM -0500, Michael Galaxy wrote:
> >> Hi All (and Peter),
> >
> > Hi, Michael,
> >
> >> 
> >> My name is Michael Galaxy (formerly Hines). Yes, I changed my last name
> >> (highly irregular for a male) and yes, that's my real last name:
> >> https://www.linkedin.com/in/mrgalaxy/)
> >> 
> >> I'm the original author of the RDMA implementation. I've been discussing
> >> with Yu Zhang for a little bit about potentially handing over 
> >> maintainership
> >> of the codebase to his team.
> >> 
> >> I simply have zero access to RoCE or Infiniband hardware at all,
> >> unfortunately. so I've never been able to run tests or use what I wrote at
> >> work, and as all of you know, if you don't have a way to test something,
> >> then you can't maintain it.
> >> 
> >> Yu Zhang put a (very kind) proposal forward to me to ask the community if
> >> they feel comfortable training his team to maintain the codebase (and run
> >> tests) while they learn about it.
> >
> > The "while learning" part is fine at least to me.  IMHO the "ownership" to
> > the code, or say, taking over the responsibility, may or may not need 100%
> > mastering the code base first.  There should still be some fundamental
> > confidence to work on the code though as a starting point, then it's about
> > serious use case to back this up, and careful testings while getting more
> > familiar with it.
> 
> How much experience we expect of maintainers depends on the subsystem
> and other circumstances.  The hard requirement isn't experience, it's
> trust.  See the recent attack on xz.
> 
> I do not mean to express any doubts whatsoever on Yu Zhang's integrity!
> I'm merely reminding y'all what's at stake.

I think we shouldn't overly obsess[1] about 'xz', because the overwhealmingly
common scenario is that volunteer maintainers are honest people. QEMU is
in a massively better peer review situation. With xz there was basically no
oversight of the new maintainer. With QEMU, we have oversight from 1000's
of people on the list, a huge pool of general maintainers, the specific
migration maintainers, and the release manager merging code.

With a lack of historical experiance with QEMU maintainership, I'd suggest
that new RDMA volunteers would start by adding themselves to the "MAINTAINERS"
file with only the 'Reviewer' classification. The main migration maintainers
would still handle pull requests, but wait for a R-b from one of the RMDA
volunteers. After some period of time the RDMA folks could graduate to full
maintainer status if the migration maintainers needed to reduce their load.
I suspect that might prove unneccesary though, given RDMA isn't an area of
code with a high turnover of patches.

With regards,
Daniel

[1] If we do want to obsess about something bad though, we should
look at our handling of binary blobs in the repo and tarballs.
ie the firmware binaries that all get built in an arbitrary
environment of their respective maintainer. If we need firmware
blobs in tree, we should strive to come up with a reprodicble
build environment that gives us byte-for-byte identical results,
so the blobs can be verified. This is rather a tangent from this
thread though :)
-- 
|: 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: [PATCH 00/12] misc: Remove sprintf() due to macOS deprecation

[Daniel P . Berrangé]

On Thu, Apr 11, 2024 at 11:38:41AM +0200, Philippe Mathieu-Daudé wrote:
> On 11/4/24 00:27, BALATON Zoltan wrote:
> > On Wed, 10 Apr 2024, Richard Henderson wrote:
> > > On 4/10/24 06:06, Philippe Mathieu-Daudé wrote:
> > > > Hi,
> > > > 
> > > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> > > > resulting in painful developper experience.
> > > 
> > > Is snprintf also deprecated?
> > > It might be easier to convert some of these fixed buffer cases that
> > > way, if allowed.
> > 
> > I had the same thought as some of these might also have performance
> > implications (although most of them are in rarely called places).
> 
> I thought GLib/GString was recommended for formatting (IIRC some
> previous discussion with Alex / Daniel), so I switched to this
> API for style, rather than thinking of performance. Anyway, I'll
> respin using sprintf() when the buffer size maths are already done.

There are places in QEMU where the strings end up living in a fixed
size struct fields, and those would be candidates for sticking with
snprint().

For stack allocated string buffers, it is preferrable to switch to
g_autofree + g_strdup_printf(), unless there's a compelling performance
reason to avoid allocation in a hot path IMHO.

With 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: [PATCH v2 04/13] tests: Update our CI to use CentOS Stream 9 instead of 8

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:06PM +0200, Thomas Huth wrote:
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
> Thus upgrade our CentOS Stream container to major version 9 now.

The second reason for doing this is that Centos Stream 8
will go EOL in about 1 month:

https://blog.centos.org/2023/04/end-dates-are-coming-for-centos-stream-8-and-centos-linux-7/

  "After May 31, 2024, CentOS Stream 8 will be archived
   and no further updates will be provided."

I'm seeking confirmation, but I suspect after that date we
will be unable to build centos8 containers, as the package
repos will likely be archived.

RHEL-8 and other derivatives (Alma Linux, Rocky Linux,
etc) remain actively supported by their respective vendors
/ communities. Only CentOS Stream EOLs.


This has implications for our CI on stable branches. It is
valid for our stable branches to continue targetting the
RHEL-8 family of distros, as a 2 year cutoff in our support
policy is evaluated at time of each given major release.

IOW, cherry-picking this change to switch to CentOS Stream
9 is possibly inappropriate for stable branches.

lcitool supports Alma Linux as target, so we could switch
stable branches to Alma Linux 8 if desired to keep CI
coverage of RHEL-8 family.

Thoughts ?

> 
> Signed-off-by: Thomas Huth 
> ---
>  .gitlab-ci.d/buildtest.yml| 16 -
>  .gitlab-ci.d/container-core.yml   |  4 +--
>  .../{centos8.docker => centos9.docker}| 34 +++
>  tests/lcitool/mappings.yml| 20 ---
>  tests/lcitool/refresh |  2 +-
>  tests/vm/centos   |  4 +--
>  6 files changed, 26 insertions(+), 54 deletions(-)
>  rename tests/docker/dockerfiles/{centos8.docker => centos9.docker} (82%)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index cfdff175c3..9f34c650d6 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -158,9 +158,9 @@ build-system-centos:
>  - .native_build_job_template
>  - .native_build_artifact_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>  CONFIGURE_ARGS: --disable-nettle --enable-gcrypt 
> --enable-vfio-user-server
>--enable-modules --enable-trace-backends=dtrace --enable-docs
>  TARGETS: ppc64-softmmu or1k-softmmu s390x-softmmu
> @@ -242,7 +242,7 @@ check-system-centos:
>  - job: build-system-centos
>artifacts: true
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>  MAKE_CHECK_ARGS: check
>  
>  avocado-system-centos:
> @@ -251,7 +251,7 @@ avocado-system-centos:
>  - job: build-system-centos
>artifacts: true
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>  MAKE_CHECK_ARGS: check-avocado
>  AVOCADO_TAGS: arch:ppc64 arch:or1k arch:s390x arch:x86_64 arch:rx
>arch:sh4 arch:nios2
> @@ -327,9 +327,9 @@ avocado-system-flaky:
>  build-tcg-disabled:
>extends: .native_build_job_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>script:
>  - mkdir build
>  - cd build
> @@ -651,9 +651,9 @@ build-tci:
>  build-without-defaults:
>extends: .native_build_job_template
>needs:
> -job: amd64-centos8-container
> +job: amd64-centos9-container
>variables:
> -IMAGE: centos8
> +IMAGE: centos9
>  CONFIGURE_ARGS:
>--without-default-devices
>--without-default-features
> diff --git a/.gitlab-ci.d/container-core.yml b/.gitlab-ci.d/container-core.yml
> index 08f8450fa1..5459447676 100644
> --- a/.gitlab-ci.d/container-core.yml
> +++ b/.gitlab-ci.d/container-core.yml
> @@ -1,10 +1,10 @@
>  include:
>- local: '/.gitlab-ci.d/container-template.yml'
>  
> -amd64-centos8-container:
> +amd64-centos9-container:
>extends: .container_job_template
>variables:
> -NAME: centos8
> +NAME: centos9
>  
>  amd64-fedora-container:
>extends: .container_job_template
> diff --git a/tests/docker/dockerfiles/centos8.docker 
> b/tests/docker/dockerfiles/centos9.docker
> similarity index 82%
> rename from tests/docker/dockerfiles/centos8.docker
> rename to tests/docker/dockerfiles/centos9.docker
> index ea618bf352..6cf47ce786 100644
> --- a/tests/docker/dockerfiles/centos8.docker
> +++ b/tests/docker/dockerfiles/centos9.docker
> @@ -1,15 +1,14 @@
>  # THIS FILE WAS AUTO-GENERATED
>  #
> -#  $ lcitool dockerfile --layers all centos-stream-8 qemu
> +#  $ lcitool dockerfile --layers all centos-stream-9 qemu
>  #
>  # https://gitlab.com/libvirt/libvirt-ci
>  
> -FROM quay.io/centos/centos:stream8
> +FROM quay.io/centos/centos:stream9
>  
>  RUN dnf distro-sync -y && \
>  

Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 09:40:11AM -0500, Eric Blake wrote:
> On Fri, Apr 12, 2024 at 03:24:11PM +0200, Thomas Huth wrote:
> > Since version 2.66, glib has useful URI parsing functions, too.
> > Use those instead of the QEMU-internal ones to be finally able
> > to get rid of the latter.
> > 
> > Signed-off-by: Thomas Huth 
> > ---
> >  block/gluster.c | 71 -
> >  1 file changed, 35 insertions(+), 36 deletions(-)
> > 
> > diff --git a/block/gluster.c b/block/gluster.c
> > index cc74af06dc..1c9505f8bb 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -17,7 +17,6 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qmp/qdict.h"
> >  #include "qapi/qmp/qerror.h"
> > -#include "qemu/uri.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> >  #include "qemu/option.h"
> > @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs)
> >  }
> >  }
> >  
> > -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
> > +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char 
> > *path)
> 
> Is it worth mentioning in the commit message that this includes a
> const-correctness tweak?
> 
> > @@ -364,57 +363,57 @@ static int 
> > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
> >  QAPI_LIST_PREPEND(gconf->server, gsconf);
> >  
> >  /* transport */
> > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> > +uri_scheme = g_uri_get_scheme(uri);
> > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) {
> 
> Pre-existing, but per RFC 3986, we should probably be using strcasecmp
> for scheme comparisons (I'm not sure if g_uri_parse guarantees a
> lower-case return, even when the user passed in upper case).  That can
> be a separate patch.

Docs say it is lowercase:

  https://developer-old.gnome.org/glib/stable/glib-URI-Functions.html

  "on return, contains the scheme (converted to lowercase), or NULL."

With 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: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:11PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Signed-off-by: Thomas Huth 
> ---
>  block/gluster.c | 71 -
>  1 file changed, 35 insertions(+), 36 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 08/13] Remove glib compatibility code that is not required anymore

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:10PM +0200, Thomas Huth wrote:
> Now that we bumped the minumum glib version to 2.66, we can drop
> the old code.
> 
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Thomas Huth 
> ---
>  qga/commands-posix-ssh.c |  8 
>  util/error-report.c  | 10 --
>  2 files changed, 18 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 07/13] Bump minimum glib version to v2.66

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:09PM +0200, Thomas Huth wrote:
> Now that we dropped support for CentOS 8 and Ubuntu 20.04, we can
> look into bumping the glib version to a new minimum for further
> clean-ups. According to repology.org, available versions are:
> 
>  CentOS Stream 9:   2.66.7
>  Debian 11: 2.66.8
>  Fedora 38: 2.74.1
>  Freebsd:   2.78.4
>  Homebrew:  2.80.0
>  Openbsd:   2.78.4
>  OpenSuse leap 15.5:2.70.5
>  pkgsrc_current:2.78.4
>  Ubuntu 22.04:  2.72.1
> 
> Thus it should be safe to bump the minimum glib version to 2.66 now.
> Version 2.66 comes with new functions for URI parsing which will
> allow further clean-ups in the following patches.
> 
> Signed-off-by: Thomas Huth 
> ---
>  meson.build  | 16 +---
>  include/glib-compat.h| 27 ++-
>  qga/commands-posix-ssh.c |  4 ++--
>  3 files changed, 5 insertions(+), 42 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 06/13] ci: move external build environment setups to CentOS Stream 9

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:08PM +0200, Thomas Huth wrote:
> From: Paolo Bonzini 
> 
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
> 
> Thus upgrade our CentOS Stream build environment playbooks to major
> version 9 now.
> 
> Signed-off-by: Paolo Bonzini 
> Reviewed-by: Thomas Huth 
> Message-ID: <20240412103708.27650-1-pbonz...@redhat.com>
> Signed-off-by: Thomas Huth 
> ---
>  .../stream/{8 => 9}/build-environment.yml | 31 ++---
>  .../stream/{8 => 9}/x86_64/configure  |  4 +-
>  .../stream/{8 => 9}/x86_64/test-avocado   |  0
>  scripts/ci/setup/build-environment.yml| 44 +++
>  4 files changed, 34 insertions(+), 45 deletions(-)
>  rename scripts/ci/org.centos/stream/{8 => 9}/build-environment.yml (75%)
>  rename scripts/ci/org.centos/stream/{8 => 9}/x86_64/configure (98%)
>  rename scripts/ci/org.centos/stream/{8 => 9}/x86_64/test-avocado (100%)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 05/13] .travis.yml: Update the jobs to Ubuntu 22.04

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:07PM +0200, Thomas Huth wrote:
> According to our support policy, we'll soon drop our official support
> for Ubuntu 20.04 ("Focal Fossa") in QEMU. Thus we should update the
> Travis jobs now to a newer release (Ubuntu 22.04 - "Jammy Jellyfish")
> for future testing. Since all jobs are using this release now, we
> can drop the entries from the individual jobs and use the global
> setting again.
> 
> Signed-off-by: Thomas Huth 
> ---
>  .travis.yml | 13 +++--
>  1 file changed, 3 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 04/13] tests: Update our CI to use CentOS Stream 9 instead of 8

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:06PM +0200, Thomas Huth wrote:
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active
> support for the previous major version 8 now.
> Thus upgrade our CentOS Stream container to major version 9 now.
> 
> Signed-off-by: Thomas Huth 
> ---
>  .gitlab-ci.d/buildtest.yml| 16 -
>  .gitlab-ci.d/container-core.yml   |  4 +--
>  .../{centos8.docker => centos9.docker}| 34 +++
>  tests/lcitool/mappings.yml| 20 ---
>  tests/lcitool/refresh |  2 +-
>  tests/vm/centos   |  4 +--
>  6 files changed, 26 insertions(+), 54 deletions(-)
>  rename tests/docker/dockerfiles/{centos8.docker => centos9.docker} (82%)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 03/13] tests/docker/dockerfiles: Run lcitool-refresh after the lcitool update

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:05PM +0200, Thomas Huth wrote:
> This update adds the removing of the EXTERNALLY-MANAGED marker files
> that has been added to the lcitool recently.

For those who don't know, python now commonly blocks the ability to
run 'pip install' outside of a venv. This generally makes sense for
a precious installation environment. Our containers are disposable
though, so a venv has no benefit. Removing the 'EXTERNALLY-MANAGED'
allows the historical arbitrary use of 'pip' outside a venv.
lcitool just does this unconditionally given the containers are
not precious.

> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/docker/dockerfiles/alpine.docker| 3 ++-
>  tests/docker/dockerfiles/centos8.docker   | 1 +
>  tests/docker/dockerfiles/debian-amd64-cross.docker| 3 ++-
>  tests/docker/dockerfiles/debian-arm64-cross.docker| 3 ++-
>  tests/docker/dockerfiles/debian-armel-cross.docker| 3 ++-
>  tests/docker/dockerfiles/debian-armhf-cross.docker| 3 ++-
>  tests/docker/dockerfiles/debian-i686-cross.docker | 3 ++-
>  tests/docker/dockerfiles/debian-mips64el-cross.docker | 3 ++-
>  tests/docker/dockerfiles/debian-mipsel-cross.docker   | 3 ++-
>  tests/docker/dockerfiles/debian-ppc64el-cross.docker  | 3 ++-
>  tests/docker/dockerfiles/debian-riscv64-cross.docker  | 3 ++-
>  tests/docker/dockerfiles/debian-s390x-cross.docker| 3 ++-
>  tests/docker/dockerfiles/debian.docker| 1 +
>  tests/docker/dockerfiles/fedora-win64-cross.docker| 3 ++-
>  tests/docker/dockerfiles/fedora.docker| 1 +
>  tests/docker/dockerfiles/opensuse-leap.docker | 1 +
>  tests/docker/dockerfiles/ubuntu2204.docker| 1 +
>  17 files changed, 29 insertions(+), 12 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH v2 02/13] tests/lcitool/libvirt-ci: Update to the latest master branch

[Daniel P . Berrangé]

On Fri, Apr 12, 2024 at 03:24:04PM +0200, Thomas Huth wrote:
> We need the latest fixes for the lcitool to be able to properly
> update our CentOS docker file to CentOS Stream 9.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/lcitool/libvirt-ci | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Daniel P . Berrangé]

On Fri, Mar 29, 2024 at 11:28:54AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Zhijian,
> 
> On 29/3/24 02:53, Zhijian Li (Fujitsu) wrote:
> > 
> > 
> > On 28/03/2024 23:01, Peter Xu wrote:
> > > On Thu, Mar 28, 2024 at 11:18:04AM -0300, Fabiano Rosas wrote:
> > > > Philippe Mathieu-Daudé  writes:
> > > > 
> > > > > The whole RDMA subsystem was deprecated in commit e9a54265f5
> > > > > ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> > > > > released in v8.2.
> > > > > 
> > > > > Remove:
> > > > >- RDMA handling from migration
> > > > >- dependencies on libibumad, libibverbs and librdmacm
> > > > > 
> > > > > Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> > > > > in old migration streams.
> > > > > 
> > > > > Cc: Peter Xu 
> > > > > Cc: Li Zhijian 
> > > > > Acked-by: Fabiano Rosas 
> > > > > Signed-off-by: Philippe Mathieu-Daudé 
> > > > 
> > > > Just to be clear, because people raised the point in the last version,
> > > > the first link in the deprecation commit links to a thread comprising
> > > > entirely of rdma migration patches. I don't see any ambiguity on whether
> > > > the deprecation was intended to include migration. There's even an ack
> > > > from Juan.
> > > 
> > > Yes I remember that's the plan.
> > > 
> > > > 
> > > > So on the basis of not reverting the previous maintainer's decision, my
> > > > Ack stands here.
> > > > 
> > > > We also had pretty obvious bugs ([1], [2]) in the past that would have
> > > > been caught if we had any kind of testing for the feature, so I can't
> > > > even say this thing works currently.
> > > > 
> > > > @Peter Xu, @Li Zhijian, what are your thoughts on this?
> > > 
> > > Generally I definitely agree with such a removal sooner or later, as 
> > > that's
> > > how deprecation works, and even after Juan's left I'm not aware of any
> > > other new RDMA users.  Personally, I'd slightly prefer postponing it one
> > > more release which might help a bit of our downstream maintenance, however
> > > I assume that's not a blocker either, as I think we can also manage it.
> > > 
> > > IMHO it's more important to know whether there are still users and whether
> > > they would still like to see it around. That's also one thing I notice 
> > > that
> > > e9a54265f533f didn't yet get acks from RDMA users that we are aware, even
> > > if they're rare. According to [2] it could be that such user may only rely
> > > on the release versions of QEMU when it broke things.
> > > 
> > > So I'm copying Yu too (while Zhijian is already in the loop), just in case
> > > someone would like to stand up and speak.
> > 
> > 
> > I admit RDMA migration was lack of testing(unit/CI test), which led to the 
> > a few
> > obvious bugs being noticed too late.
> > However I was a bit surprised when I saw the removal of the RDMA migration. 
> > I wasn't
> > aware that this feature has not been marked as deprecated(at least there is 
> > no
> > prompt to end-user).
> > 
> > 
> > > IMHO it's more important to know whether there are still users and whether
> > > they would still like to see it around.
> > 
> > Agree.
> > I didn't immediately express my opinion in V1 because I'm also consulting 
> > our
> > customers for this feature in the future.
> > 
> > Personally, I agree with Perter's idea that "I'd slightly prefer postponing 
> > it one
> > more release which might help a bit of our downstream maintenance"
> 
> Do you mind posting a deprecation patch to clarify the situation?

The key thing the first deprecation patch missed was that it failed
to issue a warning message when RDMA migration was actually used.

With 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: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib

[Daniel P . Berrangé]

On Thu, Mar 28, 2024 at 09:54:49AM -0500, Eric Blake wrote:
> On Thu, Mar 28, 2024 at 03:06:03PM +0100, Thomas Huth wrote:
> > Since version 2.66, glib has useful URI parsing functions, too.
> > Use those instead of the QEMU-internal ones to be finally able
> > to get rid of the latter. The g_uri_get_host() also takes care
> > of removing the square brackets from IPv6 addresses, so we can
> > drop that part of the QEMU code now, too.
> > 
> > Signed-off-by: Thomas Huth 
> > ---
> >  block/nbd.c | 66 ++---
> >  1 file changed, 38 insertions(+), 28 deletions(-)
> > 
> > diff --git a/block/nbd.c b/block/nbd.c
> > index ef05f7cdfd..95b507f872 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -31,7 +31,6 @@
> >  #include "qemu/osdep.h"
> >  
> >  #include "trace.h"
> > -#include "qemu/uri.h"
> >  #include "qemu/option.h"
> >  #include "qemu/cutils.h"
> >  #include "qemu/main-loop.h"
> > @@ -1514,30 +1513,34 @@ static void nbd_client_close(BlockDriverState *bs)
> >  
> >  static int nbd_parse_uri(const char *filename, QDict *options)
> >  {
> > -URI *uri;
> > +GUri *uri;
> 
> Is it worth using 'g_autoptr(GUri) uri = NULL;' here, to simplify cleanup 
> later?
> 
> >  const char *p;
> > -QueryParams *qp = NULL;
> > +GHashTable *qp = NULL;
> 
> Presumably would be easier if qp is also auto-free.
> 
> > +int qp_n;
> >  int ret = 0;
> >  bool is_unix;
> > +const char *uri_scheme, *uri_query, *uri_server;
> > +int uri_port;
> >  
> > -uri = uri_parse(filename);
> > +uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);
> 
> The glib API is fairly close to what we have in qemu, making this a
> nice switchover.
> 
> >  /* nbd[+tcp]://host[:port]/export */
> > -if (!uri->server) {
> > +if (!uri_server) {
> >  ret = -EINVAL;
> >  goto out;
> >  }
> >  
> > -/* strip braces from literal IPv6 address */
> > -if (uri->server[0] == '[') {
> > -host = qstring_from_substr(uri->server, 1,
> > -   strlen(uri->server) - 1);
> > -} else {
> > -host = qstring_from_str(uri->server);
> > -}
> > -
> >  qdict_put_str(options, "server.type", "inet");
> > -qdict_put(options, "server.host", host);
> > +qdict_put_str(options, "server.host", uri_server);
> >  
> > -port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
> > +port_str = g_strdup_printf("%d", uri_port != -1 ? uri_port
> > +: 
> > NBD_DEFAULT_PORT);
> >  qdict_put_str(options, "server.port", port_str);
> 
> If a user requests nbd://hostname:0/export, this now sets server.port
> to "0" instead of "10809".  Is that an intentional change?  No one
> actually passes an explicit ":0" port on purpose, but we do have to
> worry about malicious URIs.

Passing '0' will cause the kernel to allocate a random free
port, so that is potentially introducing new semantics ?


With 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: [PATCH-for-9.1] rdma: Remove RDMA subsystem and pvrdma device

[Daniel P . Berrangé]

On Thu, Mar 28, 2024 at 01:57:27PM +0100, Philippe Mathieu-Daudé wrote:
> On 28/3/24 10:06, Daniel P. Berrangé wrote:
> > On Wed, Mar 27, 2024 at 11:55:48AM +0100, Philippe Mathieu-Daudé wrote:
> > > The whole RDMA subsystem was deprecated in commit e9a54265f5
> > > ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> > > released in v8.2. Time to remove it.
> > > 
> > > Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> > > in old migration streams.
> > > 
> > > Remove the dependencies on libibumad and libibverbs.
> > 
> > > 
> > > Remove the generated vmw_pvrdma/ directory from linux-headers.
> > > 
> > > Remove RDMA handling from migration.
> > > 
> > > Remove RDMA handling in GlusterFS block driver.
> > 
> > The RDMA support in GlusterFS is completely opaque to QEMU.
> > All we have there is the CLI syntax to enable use of the
> > RDMA support inside libglusterfs. I'm not convinced that
> > the justification for deprecation (lack of maintanier)
> > applies to this scenario.
> 
> I'll quote commit 0552ff2465 from 2016 then:
> 
> block/gluster: deprecate rdma support
> 
> gluster volfile server fetch happens through unix and/or tcp,
> it doesn't support volfile fetch over rdma. The rdma code may
> actually mislead, so to make sure things do not break, for now
> we fallback to tcp when requested for rdma, with a warning.
> 
> If you are wondering how this worked all these days, its the gluster
> libgfapi code which handles anything other than unix transport as
> socket/tcp, sad but true.

Ok, that should have been mentioned in the commit message
then, and is another reason for removing each functional
area in a separate commit, with the relevant notes for each.

With 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: [PATCH-for-9.1] rdma: Remove RDMA subsystem and pvrdma device

[Daniel P . Berrangé]

On Wed, Mar 27, 2024 at 11:55:48AM +0100, Philippe Mathieu-Daudé wrote:
> The whole RDMA subsystem was deprecated in commit e9a54265f5
> ("hw/rdma: Deprecate the pvrdma device and the rdma subsystem")
> released in v8.2. Time to remove it.
> 
> Keep the RAM_SAVE_FLAG_HOOK definition since it might appears
> in old migration streams.
> 
> Remove the dependencies on libibumad and libibverbs.

> 
> Remove the generated vmw_pvrdma/ directory from linux-headers.
> 
> Remove RDMA handling from migration.
> 
> Remove RDMA handling in GlusterFS block driver.

The RDMA support in GlusterFS is completely opaque to QEMU.
All we have there is the CLI syntax to enable use of the
RDMA support inside libglusterfs. I'm not convinced that
the justification for deprecation (lack of maintanier)
applies to this scenario.

> Remove rdmacm-mux tool from contrib/.
> 
> Remove PVRDMA device.

I agree with Thomas that each functional area listed here
should be handld in a separate patch, since they're all
independant.

With 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: [PULL 1/1] coroutine: cap per-thread local pool size

[Daniel P . Berrangé]

Sending this PULL feels little rushed, as I still have
un-answered questions on the inital patch posting just
a few hours ago

On Tue, Mar 19, 2024 at 11:09:38AM -0400, Stefan Hajnoczi wrote:
> The coroutine pool implementation can hit the Linux vm.max_map_count
> limit, causing QEMU to abort with "failed to allocate memory for stack"
> or "failed to set up stack guard page" during coroutine creation.
> 
> This happens because per-thread pools can grow to tens of thousands of
> coroutines. Each coroutine causes 2 virtual memory areas to be created.
> Eventually vm.max_map_count is reached and memory-related syscalls fail.
> The per-thread pool sizes are non-uniform and depend on past coroutine
> usage in each thread, so it's possible for one thread to have a large
> pool while another thread's pool is empty.
> 
> Switch to a new coroutine pool implementation with a global pool that
> grows to a maximum number of coroutines and per-thread local pools that
> are capped at hardcoded small number of coroutines.
> 
> This approach does not leave large numbers of coroutines pooled in a
> thread that may not use them again. In order to perform well it
> amortizes the cost of global pool accesses by working in batches of
> coroutines instead of individual coroutines.
> 
> The global pool is a list. Threads donate batches of coroutines to when
> they have too many and take batches from when they have too few:
> 
> .---.
> | Batch 1 | Batch 2 | Batch 3 | ... | global_pool
> `---'
> 
> Each thread has up to 2 batches of coroutines:
> 
> .---.
> | Batch 1 | Batch 2 | per-thread local_pool (maximum 2 batches)
> `---'
> 
> The goal of this change is to reduce the excessive number of pooled
> coroutines that cause QEMU to abort when vm.max_map_count is reached
> without losing the performance of an adequately sized coroutine pool.
> 
> Here are virtio-blk disk I/O benchmark results:
> 
>   RW BLKSIZE IODEPTHOLDNEW CHANGE
> randread  4k   1 113725 117451 +3.3%
> randread  4k   8 192968 198510 +2.9%
> randread  4k  16 207138 209429 +1.1%
> randread  4k  32 212399 215145 +1.3%
> randread  4k  64 218319 221277 +1.4%
> randread128k   1  17587  17535 -0.3%
> randread128k   8  17614  17616 +0.0%
> randread128k  16  17608  17609 +0.0%
> randread128k  32  17552  17553 +0.0%
> randread128k  64  17484  17484 +0.0%
> 
> See files/{fio.sh,test.xml.j2} for the benchmark configuration:
> https://gitlab.com/stefanha/virt-playbooks/-/tree/coroutine-pool-fix-sizing
> 
> Buglink: https://issues.redhat.com/browse/RHEL-28947
> Reported-by: Sanjay Rao 
> Reported-by: Boaz Ben Shabat 
> Reported-by: Joe Mario 
> Reviewed-by: Kevin Wolf 
> Signed-off-by: Stefan Hajnoczi 
> Message-ID: <20240318183429.1039340-1-stefa...@redhat.com>
> ---
>  util/qemu-coroutine.c | 282 +-
>  1 file changed, 223 insertions(+), 59 deletions(-)
> 
> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> index 5fd2dbaf8b..2790959eaf 100644
> --- a/util/qemu-coroutine.c
> +++ b/util/qemu-coroutine.c
> @@ -18,39 +18,200 @@
>  #include "qemu/atomic.h"
>  #include "qemu/coroutine_int.h"
>  #include "qemu/coroutine-tls.h"
> +#include "qemu/cutils.h"
>  #include "block/aio.h"
>  
> -/**
> - * The minimal batch size is always 64, coroutines from the release_pool are
> - * reused as soon as there are 64 coroutines in it. The maximum pool size 
> starts
> - * with 64 and is increased on demand so that coroutines are not deleted 
> even if
> - * they are not immediately reused.
> - */
>  enum {
> -POOL_MIN_BATCH_SIZE = 64,
> -POOL_INITIAL_MAX_SIZE = 64,
> +COROUTINE_POOL_BATCH_MAX_SIZE = 128,
>  };
>  
> -/** Free list to speed up creation */
> -static QSLIST_HEAD(, Coroutine) release_pool = QSLIST_HEAD_INITIALIZER(pool);
> -static unsigned int pool_max_size = POOL_INITIAL_MAX_SIZE;
> -static unsigned int release_pool_size;
> +/*
> + * Coroutine creation and deletion is expensive so a pool of unused 
> coroutines
> + * is kept as a cache. When the pool has coroutines available, they are
> + * recycled instead of creating new ones from scratch. Coroutines are added 
> to
> + * the pool upon termination.
> + *
> + * The pool is global but each thread maintains a small local pool to avoid
> + * global pool contention. Threads fetch and return batches of coroutines 
> from
> + * the global pool to maintain their local pool. The local pool holds up to 
> two
> + * batches whereas the maximum size of the global pool is controlled by the
> + * qemu_coroutine_inc_pool_size() API.
> + *
> + * .---.
> + * | Batch 1 | Batch 2 | Batch 3 | ... | global_pool
> + * `---'
> + *
> + * .---.
> + * | Batch 1 | Batch 2 | per-thread local_pool (maximum 2 batches)
> + * `---

Re: [PATCH v3] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Fri, Mar 15, 2024 at 09:58:38AM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
> 
> Add a helper to detect if a device is a DM device. In raw_co_truncate()
> check if the block device is DM and resize it executing lvresize.
> 
> Signed-off-by: Alexander Ivanov 
> ---
>  block/file-posix.c | 61 ++
>  1 file changed, 61 insertions(+)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..af17a43fe9 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd, 
> int64_t offset,
>  return raw_thread_pool_submit(handle_aiocb_truncate, &acb);
>  }
>  
> +static bool device_is_dm(struct stat *st)
> +{
> +unsigned int maj, maj2;
> +char line[32], devname[16];
> +bool ret = false;
> +FILE *f;
> +
> +if (!S_ISBLK(st->st_mode)) {
> +return false;
> +}
> +
> +f = fopen("/proc/devices", "r");
> +if (!f) {
> +return false;
> +}
> +
> +maj = major(st->st_rdev);
> +
> +while (fgets(line, sizeof(line), f)) {
> +if (sscanf(line, "%u %15s", &maj2, devname) != 2) {
> +continue;
> +}
> +if (strcmp(devname, "device-mapper") == 0) {
> +ret = (maj == maj2);
> +break;
> +}
> +}
> +
> +fclose(f);
> +return ret;
> +}
> +
>  static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  bool exact, PreallocMode prealloc,
>  BdrvRequestFlags flags, Error **errp)
> @@ -2670,6 +2702,35 @@ static int coroutine_fn 
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>  int64_t cur_length = raw_getlength(bs);
>  
> +/*
> + * Try to resize an LVM device using LVM tools.
> + */
> +if (device_is_dm(&st) && offset > 0) {
> +int spawn_flags = G_SPAWN_SEARCH_PATH | 
> G_SPAWN_STDOUT_TO_DEV_NULL;
> +int status;
> +bool success;
> +char *err;
> +GError *gerr = NULL, *gerr_exit = NULL;
> +g_autofree char *size_str = g_strdup_printf("%" PRId64 "B", 
> offset);
> +const char *cmd[] = {"lvresize", "-f", "-L",
> + size_str, bs->filename, NULL};
> +
> +success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
> +   NULL, NULL, NULL, &err, &status, &gerr);
> +
> +if (success && g_spawn_check_exit_status(status, &gerr_exit)) {
> +return 0;
> +}
> +
> +if (success) {
> +error_setg(errp, "%s: %s", gerr_exit->message, err);
> +} else {
> +error_setg(errp, "lvresize execution error: %s", 
> gerr->message);
> +}
> +
> +return -EINVAL;
> +}
> +
>  if (offset != cur_length && exact) {
>  error_setg(errp, "Cannot resize device files");
>  return -ENOTSUP;
> -- 
> 2.40.1
> 
> 

With 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: [PATCH v2] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Thu, Mar 14, 2024 at 06:25:00PM +0100, Alexander Ivanov wrote:
> 
> 
> On 3/14/24 13:44, Daniel P. Berrangé wrote:
> > On Wed, Mar 13, 2024 at 11:43:27AM +0100, Alexander Ivanov wrote:
> > > If a block device is an LVM logical volume we can resize it using
> > > standard LVM tools.
> > > 
> > > Add a helper to detect if a device is a DM device. In raw_co_truncate()
> > > check if the block device is DM and resize it executing lvresize.
> > > 
> > > Signed-off-by: Alexander Ivanov 
> > > ---
> > >   block/file-posix.c | 61 ++
> > >   1 file changed, 61 insertions(+)
> > > 
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..5f07d98aa5 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd, 
> > > int64_t offset,
> > >   return raw_thread_pool_submit(handle_aiocb_truncate, &acb);
> > >   }
> > >   static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t 
> > > offset,
> > >   bool exact, PreallocMode 
> > > prealloc,
> > >   BdrvRequestFlags flags, Error 
> > > **errp)
> > > @@ -2670,6 +2702,35 @@ static int coroutine_fn 
> > > raw_co_truncate(BlockDriverState *bs, int64_t offset,
> > >   if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> > >   int64_t cur_length = raw_getlength(bs);
> > > +/*
> > > + * Try to resize an LVM device using LVM tools.
> > > + */
> > > +if (device_is_dm(&st) && offset > 0) {
> > > +int spawn_flags = G_SPAWN_SEARCH_PATH | 
> > > G_SPAWN_STDOUT_TO_DEV_NULL;
> > > +int status;
> > > +bool success;
> > > +char *err;
> > > +GError *gerr = NULL;
> > > +g_autofree char *size_str = g_strdup_printf("%ldB", offset);
> > offset is 64-bit, but '%ld' is not guaranteed to be 64-bit. I expect
> > this will break on 32-bit platforms. Try PRId64 instead.
> > 
> > > +const char *cmd[] = {"lvresize", "-f", "-L",
> > > + size_str, bs->filename, NULL};
> > > +
> > > +success = g_spawn_sync(NULL, (gchar **)cmd, NULL, 
> > > spawn_flags,
> > > +   NULL, NULL, NULL, &err, &status, 
> > > &gerr);
> > > +
> > > +if (success && WEXITSTATUS(status) == 0) {
> > > +return 0;
> > > +}
> > We should probably check  'g_spawn_check_wait_status' rather than
> > WEXITSTATUS, as this then gives us further eror message details
> > that
> Thank you.
> I think it would be better to use 'g_spawn_check_exit_status' because there
> is no
> 'g_spawn_check_wait_status' in glib before 2.70 and even in 2.78 it leads to
> 'g_spawn_check_wait_status is deprecated: Not available before 2.70' error.

Ah yes, well spotted.


With 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: [PATCH v2] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Wed, Mar 13, 2024 at 11:43:27AM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
> 
> Add a helper to detect if a device is a DM device. In raw_co_truncate()
> check if the block device is DM and resize it executing lvresize.
> 
> Signed-off-by: Alexander Ivanov 
> ---
>  block/file-posix.c | 61 ++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..5f07d98aa5 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd, 
> int64_t offset,
>  return raw_thread_pool_submit(handle_aiocb_truncate, &acb);
>  }

>  static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  bool exact, PreallocMode prealloc,
>  BdrvRequestFlags flags, Error **errp)
> @@ -2670,6 +2702,35 @@ static int coroutine_fn 
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>  int64_t cur_length = raw_getlength(bs);
>  
> +/*
> + * Try to resize an LVM device using LVM tools.
> + */
> +if (device_is_dm(&st) && offset > 0) {
> +int spawn_flags = G_SPAWN_SEARCH_PATH | 
> G_SPAWN_STDOUT_TO_DEV_NULL;
> +int status;
> +bool success;
> +char *err;
> +GError *gerr = NULL;
> +g_autofree char *size_str = g_strdup_printf("%ldB", offset);

offset is 64-bit, but '%ld' is not guaranteed to be 64-bit. I expect
this will break on 32-bit platforms. Try PRId64 instead.

> +const char *cmd[] = {"lvresize", "-f", "-L",
> + size_str, bs->filename, NULL};
> +
> +success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
> +   NULL, NULL, NULL, &err, &status, &gerr);
> +
> +if (success && WEXITSTATUS(status) == 0) {
> +return 0;
> +}

We should probably check  'g_spawn_check_wait_status' rather than
WEXITSTATUS, as this then gives us further eror message details
that

> +
> +if (!success) {
> +error_setg(errp, "lvresize execution error: %s", 
> gerr->message);
> +} else {
> +error_setg(errp, "%s", err);

...we would also include here, such as the exit code or terminal
signal.

> +}
> +
> +return -EINVAL;
> +}
> +
>  if (offset != cur_length && exact) {
>  error_setg(errp, "Cannot resize device files");
>  return -ENOTSUP;
> -- 
> 2.40.1
> 
> 

With 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: [PATCH] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Tue, Mar 12, 2024 at 06:04:26PM +0100, Alexander Ivanov wrote:
> Thank you for the review.
> 
> On 3/11/24 19:24, Daniel P. Berrangé wrote:
> > On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
> > > If a block device is an LVM logical volume we can resize it using
> > > standard LVM tools.
> > > 
> > > In raw_co_truncate() check if the block device is a LV using lvdisplay
> > > and resize it executing lvresize.
> > > 
> > > Signed-off-by: Alexander Ivanov 
> > > ---
> > >   block/file-posix.c | 27 +++
> > >   1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..cf8a04e6f7 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -2670,6 +2670,33 @@ static int coroutine_fn 
> > > raw_co_truncate(BlockDriverState *bs, int64_t offset,
> > >   if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
> > >   int64_t cur_length = raw_getlength(bs);
> > > +/*
> > > + * Check if the device is an LVM logical volume and try to resize
> > > + * it using LVM tools.
> > > + */
> > > +if (S_ISBLK(st.st_mode) && offset > 0) {
> > > +char cmd[PATH_MAX + 32];
> > > +
> > > +snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
> > > + bs->filename);
> > PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
> > for dynamic allocation, along with g_autofree for release.
> > 
> > > +ret = system(cmd);
> > IMHO using 'system' for spawning processes is dubious in any non-trivial
> > program.
> > 
> > Historically at least it does not have well defined behaviour wrt signal
> > handling in the child, before execve is called. ie potentially a signal
> > handler registered by QEMU in the parent could run in the child having
> > ill-effects.
> > 
> > 'system' also executes via the shell which opens up many risks with
> > unsanitized files path being substituted into the command line.
> > > +if (ret != 0) {
> > > +error_setg(errp, "lvdisplay returned %d error for '%s'",
> > > +   ret, bs->filename);
> > > +return ret;
> > > +}
> > Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
> > going to report errors if it isn't an LVM device.
> The problem is that we don't know if 'lvresize' returned an error because of
> non-LVM device or there was another reason. For the first variant we should
> continue original code execution, for the second - return an error.
> > 
> > If we want to detect an LVM device though, couldn't we lookup
> > 'device-mapper'  in /proc/devices and then major the device major
> > node number.
> It will require more code for getting device major, file reading and
> parsing.
> Why this way is better?

There are a variety of reasons why these LVM commands could fail.
Directly detecting whether or not this is an LVM device, rather
than inferring it from 'lvdisplay' result is a more precise check
which will allow for clearer error reporting IMHO.


Ideally we wouldn't even spawn a process for the resize operation,
but instead call into the device mapper library API. That would
let it work even when the seccomp sandbox is enabled and blocking
process spawning. IME the device mapper API is not too friendly
though, so I didn't want to suggest that as a blocker.

> > > +snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > 
> > > /dev/null",
> > > + offset, bs->filename);
> > > +ret = system(cmd);
> > > +if (ret != 0) {
> > > +error_setg(errp, "lvresize returned %d error for '%s'",
> > > +   ret, bs->filename);
> > lvresize might display an message on stderr on failure but that's at best
> > going to QEMU's stderr. Any error needs to be captured and put into
> > this error message that's fed back to the user / QMP client.
> It seems I need to implement a high level function for programs execution.
> Looked at
> g_spawn_sync(), but it uses fork() under the hood and we could have a
> performance
> issue. Haven't found a high level function that uses vfork() and allows to
> catch stderr.

GLib should be using posix_spawn internally on all modern platforms.

Resizing LVM volumes does not sound like a hot code path, so I'm
seeing what performance concerns we would have.

With 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: [PATCH] block: Use LVM tools for LV block device truncation

[Daniel P . Berrangé]

On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
> If a block device is an LVM logical volume we can resize it using
> standard LVM tools.
> 
> In raw_co_truncate() check if the block device is a LV using lvdisplay
> and resize it executing lvresize.
> 
> Signed-off-by: Alexander Ivanov 
> ---
>  block/file-posix.c | 27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..cf8a04e6f7 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2670,6 +2670,33 @@ static int coroutine_fn 
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>  int64_t cur_length = raw_getlength(bs);
>  
> +/*
> + * Check if the device is an LVM logical volume and try to resize
> + * it using LVM tools.
> + */
> +if (S_ISBLK(st.st_mode) && offset > 0) {
> +char cmd[PATH_MAX + 32];
> +
> +snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
> + bs->filename);

PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
for dynamic allocation, along with g_autofree for release.

> +ret = system(cmd);

IMHO using 'system' for spawning processes is dubious in any non-trivial
program.

Historically at least it does not have well defined behaviour wrt signal
handling in the child, before execve is called. ie potentially a signal
handler registered by QEMU in the parent could run in the child having
ill-effects.

'system' also executes via the shell which opens up many risks with
unsanitized files path being substituted into the command line.

> +if (ret != 0) {
> +error_setg(errp, "lvdisplay returned %d error for '%s'",
> +   ret, bs->filename);
> +return ret;
> +}

Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
going to report errors if it isn't an LVM device.

If we want to detect an LVM device though, couldn't we lookup
'device-mapper'  in /proc/devices and then major the device major
node number.

> +
> +snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null",
> + offset, bs->filename);
> +ret = system(cmd);
> +if (ret != 0) {
> +error_setg(errp, "lvresize returned %d error for '%s'",
> +   ret, bs->filename);

lvresize might display an message on stderr on failure but that's at best
going to QEMU's stderr. Any error needs to be captured and put into
this error message that's fed back to the user / QMP client.

> +}
> +
> +return ret;
> +}
> +

With 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: [PATCH 9/9] hostmem-file: support POSIX shm_open()

[Daniel P . Berrangé]

On Wed, Feb 28, 2024 at 12:47:59PM +0100, Stefano Garzarella wrote:
> Add a new `shm` bool option for `-object memory-backend-file`.
> 
> When this option is set to true, the POSIX shm_open(3) is used instead
> of open(2).
> 
> So a file will not be created in the filesystem, but a "POSIX shared
> memory object" will be instantiated. In Linux this turns into a file
> in /dev/shm, but in other OSes this may not happen (for example in
> macOS or FreeBSD nothing is shown in any filesystem).
> 
> This new feature is useful when we need to share guest memory with
> another process (e.g. vhost-user backend), but we don't have
> memfd_create() or any special filesystems (e.g. /dev/shm) available
> as in macOS.
> 
> Signed-off-by: Stefano Garzarella 
> ---
> I am not sure this is the best way to support shm_open() in QEMU.
> 
> Other solutions I had in mind were:
> 
> - create a new memory-backend-shm
> 
> - extend memory-backend-memfd to use shm_open() on systems where memfd is
> not available (problem: shm_open wants a name to assign to the object, but
> we can do a workaround by using a random name and do the unlink right away)

IMHO, create a new memory-backend-shm, don't overload memory-backend-memfd,
as this lets users choose between shm & memfd, even on Linux.

With 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: [PATCH 04/28] qemu-img: global option processing and error printing

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> In order to correctly print executable name in various
> error messages, pass argv[0] to error_exit() function.
> This way, error messages will refer to actual executable
> name, which may be different from 'qemu-img'.
> 
> For subcommands, pass whole argv[] array, so argv[0] is
> the executable name, not subcommand name.  In order to
> do that, avoid resetting optind but continue with the
> next option.  Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
> 
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
> 
> Introduce tryhelp() function which just prints
> 
>  try 'command-name --help' for more info
> 
> and exits.  When tryhelp() is called from within a subcommand
> handler, the message will look like:
> 
>  try 'command-name subcommand --help' for more info
> 
> qemu-img uses getopt_long() with ':' as the first char in
> optstring parameter, which means it doesn't print error
> messages but return ':' or '?' instead, and qemu-img uses
> unrecognized_option() or missing_argument() function to
> print error messages.  But it doesn't quite work:
> 
>  $ ./qemu-img -xx
>  qemu-img: unrecognized option './qemu-img'
> 
> so the aim is to let getopt_long() to print regular error
> messages instead (removing ':' prefix from optstring) and
> remove handling of '?' and ':' "options" entirely.  With
> concatenated argv[0] and the subcommand, it all finally
> does the right thing in all cases.  This will be done in
> subsequent changes command by command, with main() done
> last.
> 
> unrecognized_option() and missing_argument() functions
> prototypes aren't changed by this patch, since they're
> called from many places and will be removed a few patches
> later.  Only artifical "qemu-img" argv0 is provided in
> there for now.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 75 +++---
>  1 file changed, 38 insertions(+), 37 deletions(-)

I'm not sure how, but this change seems to have broken the iotests.
Just one example:

$ (cd  tests/qemu-iotests/ && ./check -qcow2 249)
QEMU  -- "/var/home/berrange/src/virt/qemu/build/qemu-system-x86_64" 
-nodefaults -display none -accel qtest
QEMU_IMG  -- "/var/home/berrange/src/virt/qemu/build/qemu-img" 
QEMU_IO   -- "/var/home/berrange/src/virt/qemu/build/qemu-io" --cache 
writeback --aio threads -f qcow2
QEMU_NBD  -- "/var/home/berrange/src/virt/qemu/build/qemu-nbd" 
IMGFMT-- qcow2
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 toolbox 6.6.12-200.fc39.x86_64
TEST_DIR  -- 
/var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/qemu-iotests-0t8h94bu
GDB_OPTIONS   -- 
VALGRIND_QEMU -- 
PRINT_QEMU_OUTPUT -- 

249   fail   [15:39:25] [15:39:25]   0.2s   (last: 0.4s)  failed, exit 
status 1
--- /var/home/berrange/src/virt/qemu/tests/qemu-iotests/249.out
+++ 
/var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch/qcow2-file-249/249.out.bad
@@ -1,47 +1,7 @@
 QA output created by 249
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
-{ 'execute': 'qmp_capabilities' }
-{"return": {}}

...snip

-*** done
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E 
suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E 
suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+Timeout waiting for capabilities on handle 0
Failures: 249
Failed 1 of 1 iotests



With 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: [PATCH 28/28] qemu-img: extend cvtnum() and use it in more places

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:16:09AM +0300, Michael Tokarev wrote:
> cvtnum() expects input string to specify some sort of size
> (optionally with KMG... suffix).  However, there are a lot
> of other number conversions in there (using qemu_strtol &Co),
> also, not all conversions which use cvtnum, actually expects
> size, - like dd count=nn.
> 
> Add bool issize argument to cvtnum() to specify if it should
> treat the argument as a size or something else, - this changes
> conversion routine in use and error text.
> 
> Use the new cvtnum() in more places (like where strtol were used),
> since it never return negative number in successful conversion.
> When it makes sense, also specify upper or lower bounds at the
> same time.  This simplifies option processing in multiple places,
> removing the need of local temporary variables and longer error
> reporting code.
> 
> While at it, fix errors, like depth in measure must be >= 1,
> while the previous code allowed it to be 0.
> 
> In a few places, change unsigned variables (like of type size_t)
> to be signed instead, - to avoid the need of temporary conversion
> variable.  All these variables are okay to be signed, we never
> assign <0 value to them except of the cases of conversion error,
> where we return immediately.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 118 ---------
>  1 file changed, 44 insertions(+), 74 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 27/28] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:16:08AM +0300, Michael Tokarev wrote:
> also add short description to each command and use it in --help
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 41 ++---
>  1 file changed, 34 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 19/28] qemu-img: resize: do not always eat last argument

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:16:00AM +0300, Michael Tokarev wrote:
> 'qemu-img resize --help' does not work, since it wants more
> arguments.  Also it -size is only recognized as a very last
> argument, but it is common for tools to handle other options
> after positional arguments too.
> 
> Tell getopt_long() to return non-options together with options,
> and process filename and size in the loop, and check if there's
> an argument right after filename which looks like -N (number),
> and treat it as size (decrement).  This way we can handle --help,
> and we can also have options after filename and size, and `--'
> will be handled fine too.
> 
> The only case which is not handled right is when there's an option
> between filename and size, and size is given as decrement, - in
> this case -size will be treated as option, not as size.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 41 +++--
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2a4bff2872..c8b0b68d67 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4296,7 +4296,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  {
>  Error *err = NULL;
>  int c, ret, relative;
> -const char *filename, *fmt, *size;
> +const char *filename = NULL, *fmt = NULL, *size = NULL;
>  int64_t n, total_size, current_size;
>  bool quiet = false;
>  BlockBackend *blk = NULL;
> @@ -4319,17 +4319,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  bool image_opts = false;
>  bool shrink = false;
>  
> -/* Remove size from argv manually so that negative numbers are not 
> treated
> - * as options by getopt. */
> -if (argc < 3) {
> -error_exit(argv[0], "Not enough arguments");
> -return 1;
> -}
> -
> -size = argv[--argc];
> -
>  /* Parse getopt arguments */
> -fmt = NULL;
>  for(;;) {
>  static const struct option long_options[] = {
>  {"help", no_argument, 0, 'h'},
> @@ -4339,7 +4329,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  {"shrink", no_argument, 0, OPTION_SHRINK},
>  {0, 0, 0, 0}
>  };
> -c = getopt_long(argc, argv, ":f:hq",
> +c = getopt_long(argc, argv, "-:f:hq",

In other patches you removed the initial ':' from gopt_long arg strings.

>  long_options, NULL);
>  if (c == -1) {
>  break;
> @@ -4377,12 +4367,35 @@ static int img_resize(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  case OPTION_SHRINK:
>  shrink = true;
>  break;
> +case 1: /* a non-optional argument */
> +if (!filename) {
> +filename = optarg;
> +/* see if we have -size (number) next to filename */
> +if (optind < argc) {
> +size = argv[optind];
> +if (size[0] == '-' && size[1] >= '0' && size[1] <= '9') {
> +++optind;
> +} else {
> +size = NULL;
> +}
> +}
> +} else if (!size) {
> +size = optarg;
> +} else {
> +error_exit(argv[0], "Extra argument(s) in command line");
> +}
> +break;

Can you say what scenario exercises this code 'case 1' ?  I couldn't
get it to run in any scenarios i tried, and ineed removing this,
and removing the 'getopt_long' change, I could still run  'qemu-img resize 
--help'
OK, and also run 'qemu-img resize foo -43' too.

>  }
>  }
> -if (optind != argc - 1) {
> +if (!filename && optind < argc) {
> +filename = argv[optind++];
> +}
> +if (!size && optind < argc) {
> +size = argv[optind++];
> +}
> +if (!filename || !size || optind < argc) {
>  error_exit(argv[0], "Expecting image file name and size");
>  }
> -filename = argv[optind++];
>  
>  /* Choose grow, shrink, or absolute resize mode */
>  switch (size[0]) {
> -- 
> 2.39.2
> 
> 

With 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: [PATCH 26/28] qemu-img: implement short --help, remove global help() function

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:16:07AM +0300, Michael Tokarev wrote:
> now once all individual subcommands has --help support, remove
> the large unreadable help() thing and replace it with small
> global --help, which refers to individual command --help for
> more info.
> 
> While at it, also line-wrap list of formats after 75 chars.
> 
> Since missing_argument() and unrecognized_option() are now unused,
> remove them.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 172 -
>  1 file changed, 39 insertions(+), 133 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 16/28] qemu-img: snapshot: make -l (list) the default, simplify option handling

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:57AM +0300, Michael Tokarev wrote:
> When no -l/-a/-c/-d specified, assume -l (list).
> 
> Use the same values for SNAPSHOT_LIST/etc constants as the
> option chars (lacd), this makes it possible to simplify
> option handling a lot, combining cases for 4 options into
> one.
> 
> Also remove bdrv_oflags handling (only list can use RO mode).
> 
> Signed-off-by: Michael Tokarev 
> ---
>  docs/tools/qemu-img.rst |  2 +-
>  qemu-img.c  | 52 ++---
>  2 files changed, 19 insertions(+), 35 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 06/28] qemu-img: create: refresh options/--help

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:47AM +0300, Michael Tokarev wrote:
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
> 
> Add missing long options (eg --format) in img_create().
> 
> Remove usage of missing_argument()/unrecognized_option() in
> img_create().
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 68 +++---
>  1 file changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 38ac0f1845..7e4c993b9c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -132,6 +132,31 @@ void unrecognized_option(const char *option)
>  error_exit("qemu-img", "unrecognized option '%s'", option);
>  }
>  
> +/*
> + * Print --help output for a command and exit.
> + * syntax and description are multi-line with trailing EOL
> + * (to allow easy extending of the text)
> + * syntax has each subsequent line indented by 8 chars.
> + * desrciption is indented by 2 chars for argument on each own line,
> + * and with 5 chars for argument description (like -h arg below).
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> +  const char *syntax, const char *arguments)
> +{
> +printf(
> +"Usage:\n"
> +"  %s %s %s"

For the global help there's an extra '\n' after 'Usage'. It would be
good go be consistent in this between global and per-command help.

$ ./build/qemu-img --help
qemu-img version 8.2.50 (v8.2.0-1677-g81b20f4b55)
Copyright (c) 2003-2023 Fabrice Bellard and the QEMU Project developers
QEMU disk image utility.  Usage:

  qemu-img [standard options] COMMAND [--help | command options]
...snip...

vs

$ ./build/qemu-img info --help
Display information about image.  Usage:
  qemu-img info [-f FMT | --image-opts] [-b] [-U] [--object OBJDEF]
[--output human|json] FILENAME
...snip...


I wonder if we should repeat '[standard options]' for the
per-command help too ?


> +"\n"
> +"Arguments:\n"

In the global help you called it 'Standard options', so for
consistency lets use 'Options:' here too.

> +"  -h, --help\n"
> +" print this help and exit\n"
> +"%s\n",
> +   "qemu-img", ccmd->name,
> +   syntax, arguments);
> +exit(EXIT_SUCCESS);
> +}
> +
>  /* Please keep in synch with docs/tools/qemu-img.rst */
>  static G_NORETURN
>  void help(void)
> @@ -530,23 +555,48 @@ static int img_create(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  for(;;) {
>  static const struct option long_options[] = {
>  {"help", no_argument, 0, 'h'},
> +{"quiet", no_argument, 0, 'q'},
>  {"object", required_argument, 0, OPTION_OBJECT},
> +{"format", required_argument, 0, 'f'},
> +{"backing", required_argument, 0, 'b'},
> +{"backing-format", required_argument, 0, 'F'},
> +{"backing-unsafe", no_argument, 0, 'u'},
> +{"options", required_argument, 0, 'o'},
>  {0, 0, 0, 0}
>  };
> -c = getopt_long(argc, argv, ":F:b:f:ho:qu",
> +c = getopt_long(argc, argv, "F:b:f:ho:qu",
>  long_options, NULL);
>  if (c == -1) {
>  break;
>  }
>  switch(c) {
> -case ':':
> -missing_argument(argv[optind - 1]);
> -break;
> -case '?':
> -unrecognized_option(argv[optind - 1]);
> -break;
>  case 'h':
> -help();
> +cmd_help(ccmd,
> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"
> +,
> +"  -q, --quiet\n"
> +" quiet operations\n"
> +"  -f, --format FMT\n"
> +" specifies format of the new image, default is raw\n"
> +"  -o, --options FMT_OPTS\n"
> +" format-specific options ('-o list' for list)\n"
> +"  -b, --backing BACKING_FILENAME\n"
> +" stack new image on top of BACKING_FILENAME\n"
> +" (for formats which support stacking)\n"
> +"  -F, --backing-format BACKING_FMT\n"
> +" specify format of BACKING_FILENAME\n"
> +"  -u, --backing-unsafe\n"
> +" do not fail if BACKING_FMT can not be read\n"
> +"  --object OBJDEF\n"
> +" QEMU user-creatable object (eg encryption key)\n"
> +"  FILENAME\n"
> +" image file to create.  It will be overridden if exists\n"
> +"  SIZE\n"
> +" image size with optional suffix (multiplies in 1024)\n"
> +" SIZE is required unless BACKING_IMG is specified,\n"
> +" in which case it will be the same as size of BACKING_IMG\n"
> +);
>  break;
>  case 'F':
>  base_fmt = optarg;
> @@ -571,6 +621,8 @@ static int img_create(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  case OPTION_OBJECT:
>  user_creatable_process_cmdline(optarg);
>  break;
> +default:
> +tryhelp(argv[0]);
>  }
>  }
>  
> -- 
> 2.39.2
> 
> 

With reg

Re: [PATCH 05/28] qemu-img: pass current cmd info into command handlers

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:46AM +0300, Michael Tokarev wrote:
> This info will be used to generate --help output.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 34 +-
>  1 file changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 04/28] qemu-img: global option processing and error printing

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> In order to correctly print executable name in various
> error messages, pass argv[0] to error_exit() function.
> This way, error messages will refer to actual executable
> name, which may be different from 'qemu-img'.
> 
> For subcommands, pass whole argv[] array, so argv[0] is
> the executable name, not subcommand name.  In order to
> do that, avoid resetting optind but continue with the
> next option.  Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
> 
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
> 
> Introduce tryhelp() function which just prints
> 
>  try 'command-name --help' for more info
> 
> and exits.  When tryhelp() is called from within a subcommand
> handler, the message will look like:
> 
>  try 'command-name subcommand --help' for more info
> 
> qemu-img uses getopt_long() with ':' as the first char in
> optstring parameter, which means it doesn't print error
> messages but return ':' or '?' instead, and qemu-img uses
> unrecognized_option() or missing_argument() function to
> print error messages.  But it doesn't quite work:
> 
>  $ ./qemu-img -xx
>  qemu-img: unrecognized option './qemu-img'
> 
> so the aim is to let getopt_long() to print regular error
> messages instead (removing ':' prefix from optstring) and
> remove handling of '?' and ':' "options" entirely.  With
> concatenated argv[0] and the subcommand, it all finally
> does the right thing in all cases.  This will be done in
> subsequent changes command by command, with main() done
> last.
> 
> unrecognized_option() and missing_argument() functions
> prototypes aren't changed by this patch, since they're
> called from many places and will be removed a few patches
> later.  Only artifical "qemu-img" argv0 is provided in
> there for now.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 75 +++---
>  1 file changed, 38 insertions(+), 37 deletions(-)
> @@ -5602,10 +5602,11 @@ int main(int argc, char **argv)
>  /* find the command */
>  for (cmd = img_cmds; cmd->name != NULL; cmd++) {
>  if (!strcmp(cmdname, cmd->name)) {
> +argv[0] = g_strdup_printf("%s %s", argv[0], cmdname);
>  return cmd->handler(argc, argv);

This is going to result in valgrind warning that argv[0] is leaked.

How about:

  g_autofree char *cmdargv0 = g_strdup_printf("%s %s", argv[0], cmdname);
  argv[0] = cmdargv0;
  return cmd->handler(argc, argv);

>  }
>  }
>  
>  /* not found */
> -error_exit("Command not found: %s", cmdname);
> +error_exit(argv[0], "Command not found: %s", cmdname);
>  }
> -- 
> 2.39.2
> 
> 

With 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: [PATCH 02/28] qemu-img: measure: convert img_size to signed, simplify handling

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:43AM +0300, Michael Tokarev wrote:
> qemu_opt_set_number() expects signed int64_t.
> 
> Use int64_t instead of uint64_t for img_size, use -1 as "unset"
> value instead of UINT64_MAX, and do not require temporary sval
> for conversion from string.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 19 +++
>  1 file changed, 7 insertions(+), 12 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 03/28] qemu-img: create: convert img_size to signed, simplify handling

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:44AM +0300, Michael Tokarev wrote:
> Initializing an unsigned as -1, or using temporary
> sval for conversion is awkward.  Since we don't allow
> other "negative" values anyway, use signed value and
> pass it to bdrv_img_create() (where it is properly
> converted to unsigned), simplifying code.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 01/28] qemu-img: stop printing error twice in a few places

[Daniel P . Berrangé]

On Thu, Feb 22, 2024 at 12:15:42AM +0300, Michael Tokarev wrote:
> Currently we have:
> 
>   ./qemu-img resize none +10
>   qemu-img: Could not open 'none': Could not open 'none': No such file or 
> directory
> 
> stop printing the message twice, - local_err already has
> all the info, no need to prepend additional text there.
> 
> There are a few other places like this, but I'm unsure
> about these.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With 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: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include

[Daniel P . Berrangé]

On Wed, Feb 21, 2024 at 07:31:42PM +0300, Michael Tokarev wrote:
> 20.02.2024 21:48, Daniel P. Berrangé:
> 
> > This ends up looking a bit muddled together. I don't think we
> > need repeat 'qemu-img ' twice, and could add a little
> > more whitespace
> > 
> > eg instead of:
> > 
> > $ ./build/qemu-img check --help
> > qemu-img check: Check basic image integrity.  Usage:
> > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
> >  [--output human|json] [--object OBJDEF] FILENAME
> > Arguments:
> > ...snip...
> > 
> > have it look like
> > 
> > $ ./build/qemu-img check --help
> > Check basic image integrity.
> > 
> > Usage:
> > 
> >qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
> >  [--output human|json] [--object OBJDEF] FILENAME
> > 
> > Arguments:
> > ...snip...
> 
> Here's the current way how `create' help text looks like:
> 
> $ ./qemu-img create --help
> Create and format qemu image file.  Usage:
>   qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F 
> BACKING_FMT]]
> [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]
> Arguments:
>   -h, --help
>  print this help and exit
>   -q, --quiet
>  quiet operations
>   -f, --format FMT
>  specifies format of the new image, default is raw
>   -o, --options FMT_OPTS
>  format-specific options ('-o list' for list)
>   -b, --backing BACKING_FILENAME
>  stack new image on top of BACKING_FILENAME
>  (for formats which support stacking)
>   -F, --backing-format BACKING_FMT
>  specify format of BACKING_FILENAME
>   -u, --backing-unsafe
>  do not fail if BACKING_FMT can not be read
>   --object OBJDEF
>  QEMU user-creatable object (eg encryption key)
>   FILENAME
>  image file to create.  It will be overridden if exists
>   SIZE
>  image size with optional suffix (multiplies in 1024)
>  SIZE is required unless BACKING_IMG is specified,
>  in which case it will be the same as size of BACKING_IMG
> 
> Maybe it's a good idea to add newlines around the "syntax" part,
> ie, after "Usage:" and before "Arguments:".  I don't think it needs
> extra newlines between each argument description though, - this way
> it becomes just too long.
> 
> What do you think?

I still prefer to have more vertical whitespace, as I find it harder
to read through without it. I was using the typical man page option
/ usage formatting as a guide in my feedback.

Still, it would be useful to see what other maintainers think, as I'm
just one data point.

With 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: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include

[Daniel P . Berrangé]

On Tue, Feb 20, 2024 at 10:02:32PM +0300, Michael Tokarev wrote:
> 20.02.2024 21:48, Daniel P. Berrangé:
> ...
> > $ ./build/qemu-img check --help
> > Check basic image integrity.
> > 
> > Usage:
> > 
> >qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
> >  [--output human|json] [--object OBJDEF] FILENAME
> > 
> > Arguments:
> 
> $ ./build/qemu-img check --help
> Check basic image integrity.  Usage:
> 
>qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
>   [--output human|json] [--object OBJDEF] FILENAME
> 
> Arguments:
> ...
> 
> Or just:
> 
> Check basic image integrity:
> 
>  qemu-img check...
> 
> 
> In all cases I tried to make the whole thing as compact as possible,
> to (almost) fit on a standard terminal.  The extra empty lines between
> different arguments makes it almost impossible.

IMHO fitting on a "standard" terminal is OK in terms of width, but
should be a non-goal in terms of height. Readability is more important
than avoiding vertical scroll.

> > >  "Arguments:\n"
> > >  " -h|--help - print this help and exit\n"
> 
> btw, the common way is to use comma here, not "|", --
>   -h,--help - ...
> 
> Again, I especially omitted space after "|" to make it
> more compact.  Maybe for no good.

Yes, a comma with a space would look nicer. If we have the
description on the following line, then there's no width
limit problems there.

With 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: [PATCH 00/23] qemu-img: refersh options and --help handling

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:21AM +0300, Michael Tokarev wrote:
> Quite big patchset implementing normal, readable qemu-img --help
> (and qemu-img COMMAND --help) output with readable descriptions,
> and adding many long options in the process.
> 
> In the end I stopped using qemu-img-opts.hx in qemu-img.c, perhaps
> this can be avoided, with only list of commands and their desrciptions
> kept there, but I don't see big advantage here.  The same list should
> be included in docs/tools/qemu-img.rst, - this is not done now.

I think it'd be nice for qemu-img.c to be the canonical source
of truth, so we have the getopt short arg, long args, and
help all in the same place & thus much less likely to get
out of sync.  Thus I like the approach you've taken here
to stop using the .hx file.

As a later work, it wouldn't be too terrible to have a python
script that parses qemu-img.c to look for 'cmd_help(...)' calls
and extra the help text, which could be used to feed into the
qemu-img.rxt man page generation, thus fully eliminating the
.hx file.

> 
> Also each command syntax isn't reflected in the doc for now, because
> I want to give good names for options first, - and there, we've quite
> some inconsistences and questions.  For example, measure --output=OFMT
> -O OFMT, - this is priceless :)  I've no idea why we have this ugly
> --output=json thing, why not have --json? ;)  I gave the desired
> format long name --target-format to avoid clash with --output.
> 
> For rebase, src vs tgt probably should be renamed in local variables
> too, and I'm not even sure I've got the caches right. For caches,
> the thing is inconsistent across commands.
> 
> For compare, I used --a-format/--b-format (for -f/-F), - this can
> be made --souce-format and --target-format, to compare source (file1)
> with target (file2).
> 
> For bitmap, things are scary, I'm not sure what -b SRC_FILENAME
> really means, - for now I gave it --source option, but this does
> not make it more clear, suggestions welcome.
> 
> There are many other inconsistencies, I can't fix them all in one
> go.. :)

This is already a massive improvement on the status quo. My
comments were mostly around whitespace/layout tweaks to the
help output.


With 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: [PATCH 23/23] qemu-img: inline list of supported commands, remove qemu-img-cmds.h include

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:44AM +0300, Michael Tokarev wrote:
> also add short description to each command and use it in --help
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 42 +++---
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d9c5c6078b..e57076738e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -61,6 +61,7 @@
>  typedef struct img_cmd_t {
>  const char *name;
>  int (*handler)(const struct img_cmd_t *ccmd, int argc, char **argv);
> +const char *description;
>  } img_cmd_t;
>  
>  enum {
> @@ -130,11 +131,12 @@ static G_NORETURN
>  void cmd_help(const img_cmd_t *ccmd,
>const char *syntax, const char *arguments)
>  {
> -printf("qemu-img %s %s"
> +printf("qemu-img %s: %s.  Usage:\n"
> +   "qemu-img %s %s"

This ends up looking a bit muddled together. I don't think we
need repeat 'qemu-img ' twice, and could add a little
more whitespace

eg instead of:

$ ./build/qemu-img check --help
qemu-img check: Check basic image integrity.  Usage:
qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
[--output human|json] [--object OBJDEF] FILENAME
Arguments:
...snip...

have it look like

$ ./build/qemu-img check --help
Check basic image integrity.

Usage:

  qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u]
[--output human|json] [--object OBJDEF] FILENAME

Arguments:
...snip...


> "Arguments:\n"
> " -h|--help - print this help and exit\n"
> "%s",
> -   ccmd->name, syntax, arguments);
> +   ccmd->name, ccmd->description, ccmd->name, syntax, arguments);
>  exit(EXIT_SUCCESS);
>  }
>  
> @@ -5746,10 +5748,36 @@ out:
>  }
>  
>  static const img_cmd_t img_cmds[] = {
> -#define DEF(option, callback, arg_string)\
> -{ option, callback },
> -#include "qemu-img-cmds.h"
> -#undef DEF
> +{ "amend", img_amend,
> +  "Update format-specific options of the image" },
> +{ "bench", img_bench,
> +  "Run simple image benchmark" },
> +{ "bitmap", img_bitmap,
> +  "Perform modifications of the persistent bitmap in the image" },
> +{ "check", img_check,
> +  "Check basic image integrity" },
> +{ "commit", img_commit,
> +  "Commit image to its backing file" },
> +{ "compare", img_compare,
> +  "Check if two images have the same contents" },
> +{ "convert", img_convert,
> +  "Copy one image to another with optional format conversion" },
> +{ "create", img_create,
> +  "Create and format new image file" },
> +{ "dd", img_dd,
> +  "Copy input to output with optional format conversion" },
> +{ "info", img_info,
> +  "Display information about image" },
> +{ "map", img_map,
> +  "Dump image metadata" },
> +{ "measure", img_measure,
> +  "Calculate file size requred for a new image" },
> +{ "rebase", img_rebase,
> +  "Change backing file of the image" },
> +{ "resize", img_resize,
> +  "Resize the image to the new size" },
> +{ "snapshot", img_snapshot,
> +  "List or manipulate snapshots within image" },
>  { NULL, NULL, },
>  };
>  
> @@ -5813,7 +5841,7 @@ QEMU_IMG_VERSION
>  "   [[enable=]][,events=][,file=]\n"
>  "Recognized commands (run qemu-img command --help for command-specific 
> help):\n");
>  for (cmd = img_cmds; cmd->name != NULL; cmd++) {
> -printf("  %s\n", cmd->name);
> +printf("  %s - %s\n", cmd->name, cmd->description);
>  }
>  c = printf("Supported image formats:");
>  bdrv_iterate_format(format_print, &c, false);
> -- 
> 2.39.2
> 
> 

With 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: [PATCH 22/23] qemu-img: implement short --help, remove global help() function

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:43AM +0300, Michael Tokarev wrote:
> now once all individual subcommands has --help support, remove
> the large unreadable help() thing and replace it with small
> global --help, which refers to individual command --help for
> more info.
> 
> While at it, also line-wrap list of formats after 74 chars.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 148 +++--
>  1 file changed, 30 insertions(+), 118 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e2c8855ff5..d9c5c6078b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -94,11 +94,6 @@ typedef enum OutputFormat {
>  /* Default to cache=writeback as data integrity is not important for 
> qemu-img */
>  #define BDRV_DEFAULT_CACHE "writeback"
>  
> -static void format_print(void *opaque, const char *name)
> -{
> -printf(" %s", name);
> -}
> -
>  static G_NORETURN G_GNUC_PRINTF(2, 3)
>  void error_exit(const img_cmd_t *ccmd, const char *fmt, ...)
>  {
> @@ -154,114 +149,6 @@ static OutputFormat parse_output_format(const img_cmd_t 
> *ccmd, const char *arg)
>  }
>  }
>  
> -/* Please keep in synch with docs/tools/qemu-img.rst */
> -static G_NORETURN
> -void help(void)
> -{
> -const char *help_msg =
> -   QEMU_IMG_VERSION
> -   "usage: qemu-img [standard options] command [command options]\n"
> -   "QEMU disk image utility\n"
> -   "\n"
> -   "'-h', '--help'   display this help and exit\n"
> -   "'-V', '--version'output version information and exit\n"
> -   "'-T', '--trace'  
> [[enable=]][,events=][,file=]\n"
> -   " specify tracing options\n"
> -   "\n"
> -   "Command syntax:\n"
> -#define DEF(option, callback, arg_string)\
> -   "  " arg_string "\n"
> -#include "qemu-img-cmds.h"
> -#undef DEF
> -   "\n"
> -   "Command parameters:\n"
> -   "  'filename' is a disk image filename\n"
> -   "  'objectdef' is a QEMU user creatable object definition. See 
> the qemu(1)\n"
> -   "manual page for a description of the object properties. The 
> most common\n"
> -   "object type is a 'secret', which is used to supply passwords 
> and/or\n"
> -   "encryption keys.\n"
> -   "  'fmt' is the disk image format. It is guessed automatically in 
> most cases\n"
> -   "  'cache' is the cache mode used to write the output disk image, 
> the valid\n"
> -   "options are: 'none', 'writeback' (default, except for 
> convert), 'writethrough',\n"
> -   "'directsync' and 'unsafe' (default for convert)\n"
> -   "  'src_cache' is the cache mode used to read input disk images, 
> the valid\n"
> -   "options are the same as for the 'cache' option\n"
> -   "  'size' is the disk image size in bytes. Optional suffixes\n"
> -   "'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' 
> (gigabyte, 1024M),\n"
> -   "'T' (terabyte, 1024G), 'P' (petabyte, 1024T) and 'E' 
> (exabyte, 1024P)  are\n"
> -   "supported. 'b' is ignored.\n"
> -   "  'output_filename' is the destination disk image filename\n"
> -   "  'output_fmt' is the destination format\n"
> -   "  'options' is a comma separated list of format specific options 
> in a\n"
> -   "name=value format. Use -o help for an overview of the 
> options supported by\n"
> -   "the used format\n"
> -   "  'snapshot_param' is param used for internal snapshot, format\n"
> -   "is 'snapshot.id=[ID],snapshot.name=[NAME]', or\n"
> -   "'[ID_OR_NAME]'\n"
> -   "  '-c' indicates that target image must be compressed (qcow 
> format only)\n"
> -   "  '-u' allows unsafe backing chains. For rebasing, it is assumed 
> that old and\n"
> -   "   new backing file match exactly. The image doesn't need a 
> working\n"
> -   "   backing file before rebasing in this case (useful for 
> renaming the\n"
> -   "   backing file). For image creation, allow creating without 
> attempting\n"
> -   "   to open the backing file.\n"
> -   "  '-h' with or without a command shows this help and lists the 
> supported formats\n"
> -   "  '-p' show progress of command (only certain commands)\n"
> -   "  '-q' use Quiet mode - do not print any output (except 
> errors)\n"
> -   "  '-S' indicates the consecutive number of bytes (defaults to 
> 4k) that must\n"
> -   "   contain only zeros for qemu-img to create a sparse image 
> during\n"
> -   "   conversion. If the number of bytes is 0, the source will 
> not be scanned for\n"
> -   "   unallocated or zero sectors, and the destination image 
> will always be\n"
> -   "   fully allocated\n"
> -  

Re: [PATCH 02/23] qemu-img: refresh options/--help for "create" subcommand

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:23AM +0300, Michael Tokarev wrote:
> Add missing long options (eg --format).
> 
> Create helper function cmd_help() to display command-specific
> help text, and use it to print --help for 'create' subcommand.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 45 -
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 05f80b6e5b..7edfc56572 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -126,6 +126,25 @@ void unrecognized_option(const img_cmd_t *ccmd, const 
> char *option)
>  error_exit(ccmd, "unrecognized option '%s'", option);
>  }
>  
> +/*
> + * Print --help output for a command and exit.
> + * syntax and description are multi-line with trailing EOL
> + * (to allow easy extending of the text)
> + * syntax has each subsequent line starting with \t
> + * desrciption is indented by one char
> + */
> +static G_NORETURN
> +void cmd_help(const img_cmd_t *ccmd,
> +  const char *syntax, const char *arguments)
> +{
> +printf("qemu-img %s %s"

I think we want an extra "\n" before & after 'Arguments:'

> +   "Arguments:\n"
> +   " -h|--help - print this help and exit\n"
> +   "%s",
> +   ccmd->name, syntax, arguments);
> +exit(EXIT_SUCCESS);
> +}
> +
>  /* Please keep in synch with docs/tools/qemu-img.rst */
>  static G_NORETURN
>  void help(void)
> @@ -524,7 +543,13 @@ static int img_create(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  for(;;) {
>  static const struct option long_options[] = {
>  {"help", no_argument, 0, 'h'},
> +{"quiet", no_argument, 0, 'q'},
>  {"object", required_argument, 0, OPTION_OBJECT},
> +{"format", required_argument, 0, 'f'},
> +{"backing", required_argument, 0, 'b'},
> +{"backing-format", required_argument, 0, 'F'},
> +{"backing-unsafe", no_argument, 0, 'u'},
> +{"options", required_argument, 0, 'o'},
>  {0, 0, 0, 0}
>  };
>  c = getopt_long(argc, argv, ":F:b:f:ho:qu",
> @@ -540,7 +565,25 @@ static int img_create(const img_cmd_t *ccmd, int argc, 
> char **argv)
>  unrecognized_option(ccmd, argv[optind - 1]);
>  break;
>  case 'h':
> -help();
> +cmd_help(ccmd,
> +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n"
> +"[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n"
> +,
> +" -q|--quiet - quiet operations\n"
> +" -f|--format FMT - specifies format of the new image, default is raw\n"
> +" -o|--options FMT_OPTS - format-specific options ('-o list' for list)\n"
> +" -b|--backing BACKING_FILENAME - stack new image on top of 
> BACKING_FILENAME\n"
> +"  (for formats which support stacking)\n"
> +" -F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME\n"
> +" -u|--backing-unsafe - do not fail if BACKING_FMT can not be read\n"
> +" --object OBJDEF - QEMU user-creatable object (eg encryption key)\n"
> +" FILENAME - image file to create.  It will be overriden if exists\n"
> +" SIZE - image size with optional suffix: 'b' (byte, default), 'k' or\n"
> +"  'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),\n"
> +"  'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)\n"
> +"  SIZE is required unless BACKING_IMG is specified, in which case\n"
> +"  it will be the same as size of BACKING_IMG\n"

This comes out as a bit of a wall of dense text.

I think we should have 2 space indent for options, and a further
4 space for continuations, and also put the description on its
own line.

eg so instead of getting:

$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
[--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]
Arguments:
 -h|--help - print this help and exit
 -q|--quiet - quiet operations
 -f|--format FMT - specifies format of the new image, default is raw
 -o|--options FMT_OPTS - format-specific options ('-o list' for list)
 -b|--backing BACKING_FILENAME - stack new image on top of BACKING_FILENAME
  (for formats which support stacking)
 -F|--backing-format BACKING_FMT - specify format of BACKING_FILENAME
 -u|--backing-unsafe - do not fail if BACKING_FMT can not be read
 --object OBJDEF - QEMU user-creatable object (eg encryption key)
 FILENAME - image file to create.  It will be overriden if exists
 SIZE - image size with optional suffix: 'b' (byte, default), 'k' or
  'K' (kilobyte, 1024b), 'M' (megabyte, 1024K), 'G' (gigabyte, 1024M),
  'T' (terabyte, 1024G), 'P' (petabyte, 1024T), or 'E' (exabyte, 1024P)
  SIZE is required unless BACKING_IMG is specified, in which case
  it will be the same as size of BACKING_IMG


we would get:

$ ./build/qemu-img create --help
qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]
[--object OBJDEF] [-u] FILENAME [SIZE[b

Re: [PATCH 15/23] qemu-img: resize: do not always eat last argument

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:36AM +0300, Michael Tokarev wrote:
> 'qemu-img resize --help' does not work, since it wants more arguments.
> Only eat last option at the beginning if it starts like -N.., and allow
> getopt() to do its work, and eat it up at the end if not already eaten.
> This will not allow to mix options and size anyway, but it is better
> than now.
> 
> Signed-off-by: Michael Tokarev 
> ---
>  qemu-img.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 69d41e0a92..929a25a021 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4271,13 +4271,13 @@ static int img_resize(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  
>  /* Remove size from argv manually so that negative numbers are not 
> treated
>   * as options by getopt. */
> -if (argc < 3) {
> -error_exit(ccmd, "Not enough arguments");
> -return 1;
> +if (argc > 1 && argv[argc - 1][0] == '-'
> +&& argv[argc-1][1] >= '0' && argv[argc-1][1] <= '9') {
> +size = argv[--argc];
> +} else {
> +size = NULL;
>  }

We already have a variable 'int relative' that is set to '-1'
or '+1' depending on whether we have a -ve or +ve size.

I think it is clearer to follow if we just set 'relative' much
earlier before parsing by moving this chunk of code to before
the getopt:

switch (size[0]) {
case '+':
relative = 1;
size++;
break;
case '-':
relative = -1;
size++;
break;
default:
relative = 0;
break;
}

once we've done that we can simply replace the '-' with '+'
to stop getopt getting upset.

>  
> -size = argv[--argc];
> -
>  /* Parse getopt arguments */
>  fmt = NULL;
>  for(;;) {
> @@ -4329,10 +4329,13 @@ static int img_resize(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  break;
>  }
>  }
> -if (optind != argc - 1) {
> +if (optind + 1 + (size == NULL) != argc) {
>  error_exit(ccmd, "Expecting image file name and size");
>  }
>  filename = argv[optind++];
> +if (!size) {
> +size = argv[optind++];
> +}
>  
>  /* Choose grow, shrink, or absolute resize mode */
>  switch (size[0]) {

With 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: [PATCH 12/23] qemu-img: make -l (list) the default for "snapshot" subcommand

[Daniel P . Berrangé]

On Sat, Feb 10, 2024 at 12:22:33AM +0300, Michael Tokarev wrote:
> also remove bdrv_oflags handling (only list can use RO mode)
> ---
>  qemu-img.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)

I'd suggest docs/tools/qemu-img.rst should also be updated to say

 Lists all snapshots in the given image (default action)

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 1e09b78d00..d9dfff2f07 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3541,7 +3541,7 @@ static int img_snapshot(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  BlockDriverState *bs;
>  QEMUSnapshotInfo sn;
>  char *filename, *fmt = NULL, *snapshot_name = NULL;
> -int c, ret = 0, bdrv_oflags;
> +int c, ret = 0;
>  int action = 0;
>  bool quiet = false;
>  Error *err = NULL;
> @@ -3549,7 +3549,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  bool force_share = false;
>  int64_t rt;
>  
> -bdrv_oflags = BDRV_O_RDWR;
>  /* Parse commandline parameters */
>  for(;;) {
>  static const struct option long_options[] = {
> @@ -3583,7 +3582,6 @@ static int img_snapshot(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  return 0;
>  }
>  action = SNAPSHOT_LIST;
> -bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
>  break;
>  case 'a':
>  if (action) {
> @@ -3629,9 +3627,14 @@ static int img_snapshot(const img_cmd_t *ccmd, int 
> argc, char **argv)
>  }
>  filename = argv[optind++];
>  
> +if (!action) {
> +action = SNAPSHOT_LIST;
> +}
> +
>  /* Open the image */
> -blk = img_open(image_opts, filename, fmt, bdrv_oflags, false, quiet,
> -   force_share);
> +blk = img_open(image_opts, filename, fmt,
> +   action == SNAPSHOT_LIST ? 0 : BDRV_O_RDWR,
> +   false, quiet, force_share);
>  if (!blk) {
>  return 1;
>  }
> -- 
> 2.39.2
> 
> 

With 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 :|