Skip to contentSkip to conversation

Misc VNC, I/O, Crypto & checkpatch fixes

GIT PULL Applied Daniel P. Berrangé 6 participants 33 messages May 22, 2025 – Jul 10, 2025

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

The following changes since commit f0737158b483e7ec2b2512145aeab888b85cc1f7:

Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2025-05-20 10:26:30 -0400)

are available in the Git repository at:

https://gitlab.com/berrange/qemu tags/misc-next-pull-request

for you to fetch changes up to 81941aa896b718c194f08cebb4303561b64c60b4:

scripts/checkpatch.pl: mandate SPDX tag for Rust src files (2025-05-22 11:24:41 +0100)

----------------------------------------------------------------

* Fix VNC tight encoding with 8/16-bpp formats with mixed endian server/client
* Fix VNC non-tight encoding with mixed endian server/client
* Drop built-in AES impl from non-TCG usage, requiring nettle/gcrypt/gnutls
* Fix validation of SPDX-License-Identifier in new files
* Mandate SPDX-License-Identifier in Rust source
* Reject license boilerplate in new files
* Add full control over TCP keep alive setting for sockets

----------------------------------------------------------------

Daniel P. Berrangé (17):
ui/vnc.c: replace big endian flag with byte order value ui/vnc: take account of client byte order in pixman format ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats
tests: skip encrypted secret tests if AES is not available
tests: skip legacy qcow2 encryption test if AES is not available
tests: fix skipping cipher tests when AES is not available
crypto: fully drop built-in cipher provider
Revert "scripts: mandate that new files have SPDX-License-Identifier" scripts/checkpatch.pl: fix various indentation mistakes scripts/checkpatch: introduce tracking of file start/end scripts/checkpatch: use new hook for ACPI test data check scripts/checkpatch: use new hook for file permissions check scripts/checkpatch: expand pattern for matching makefiles scripts/checkpatch: use new hook for MAINTAINERS update check scripts/checkpatch: reimplement mandate for SPDX-License-Identifier scripts/checkpatch: reject license boilerplate on new files scripts/checkpatch.pl: mandate SPDX tag for Rust src files

Juraj Marcin (6):
io: Fix partial struct copy in qio_dns_resolver_lookup_sync_inet() util/qemu-sockets: Refactor setting client sockopts into a separate
function
util/qemu-sockets: Refactor success and failure paths in
inet_listen_saddr()
util/qemu-sockets: Add support for keep-alive flag to passive sockets util/qemu-sockets: Refactor inet_parse() to use QemuOpts util/qemu-sockets: Introduce inet socket options controlling TCP
keep-alive

 crypto/cipher-builtin.c.inc     | 303 ------------------------
 crypto/cipher-stub.c.inc        |  30 +++
 crypto/cipher.c                 |   2 +-
 include/ui/qemu-pixman.h        |   4 +-
 io/dns-resolver.c               |  21 +-
 meson.build                     |  30 +++
 qapi/sockets.json               |  23 +-
 scripts/checkpatch.pl           | 392 +++++++++++++++++++++-----------
 tests/unit/test-crypto-block.c  |   7 +
 tests/unit/test-crypto-cipher.c |  13 +-
 tests/unit/test-crypto-secret.c |  28 ++-
 tests/unit/test-util-sockets.c  | 235 +++++++++++++++++++
 ui/qemu-pixman.c                |  15 +-
 ui/vnc-enc-tight.c              |  20 +-
 ui/vnc-enc-zrle.c               |   2 +-
 ui/vnc-jobs.c                   |   2 +-
 ui/vnc.c                        |   9 +-
 ui/vnc.h                        |   2 +-
 util/qemu-sockets.c             | 327 ++++++++++++++++----------
 19 files changed, 843 insertions(+), 622 deletions(-)
 delete mode 100644 crypto/cipher-builtin.c.inc
 create mode 100644 crypto/cipher-stub.c.inc
Signature · 1 line
2.49.0
#1 lore raw

[PULL 14/23] scripts/checkpatch: use new hook for MAINTAINERS update check

When seeing a new/deleted/renamed file we check to see if MAINTAINERS is updated, but we don't give the user a list of files affected, as we don't want to repeat the same warning many times over.

Using the new file list hook, we can give a single warning at the end with a list of filenames included.

---
 scripts/checkpatch.pl | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
scripts/checkpatch.pl +19 -18
index 82ec71e05d..c05559a108 100755
@@ -1443,6 +1443,25 @@ sub process_file_list {
 		      join("\n  ", @acpi_nontestexpected) .
 		      "\n\nfound in the same patch\n");
 	}
+
+	my $sawmaintainers = 0;
+	my @maybemaintainers;
+	foreach my $fileinfo (@fileinfos) {
+		if ($fileinfo->{action} ne "modified" &&
+		    $fileinfo->{filenew} !~ m#^tests/data/acpi/#) {
+			push @maybemaintainers, $fileinfo->{filenew};
+		}
+		if ($fileinfo->{filenew} eq "MAINTAINERS") {
+			$sawmaintainers = 1;
+		}
+	}
+
+	# If we don't see a MAINTAINERS update, prod the user to check
+	if (int(@maybemaintainers) > 0 && !$sawmaintainers) {
+		WARN("added, moved or deleted file(s):\n\n  " .
+		     join("\n  ", @maybemaintainers) .
+		     "\n\nDoes MAINTAINERS need updating?\n");
+	}
 }
 
 # Called at the start of processing a diff hunk for a file
@@ -1486,7 +1505,6 @@ sub process {
 
 	my $in_header_lines = $file ? 0 : 1;
 	my $in_commit_log = 0;		#Scanning lines before patch
-	my $reported_maintainer_file = 0;
 	my $reported_mixing_imported_file = 0;
 	my $in_imported_file = 0;
 	my $in_no_imported_file = 0;
@@ -1761,23 +1779,6 @@ sub process {
 			}
 		}
 
-# Check if MAINTAINERS is being updated.  If so, there's probably no need to
-# emit the "does MAINTAINERS need updating?" message on file add/move/delete
-		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
-			$reported_maintainer_file = 1;
-		}
-
-# Check for added, moved or deleted files
-		if (!$reported_maintainer_file && !$in_commit_log &&
-		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
-		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
-		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
-		      (defined($1) || defined($2)))) &&
-		    $realfile !~ m#^tests/data/acpi/#) {
-			$reported_maintainer_file = 1;
-			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
-		}
-
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
 			&checkspdx($realfile, $1);
Signature · 1 line
2.49.0
#2 lore raw

[PULL 05/23] tests: skip legacy qcow2 encryption test if AES is not available

In reply to Daniel P. Berrangé

This avoids test breakage when we drop support for using the built-in AES impl as a fallback for missing crypto libraries.

---
 tests/unit/test-crypto-block.c | 7 +++++++
 1 file changed, 7 insertions(+)
tests/unit/test-crypto-block.c +7 -0
index 9217b9a2ef..3ac7f17b2a 100644
@@ -574,6 +574,13 @@ int main(int argc, char **argv)
     for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
         if (test_data[i].open_opts->format == QCRYPTO_BLOCK_FORMAT_LUKS &&
             !qcrypto_hash_supports(test_data[i].hash_alg)) {
+            g_printerr("# skip unsupported %s\n",
+                       QCryptoHashAlgo_str(test_data[i].hash_alg));
+            continue;
+        }
+        if (!qcrypto_cipher_supports(QCRYPTO_CIPHER_ALGO_AES_128,
+                                     QCRYPTO_CIPHER_MODE_CBC)) {
+            g_printerr("# skip unsupported aes-128:cbc\n");
             continue;
         }
         if (!test_data[i].slow ||
Signature · 1 line
2.49.0
#3 lore raw

[PULL 07/23] crypto: fully drop built-in cipher provider

In reply to Daniel P. Berrangé

When originally creating the internal crypto cipher APIs, they were wired up to use the built-in D3DES and AES implementations, as a way to gracefully transition to the new APIs without introducing an immediate hard dep on any external crypto libraries for the VNC password auth (D3DES) or the qcow2 encryption (AES).

In the 6.1.0 release we dropped the built-in D3DES impl, and also the XTS mode for the AES impl, leaving only AES with ECB/CBC modes. The rational was that with the system emulators, it is expected that 3rd party crypto libraries will be available.

The qcow2 LUKS impl is preferred to the legacy raw AES impl, and by default that requires AES in XTS mode, limiting the usefulness of the built-in cipher provider.

The built-in AES impl has known timing attacks and is only suitable for use cases where a security boundary is already not expected to be provided (TCG).

Providing a built-in cipher impl thus potentially misleads users, should they configure a QEMU without any crypto library, and try to use it with the LUKS backend, even if that requires a non-default configuration choice.

Complete what we started in 6.1.0 and purge the remaining AES support.

Use of either gnutls, nettle, or libcrypt is now mandatory for any cipher support, except for TCG impls.

---
 crypto/cipher-builtin.c.inc | 303 ------------------------------------
 crypto/cipher-stub.c.inc    |  30 ++++
 crypto/cipher.c             |   2 +-
 3 files changed, 31 insertions(+), 304 deletions(-)
 delete mode 100644 crypto/cipher-builtin.c.inc
 create mode 100644 crypto/cipher-stub.c.inc
crypto/cipher-builtin.c.inc deleted +0 -303
deleted file mode 100644
index da5fcbd9a3..0000000000
@@ -1,303 +0,0 @@
-/*
- * QEMU Crypto cipher built-in algorithms
- *
- * Copyright (c) 2015 Red Hat, Inc.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- *
- */
-
-#include "crypto/aes.h"
-
-typedef struct QCryptoCipherBuiltinAESContext QCryptoCipherBuiltinAESContext;
-struct QCryptoCipherBuiltinAESContext {
-    AES_KEY enc;
-    AES_KEY dec;
-};
-
-typedef struct QCryptoCipherBuiltinAES QCryptoCipherBuiltinAES;
-struct QCryptoCipherBuiltinAES {
-    QCryptoCipher base;
-    QCryptoCipherBuiltinAESContext key;
-    uint8_t iv[AES_BLOCK_SIZE];
-};
-
-
-static inline bool qcrypto_length_check(size_t len, size_t blocksize,
-                                        Error **errp)
-{
-    if (unlikely(len & (blocksize - 1))) {
-        error_setg(errp, "Length %zu must be a multiple of block size %zu",
-                   len, blocksize);
-        return false;
-    }
-    return true;
-}
-
-static void qcrypto_cipher_ctx_free(QCryptoCipher *cipher)
-{
-    g_free(cipher);
-}
-
-static int qcrypto_cipher_no_setiv(QCryptoCipher *cipher,
-                                   const uint8_t *iv, size_t niv,
-                                   Error **errp)
-{
-    error_setg(errp, "Setting IV is not supported");
-    return -1;
-}
-
-static void do_aes_encrypt_ecb(const void *vctx,
-                               size_t len,
-                               uint8_t *out,
-                               const uint8_t *in)
-{
-    const QCryptoCipherBuiltinAESContext *ctx = vctx;
-
-    /* We have already verified that len % AES_BLOCK_SIZE == 0. */
-    while (len) {
-        AES_encrypt(in, out, &ctx->enc);
-        in += AES_BLOCK_SIZE;
-        out += AES_BLOCK_SIZE;
-        len -= AES_BLOCK_SIZE;
-    }
-}
-
-static void do_aes_decrypt_ecb(const void *vctx,
-                               size_t len,
-                               uint8_t *out,
-                               const uint8_t *in)
-{
-    const QCryptoCipherBuiltinAESContext *ctx = vctx;
-
-    /* We have already verified that len % AES_BLOCK_SIZE == 0. */
-    while (len) {
-        AES_decrypt(in, out, &ctx->dec);
-        in += AES_BLOCK_SIZE;
-        out += AES_BLOCK_SIZE;
-        len -= AES_BLOCK_SIZE;
-    }
-}
-
-static void do_aes_encrypt_cbc(const AES_KEY *key,
-                               size_t len,
-                               uint8_t *out,
-                               const uint8_t *in,
-                               uint8_t *ivec)
-{
-    uint8_t tmp[AES_BLOCK_SIZE];
-    size_t n;
-
-    /* We have already verified that len % AES_BLOCK_SIZE == 0. */
-    while (len) {
-        for (n = 0; n < AES_BLOCK_SIZE; ++n) {
-            tmp[n] = in[n] ^ ivec[n];
-        }
-        AES_encrypt(tmp, out, key);
-        memcpy(ivec, out, AES_BLOCK_SIZE);
-        len -= AES_BLOCK_SIZE;
-        in += AES_BLOCK_SIZE;
-        out += AES_BLOCK_SIZE;
-    }
-}
-
-static void do_aes_decrypt_cbc(const AES_KEY *key,
-                               size_t len,
-                               uint8_t *out,
-                               const uint8_t *in,
-                               uint8_t *ivec)
-{
-    uint8_t tmp[AES_BLOCK_SIZE];
-    size_t n;
-
-    /* We have already verified that len % AES_BLOCK_SIZE == 0. */
-    while (len) {
-        memcpy(tmp, in, AES_BLOCK_SIZE);
-        AES_decrypt(in, out, key);
-        for (n = 0; n < AES_BLOCK_SIZE; ++n) {
-            out[n] ^= ivec[n];
-        }
-        memcpy(ivec, tmp, AES_BLOCK_SIZE);
-        len -= AES_BLOCK_SIZE;
-        in += AES_BLOCK_SIZE;
-        out += AES_BLOCK_SIZE;
-    }
-}
-
-static int qcrypto_cipher_aes_encrypt_ecb(QCryptoCipher *cipher,
-                                          const void *in, void *out,
-                                          size_t len, Error **errp)
-{
-    QCryptoCipherBuiltinAES *ctx
-        = container_of(cipher, QCryptoCipherBuiltinAES, base);
-
-    if (!qcrypto_length_check(len, AES_BLOCK_SIZE, errp)) {
-        return -1;
-    }
-    do_aes_encrypt_ecb(&ctx->key, len, out, in);
-    return 0;
-}
-
-static int qcrypto_cipher_aes_decrypt_ecb(QCryptoCipher *cipher,
-                                          const void *in, void *out,
-                                          size_t len, Error **errp)
-{
-    QCryptoCipherBuiltinAES *ctx
-        = container_of(cipher, QCryptoCipherBuiltinAES, base);
-
-    if (!qcrypto_length_check(len, AES_BLOCK_SIZE, errp)) {
-        return -1;
-    }
-    do_aes_decrypt_ecb(&ctx->key, len, out, in);
-    return 0;
-}
-
-static int qcrypto_cipher_aes_encrypt_cbc(QCryptoCipher *cipher,
-                                          const void *in, void *out,
-                                          size_t len, Error **errp)
-{
-    QCryptoCipherBuiltinAES *ctx
-        = container_of(cipher, QCryptoCipherBuiltinAES, base);
-
-    if (!qcrypto_length_check(len, AES_BLOCK_SIZE, errp)) {
-        return -1;
-    }
-    do_aes_encrypt_cbc(&ctx->key.enc, len, out, in, ctx->iv);
-    return 0;
-}
-
-static int qcrypto_cipher_aes_decrypt_cbc(QCryptoCipher *cipher,
-                                          const void *in, void *out,
-                                          size_t len, Error **errp)
-{
-    QCryptoCipherBuiltinAES *ctx
-        = container_of(cipher, QCryptoCipherBuiltinAES, base);
-
-    if (!qcrypto_length_check(len, AES_BLOCK_SIZE, errp)) {
-        return -1;
-    }
-    do_aes_decrypt_cbc(&ctx->key.dec, len, out, in, ctx->iv);
-    return 0;
-}
-
-static int qcrypto_cipher_aes_setiv(QCryptoCipher *cipher, const uint8_t *iv,
-                             size_t niv, Error **errp)
-{
-    QCryptoCipherBuiltinAES *ctx
-        = container_of(cipher, QCryptoCipherBuiltinAES, base);
-
-    if (niv != AES_BLOCK_SIZE) {
-        error_setg(errp, "IV must be %d bytes not %zu",
-                   AES_BLOCK_SIZE, niv);
-        return -1;
-    }
-
-    memcpy(ctx->iv, iv, AES_BLOCK_SIZE);
-    return 0;
-}
-
-static const struct QCryptoCipherDriver qcrypto_cipher_aes_driver_ecb = {
-    .cipher_encrypt = qcrypto_cipher_aes_encrypt_ecb,
-    .cipher_decrypt = qcrypto_cipher_aes_decrypt_ecb,
-    .cipher_setiv = qcrypto_cipher_no_setiv,
-    .cipher_free = qcrypto_cipher_ctx_free,
-};
-
-static const struct QCryptoCipherDriver qcrypto_cipher_aes_driver_cbc = {
-    .cipher_encrypt = qcrypto_cipher_aes_encrypt_cbc,
-    .cipher_decrypt = qcrypto_cipher_aes_decrypt_cbc,
-    .cipher_setiv = qcrypto_cipher_aes_setiv,
-    .cipher_free = qcrypto_cipher_ctx_free,
-};
-
-bool qcrypto_cipher_supports(QCryptoCipherAlgo alg,
-                             QCryptoCipherMode mode)
-{
-    switch (alg) {
-    case QCRYPTO_CIPHER_ALGO_AES_128:
-    case QCRYPTO_CIPHER_ALGO_AES_192:
-    case QCRYPTO_CIPHER_ALGO_AES_256:
-        switch (mode) {
-        case QCRYPTO_CIPHER_MODE_ECB:
-        case QCRYPTO_CIPHER_MODE_CBC:
-            return true;
-        default:
-            return false;
-        }
-        break;
-    default:
-        return false;
-    }
-}
-
-static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgo alg,
-                                             QCryptoCipherMode mode,
-                                             const uint8_t *key,
-                                             size_t nkey,
-                                             Error **errp)
-{
-    if (!qcrypto_cipher_validate_key_length(alg, mode, nkey, errp)) {
-        return NULL;
-    }
-
-    switch (alg) {
-    case QCRYPTO_CIPHER_ALGO_AES_128:
-    case QCRYPTO_CIPHER_ALGO_AES_192:
-    case QCRYPTO_CIPHER_ALGO_AES_256:
-        {
-            QCryptoCipherBuiltinAES *ctx;
-            const QCryptoCipherDriver *drv;
-
-            switch (mode) {
-            case QCRYPTO_CIPHER_MODE_ECB:
-                drv = &qcrypto_cipher_aes_driver_ecb;
-                break;
-            case QCRYPTO_CIPHER_MODE_CBC:
-                drv = &qcrypto_cipher_aes_driver_cbc;
-                break;
-            default:
-                goto bad_mode;
-            }
-
-            ctx = g_new0(QCryptoCipherBuiltinAES, 1);
-            ctx->base.driver = drv;
-
-            if (AES_set_encrypt_key(key, nkey * 8, &ctx->key.enc)) {
-                error_setg(errp, "Failed to set encryption key");
-                goto error;
-            }
-            if (AES_set_decrypt_key(key, nkey * 8, &ctx->key.dec)) {
-                error_setg(errp, "Failed to set decryption key");
-                goto error;
-            }
-
-            return &ctx->base;
-
-        error:
-            g_free(ctx);
-            return NULL;
-        }
-
-    default:
-        error_setg(errp,
-                   "Unsupported cipher algorithm %s",
-                   QCryptoCipherAlgo_str(alg));
-        return NULL;
-    }
-
- bad_mode:
-    error_setg(errp, "Unsupported cipher mode %s",
-               QCryptoCipherMode_str(mode));
-    return NULL;
-}
crypto/cipher-stub.c.inc new file +30 -0
new file mode 100644
index 0000000000..1b7ea81eac
@@ -0,0 +1,30 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * QEMU Crypto cipher impl stub
+ *
+ * Copyright (c) 2025 Red Hat, Inc.
+ *
+ */
+
+bool qcrypto_cipher_supports(QCryptoCipherAlgo alg,
+                             QCryptoCipherMode mode)
+{
+    return false;
+}
+
+static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgo alg,
+                                             QCryptoCipherMode mode,
+                                             const uint8_t *key,
+                                             size_t nkey,
+                                             Error **errp)
+{
+    if (!qcrypto_cipher_validate_key_length(alg, mode, nkey, errp)) {
+        return NULL;
+    }
+
+    error_setg(errp,
+               "Unsupported cipher algorithm %s, no crypto library enabled in build",
+               QCryptoCipherAlgo_str(alg));
+    return NULL;
+}
crypto/cipher.c +1 -1
index c14a8b8a11..229710f76b 100644
@@ -145,7 +145,7 @@ qcrypto_cipher_validate_key_length(QCryptoCipherAlgo alg,
 #elif defined CONFIG_GNUTLS_CRYPTO
 #include "cipher-gnutls.c.inc"
 #else
-#include "cipher-builtin.c.inc"
+#include "cipher-stub.c.inc"
 #endif
 
 QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgo alg,
Signature · 1 line
2.49.0
#4 lore raw

[PULL 04/23] tests: skip encrypted secret tests if AES is not available

In reply to Daniel P. Berrangé

This avoids test breakage when we drop support for using the built-in AES impl as a fallback for missing crypto libraries.

---
 tests/unit/test-crypto-secret.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)
tests/unit/test-crypto-secret.c +16 -12
index ffd13ff70e..fc32a01747 100644
@@ -22,6 +22,7 @@
 
 #include "crypto/init.h"
 #include "crypto/secret.h"
+#include "crypto/cipher.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
 #if defined(CONFIG_KEYUTILS) && defined(CONFIG_SECRET_KEYRING)
@@ -597,18 +598,21 @@ int main(int argc, char **argv)
     g_test_add_func("/crypto/secret/conv/utf8/base64",
                     test_secret_conv_utf8_base64);
 
-    g_test_add_func("/crypto/secret/crypt/raw",
-                    test_secret_crypt_raw);
-    g_test_add_func("/crypto/secret/crypt/base64",
-                    test_secret_crypt_base64);
-    g_test_add_func("/crypto/secret/crypt/shortkey",
-                    test_secret_crypt_short_key);
-    g_test_add_func("/crypto/secret/crypt/shortiv",
-                    test_secret_crypt_short_iv);
-    g_test_add_func("/crypto/secret/crypt/missingiv",
-                    test_secret_crypt_missing_iv);
-    g_test_add_func("/crypto/secret/crypt/badiv",
-                    test_secret_crypt_bad_iv);
+    if (qcrypto_cipher_supports(QCRYPTO_CIPHER_ALGO_AES_128,
+                                QCRYPTO_CIPHER_MODE_CBC)) {
+        g_test_add_func("/crypto/secret/crypt/raw",
+                        test_secret_crypt_raw);
+        g_test_add_func("/crypto/secret/crypt/base64",
+                        test_secret_crypt_base64);
+        g_test_add_func("/crypto/secret/crypt/shortkey",
+                        test_secret_crypt_short_key);
+        g_test_add_func("/crypto/secret/crypt/shortiv",
+                        test_secret_crypt_short_iv);
+        g_test_add_func("/crypto/secret/crypt/missingiv",
+                        test_secret_crypt_missing_iv);
+        g_test_add_func("/crypto/secret/crypt/badiv",
+                        test_secret_crypt_bad_iv);
+    }
 
     return g_test_run();
 }
Signature · 1 line
2.49.0
#5 lore raw

[PULL 22/23] util/qemu-sockets: Introduce inet socket options controlling TCP keep-alive

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

With the default TCP stack configuration, it could be even 2 hours before the connection times out due to the other side not being reachable. However, in some cases, the application needs to be aware of a connection issue much sooner.

This is the case, for example, for postcopy live migration. If there is no traffic from the migration destination guest (server-side) to the migration source guest (client-side), the destination keeps waiting for pages indefinitely and does not switch to the postcopy-paused state. This can happen, for example, if the destination QEMU instance is started with the '-S' command line option and the machine is not started yet, or if the machine is idle and produces no new page faults for not-yet-migrated pages.

This patch introduces new inet socket parameters that control count, idle period, and interval of TCP keep-alive packets before the connection is considered broken. These parameters are available on systems where the respective TCP socket options are defined, that includes Linux, Windows, macOS, but not OpenBSD. Additionally, macOS defines TCP_KEEPIDLE as TCP_KEEPALIVE instead, so the patch supplies its own definition.

The default value for all is 0, which means the system configuration is used.

---
 meson.build                    | 30 +++++++++++++
 qapi/sockets.json              | 19 ++++++++
 tests/unit/test-util-sockets.c | 39 +++++++++++++++++
 util/qemu-sockets.c            | 80 ++++++++++++++++++++++++++++++++++
 4 files changed, 168 insertions(+)
meson.build +30 -0
index ad2053f968..fdad3fb528 100644
@@ -2760,6 +2760,36 @@ if linux_io_uring.found()
   config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
                        cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
 endif
+config_host_data.set('HAVE_TCP_KEEPCNT',
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPCNT') or
+                     cc.compiles('''
+                     #include <ws2tcpip.h>
+                     #ifndef TCP_KEEPCNT
+                     #error
+                     #endif
+                     int main(void) { return 0; }''',
+                     name: 'Win32 TCP_KEEPCNT'))
+# On Darwin TCP_KEEPIDLE is available under different name, TCP_KEEPALIVE.
+# https://github.com/apple/darwin-xnu/blob/xnu-4570.1.46/bsd/man/man4/tcp.4#L172
+config_host_data.set('HAVE_TCP_KEEPIDLE',
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE') or
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPALIVE') or
+                     cc.compiles('''
+                     #include <ws2tcpip.h>
+                     #ifndef TCP_KEEPIDLE
+                     #error
+                     #endif
+                     int main(void) { return 0; }''',
+                     name: 'Win32 TCP_KEEPIDLE'))
+config_host_data.set('HAVE_TCP_KEEPINTVL',
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPINTVL') or
+                     cc.compiles('''
+                     #include <ws2tcpip.h>
+                     #ifndef TCP_KEEPINTVL
+                     #error
+                     #endif
+                     int main(void) { return 0; }''',
+                     name: 'Win32 TCP_KEEPINTVL'))
 
 # has_member
 config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
qapi/sockets.json +19 -0
index 62797cd027..f9f559daba 100644
@@ -59,6 +59,22 @@
 # @keep-alive: enable keep-alive when connecting to/listening on this socket.
 #     (Since 4.2, not supported for listening sockets until 10.1)
 #
+# @keep-alive-count: number of keep-alive packets sent before the connection is
+#     closed.  Only supported for TCP sockets on systems where TCP_KEEPCNT
+#     socket option is defined (this includes Linux, Windows, macOS, FreeBSD,
+#     but not OpenBSD).  When set to 0, system setting is used.  (Since 10.1)
+#
+# @keep-alive-idle: time in seconds the connection needs to be idle before
+#     sending a keepalive packet.  Only supported for TCP sockets on systems
+#     where TCP_KEEPIDLE socket option is defined (this includes Linux,
+#     Windows, macOS, FreeBSD, but not OpenBSD).  When set to 0, system setting
+#     is used.  (Since 10.1)
+#
+# @keep-alive-interval: time in seconds between keep-alive packets.  Only
+#     supported for TCP sockets on systems where TCP_KEEPINTVL is defined (this
+#     includes Linux, Windows, macOS, FreeBSD, but not OpenBSD).  When set to
+#     0, system setting is used.  (Since 10.1)
+#
 # @mptcp: enable multi-path TCP.  (Since 6.1)
 #
 # Since: 1.3
@@ -71,6 +87,9 @@
     '*ipv4': 'bool',
     '*ipv6': 'bool',
     '*keep-alive': 'bool',
+    '*keep-alive-count': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPCNT' },
+    '*keep-alive-idle': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
+    '*keep-alive-interval': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPINTVL' },
     '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
 
 ##
tests/unit/test-util-sockets.c +39 -0
index 9e39b92e7c..8492f4d68f 100644
@@ -359,6 +359,24 @@ static void inet_parse_test_helper(const char *str,
         g_assert_cmpint(addr.ipv6, ==, exp_addr->ipv6);
         g_assert_cmpint(addr.has_keep_alive, ==, exp_addr->has_keep_alive);
         g_assert_cmpint(addr.keep_alive, ==, exp_addr->keep_alive);
+#ifdef HAVE_TCP_KEEPCNT
+        g_assert_cmpint(addr.has_keep_alive_count, ==,
+                        exp_addr->has_keep_alive_count);
+        g_assert_cmpint(addr.keep_alive_count, ==,
+                        exp_addr->keep_alive_count);
+#endif
+#ifdef HAVE_TCP_KEEPIDLE
+        g_assert_cmpint(addr.has_keep_alive_idle, ==,
+                        exp_addr->has_keep_alive_idle);
+        g_assert_cmpint(addr.keep_alive_idle, ==,
+                        exp_addr->keep_alive_idle);
+#endif
+#ifdef HAVE_TCP_KEEPINTVL
+        g_assert_cmpint(addr.has_keep_alive_interval, ==,
+                        exp_addr->has_keep_alive_interval);
+        g_assert_cmpint(addr.keep_alive_interval, ==,
+                        exp_addr->keep_alive_interval);
+#endif
 #ifdef HAVE_IPPROTO_MPTCP
         g_assert_cmpint(addr.has_mptcp, ==, exp_addr->has_mptcp);
         g_assert_cmpint(addr.mptcp, ==, exp_addr->mptcp);
@@ -460,6 +478,18 @@ static void test_inet_parse_all_options_good(void)
         .ipv6 = true,
         .has_keep_alive = true,
         .keep_alive = true,
+#ifdef HAVE_TCP_KEEPCNT
+        .has_keep_alive_count = true,
+        .keep_alive_count = 10,
+#endif
+#ifdef HAVE_TCP_KEEPIDLE
+        .has_keep_alive_idle = true,
+        .keep_alive_idle = 60,
+#endif
+#ifdef HAVE_TCP_KEEPINTVL
+        .has_keep_alive_interval = true,
+        .keep_alive_interval = 30,
+#endif
 #ifdef HAVE_IPPROTO_MPTCP
         .has_mptcp = true,
         .mptcp = false,
@@ -467,6 +497,15 @@ static void test_inet_parse_all_options_good(void)
     };
     inet_parse_test_helper(
         "[::1]:5000,numeric=on,to=5006,ipv4=off,ipv6=on,keep-alive=on"
+#ifdef HAVE_TCP_KEEPCNT
+        ",keep-alive-count=10"
+#endif
+#ifdef HAVE_TCP_KEEPIDLE
+        ",keep-alive-idle=60"
+#endif
+#ifdef HAVE_TCP_KEEPINTVL
+        ",keep-alive-interval=30"
+#endif
 #ifdef HAVE_IPPROTO_MPTCP
         ",mptcp=off"
 #endif
util/qemu-sockets.c +80 -0

104 lines not on this page · raw message

Signature · 1 line
2.49.0
#6 lore raw

[PULL 08/23] Revert "scripts: mandate that new files have SPDX-License-Identifier"

In reply to Daniel P. Berrangé

This reverts commit fa4d79c64dae03ffa269e42e21822453856618b7.

The logic in this commit was flawed in two critical ways

* It always failed to report SPDX validation on the last newly added file. IOW, it only worked if at least 2 new files were added in a commit

* If an existing file change, followed a new file change, in the commit and the existing file context/changed lines included SPDX-License-Identifier, it would incorrectly associate this with the previous newly added file.

Simply reverting this commit will make it significantly easier to understand the improved logic in the following commit.

---
 scripts/checkpatch.pl | 30 ------------------------------
 1 file changed, 30 deletions(-)
scripts/checkpatch.pl +0 -30
index 365892de04..d355c0dad5 100755
@@ -1442,8 +1442,6 @@ sub process {
 	my $in_imported_file = 0;
 	my $in_no_imported_file = 0;
 	my $non_utf8_charset = 0;
-	my $expect_spdx = 0;
-	my $expect_spdx_file;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -1681,34 +1679,6 @@ sub process {
 			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
 
-# All new files should have a SPDX-License-Identifier tag
-		if ($line =~ /^new file mode\s*\d+\s*$/) {
-		    if ($expect_spdx) {
-			if ($expect_spdx_file =~
-			    /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
-			    # source code files MUST have SPDX license declared
-			    ERROR("New file '$expect_spdx_file' requires " .
-				  "'SPDX-License-Identifier'");
-			} else {
-			    # Other files MAY have SPDX license if appropriate
-			    WARN("Does new file '$expect_spdx_file' need " .
-				 "'SPDX-License-Identifier'?");
-			}
-		    }
-		    $expect_spdx = 1;
-		    $expect_spdx_file = undef;
-		} elsif ($expect_spdx) {
-		    $expect_spdx_file = $realfile unless
-			defined $expect_spdx_file;
-
-		    # SPDX tags may occurr in comments which were
-		    # stripped from '$line', so use '$rawline'
-		    if ($rawline =~ /SPDX-License-Identifier/) {
-			$expect_spdx = 0;
-			$expect_spdx_file = undef;
-		    }
-		}
-
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
 		    &checkspdx($realfile, $1);
Signature · 1 line
2.49.0
#7 lore raw

[PULL 06/23] tests: fix skipping cipher tests when AES is not available

In reply to Daniel P. Berrangé

This avoid tests breakage when we drop support for using the built-in AES impl as a fallback for missing crypto libraries.

---
 tests/unit/test-crypto-cipher.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
tests/unit/test-crypto-cipher.c +9 -4
index b328b482e1..1331d558cf 100644
@@ -828,11 +828,16 @@ int main(int argc, char **argv)
         }
     }
 
-    g_test_add_func("/crypto/cipher/null-iv",
-                    test_cipher_null_iv);
+    if (qcrypto_cipher_supports(QCRYPTO_CIPHER_ALGO_AES_256,
+                                QCRYPTO_CIPHER_MODE_CBC)) {
+        g_test_add_func("/crypto/cipher/null-iv",
+                        test_cipher_null_iv);
 
-    g_test_add_func("/crypto/cipher/short-plaintext",
-                    test_cipher_short_plaintext);
+        g_test_add_func("/crypto/cipher/short-plaintext",
+                        test_cipher_short_plaintext);
+    } else {
+        g_printerr("# skip unsupported aes-256:cbc\n");
+    }
 
     return g_test_run();
 }
Signature · 1 line
2.49.0
#8 lore raw

[PULL 11/23] scripts/checkpatch: use new hook for ACPI test data check

In reply to Daniel P. Berrangé

The ACPI test data check needs to analyse a list of all files in a commit, so can use the new hook for processing the file list.

---
 scripts/checkpatch.pl | 61 ++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 32 deletions(-)
scripts/checkpatch.pl +29 -32
index bb3830d693..817df98231 100755
@@ -1330,29 +1330,6 @@ sub WARN {
 	}
 }
 
-# According to tests/qtest/bios-tables-test.c: do not
-# change expected file in the same commit with adding test
-sub checkfilename {
-	my ($name, $acpi_testexpected, $acpi_nontestexpected) = @_;
-
-        # Note: shell script that rebuilds the expected files is in the same
-        # directory as files themselves.
-        # Note: allowed diff list can be changed both when changing expected
-        # files and when changing tests.
-	if ($name =~ m#^tests/data/acpi/# and not $name =~ m#^\.sh$#) {
-		$$acpi_testexpected = $name;
-	} elsif ($name !~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
-		$$acpi_nontestexpected = $name;
-	}
-	if (defined $$acpi_testexpected and defined $$acpi_nontestexpected) {
-		ERROR("Do not add expected files together with tests, " .
-		      "follow instructions in " .
-		      "tests/qtest/bios-tables-test.c: both " .
-		      $$acpi_testexpected . " and " .
-		      $$acpi_nontestexpected . " found\n");
-	}
-}
-
 sub checkspdx {
     my ($file, $expr) = @_;
 
@@ -1438,6 +1415,34 @@ sub checkspdx {
 # real filenames that were seen in the patch
 sub process_file_list {
 	my @fileinfos = @_;
+
+	# According to tests/qtest/bios-tables-test.c: do not
+	# change expected file in the same commit with adding test
+	my @acpi_testexpected;
+	my @acpi_nontestexpected;
+
+	foreach my $fileinfo (@fileinfos) {
+		# Note: shell script that rebuilds the expected files is in
+		# the same directory as files themselves.
+		# Note: allowed diff list can be changed both when changing
+		#  expected files and when changing tests.
+		if ($fileinfo->{filenew} =~ m#^tests/data/acpi/# &&
+		    $fileinfo->{filenew} !~ m#^\.sh$#) {
+			push @acpi_testexpected, $fileinfo->{filenew};
+		} elsif ($fileinfo->{filenew} !~
+			 m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
+			push @acpi_nontestexpected, $fileinfo->{filenew};
+		}
+	}
+	if (int(@acpi_testexpected) > 0 and int(@acpi_nontestexpected) > 0) {
+		ERROR("Do not add expected files together with tests, " .
+		      "follow instructions in " .
+		      "tests/qtest/bios-tables-test.c. Files\n\n  " .
+		      join("\n  ", @acpi_testexpected) .
+		      "\n\nand\n\n  " .
+		      join("\n  ", @acpi_nontestexpected) .
+		      "\n\nfound in the same patch\n");
+	}
 }
 
 # Called at the start of processing a diff hunk for a file
@@ -1502,9 +1507,6 @@ sub process {
 	my %suppress_whiletrailers;
 	my %suppress_export;
 
-        my $acpi_testexpected;
-        my $acpi_nontestexpected;
-
 	# Pre-scan the patch sanitizing the lines.
 
 	sanitise_line_reset();
@@ -1643,7 +1645,6 @@ sub process {
 			$fileold =~ s@^([^/]*)/@@ if (!$file);
 			$filenew =~ s@^([^/]*)/@@ if (!$file);
 			$realfile = $filenew;
-	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
 
 			$fileinfo = {
 				"isgit" => 1,
@@ -1677,8 +1678,6 @@ sub process {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
 
-	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
-
 			$p1_prefix = $1;
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
@@ -1771,9 +1770,7 @@ sub process {
 		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
 		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
 		      (defined($1) || defined($2)))) &&
-                      !(($realfile ne '') &&
-                        defined($acpi_testexpected) &&
-                        ($realfile eq $acpi_testexpected))) {
+		    $realfile !~ m#^tests/data/acpi/#) {
 			$reported_maintainer_file = 1;
 			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
Signature · 1 line
2.49.0
#9 lore raw

[PULL 01/23] ui/vnc.c: replace big endian flag with byte order value

In reply to Daniel P. Berrangé

It will make it easier to do certain comparisons in future if we store G_BIG_ENDIAN/G_LITTLE_ENDIAN directly, instead of a boolean flag, as we can then compare directly to the G_BYTE_ORDER constant.

---
 ui/vnc-enc-tight.c | 2 +-
 ui/vnc-enc-zrle.c  | 2 +-
 ui/vnc-jobs.c      | 2 +-
 ui/vnc.c           | 6 +++---
 ui/vnc.h           | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
ui/vnc-enc-tight.c +1 -1
index 41f559eb83..f8aaa8f346 100644
@@ -150,7 +150,7 @@ tight_detect_smooth_image24(VncState *vs, int w, int h)
      * If client is big-endian, color samples begin from the second
      * byte (offset 1) of a 32-bit pixel value.
      */
-    off = vs->client_be;
+    off = vs->client_endian == G_BIG_ENDIAN ? 1 : 0;
 
     memset(stats, 0, sizeof (stats));
 
ui/vnc-enc-zrle.c +1 -1
index bd33b89063..97ec6c7119 100644
@@ -255,7 +255,7 @@ static void zrle_write_u8(VncState *vs, uint8_t value)
 static int zrle_send_framebuffer_update(VncState *vs, int x, int y,
                                         int w, int h)
 {
-    bool be = vs->client_be;
+    bool be = vs->client_endian == G_BIG_ENDIAN;
     size_t bytes;
     int zywrle_level;
 
ui/vnc-jobs.c +1 -1
index fcca7ec632..d3486af9e2 100644
@@ -188,7 +188,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
     local->lossy_rect = orig->lossy_rect;
     local->write_pixels = orig->write_pixels;
     local->client_pf = orig->client_pf;
-    local->client_be = orig->client_be;
+    local->client_endian = orig->client_endian;
     local->tight = orig->tight;
     local->zlib = orig->zlib;
     local->hextile = orig->hextile;
ui/vnc.c +3 -3
index 9e097dc4b4..ab18172c4d 100644
@@ -891,7 +891,7 @@ void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
         buf[0] = v;
         break;
     case 2:
-        if (vs->client_be) {
+        if (vs->client_endian == G_BIG_ENDIAN) {
             buf[0] = v >> 8;
             buf[1] = v;
         } else {
@@ -901,7 +901,7 @@ void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
         break;
     default:
     case 4:
-        if (vs->client_be) {
+        if (vs->client_endian == G_BIG_ENDIAN) {
             buf[0] = v >> 24;
             buf[1] = v >> 16;
             buf[2] = v >> 8;
@@ -2312,7 +2312,7 @@ static void set_pixel_format(VncState *vs, int bits_per_pixel,
     vs->client_pf.bits_per_pixel = bits_per_pixel;
     vs->client_pf.bytes_per_pixel = bits_per_pixel / 8;
     vs->client_pf.depth = bits_per_pixel == 32 ? 24 : bits_per_pixel;
-    vs->client_be = big_endian_flag;
+    vs->client_endian = big_endian_flag ? G_BIG_ENDIAN : G_LITTLE_ENDIAN;
 
     if (!true_color_flag) {
         send_color_map(vs);
ui/vnc.h +1 -1
index acc53a2cc1..02613aa63a 100644
@@ -323,7 +323,7 @@ struct VncState
     VncWritePixels *write_pixels;
     PixelFormat client_pf;
     pixman_format_code_t client_format;
-    bool client_be;
+    int client_endian; /* G_LITTLE_ENDIAN or G_BIG_ENDIAN */
 
     CaptureVoiceOut *audio_cap;
     struct audsettings as;
Signature · 1 line
2.49.0
#10 lore raw

[PULL 10/23] scripts/checkpatch: introduce tracking of file start/end

In reply to Daniel P. Berrangé

Some checks want to be performed either at the start of a new file within a patch, or at the end. This is complicated by the fact that the information relevant to the check may be spread across multiple lines. It is further complicated by a need to support both git and non-git diffs, and special handling for renames where there might not be any patch hunks.

To handle this more sanely, introduce explicit tracking of file start/end, taking account of git metadata, and calling a hook function at each transition.

---
 scripts/checkpatch.pl | 110 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 3 deletions(-)
scripts/checkpatch.pl +107 -3
index 2c19f6f0f2..bb3830d693 100755
@@ -1417,6 +1417,39 @@ sub checkspdx {
     }
 }
 
+# All three of the methods below take a 'file info' record
+# which is a hash ref containing
+#
+#  'isgit': 1 if an enhanced git diff or 0 for a plain diff
+#  'githeader': 1 if still parsing git patch header, 0 otherwise
+#  'linestart': line number of start of file diff
+#  'lineend': line number of end of file diff
+#  'filenew': the new filename
+#  'fileold': the old filename (same as 'new filename' except
+#             for renames in git diffs)
+#  'action': one of 'modified' (always) or 'new' or 'deleted' or
+#            'renamed' (git diffs only)
+#  'mode': file mode for new/deleted files (git diffs only)
+#  'similarity': file similarity when renamed (git diffs only)
+#  'facts': hash ref for storing any metadata related to checks
+#
+
+# Called at the end of each patch, with the list of
+# real filenames that were seen in the patch
+sub process_file_list {
+	my @fileinfos = @_;
+}
+
+# Called at the start of processing a diff hunk for a file
+sub process_start_of_file {
+	my $fileinfo = shift;
+}
+
+# Called at the end of processing a diff hunk for a file
+sub process_end_of_file {
+	my $fileinfo = shift;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1453,7 +1486,10 @@ sub process {
 	my $realfile = '';
 	my $realline = 0;
 	my $realcnt = 0;
+	my $fileinfo;
+	my @fileinfolist;
 	my $here = '';
+	my $oldhere = '';
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
@@ -1591,17 +1627,56 @@ sub process {
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
 
+		$oldhere = $here;
 		$here = "#$linenr: " if (!$file);
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
-			$realfile =~ s@^([^/]*)/@@ if (!$file);
+		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
+			my $fileold = $1;
+			my $filenew = $2;
+
+			if (defined $fileinfo) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo)
+		        }
+			$fileold =~ s@^([^/]*)/@@ if (!$file);
+			$filenew =~ s@^([^/]*)/@@ if (!$file);
+			$realfile = $filenew;
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
+
+			$fileinfo = {
+				"isgit" => 1,
+				"githeader" => 1,
+				"linestart" => $here,
+				"lineend" => 0,
+				"fileold" => $fileold,
+				"filenew" => $filenew,
+				"action" => "modified",
+				"mode" => 0,
+				"similarity" => 0,
+				"facts" => {},
+		        };
+			push @fileinfolist, $fileinfo;
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^(new|deleted) (?:file )?mode\s+([0-7]+)$/) {
+			$fileinfo->{action} = $1;
+			$fileinfo->{mode} = oct($2);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^similarity index (\d+)%/) {
+			$fileinfo->{similarity} = int($1);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^rename (from|to) [\w\/\.\-]+\s*$/) {
+			$fileinfo->{action} = "renamed";
+			# For a no-change rename, we'll never have any "+++..."
+			# lines, so trigger actions now
+			if ($1 eq "to" && $fileinfo->{similarity} == 100)  {
+				process_start_of_file($fileinfo);
+			}
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
+
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
 
 			$p1_prefix = $1;
@@ -1610,6 +1685,30 @@ sub process {
 				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
 			}
 
+			if (defined $fileinfo && !$fileinfo->{isgit}) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo);
+			}
+
+			if (!defined $fileinfo || !$fileinfo->{isgit}) {
+				$fileinfo = {
+					"isgit" => 0,
+					"githeader" => 0,
+					"linestart" => $here,
+					"lineend" => 0,
+					"fileold" => $realfile,
+					"filenew" => $realfile,
+					"action" => "modified",
+					"mode" => 0,
+					"similarity" => 0,
+					"facts" => {},
+			        };
+				push @fileinfolist, $fileinfo;
+			} else {
+				$fileinfo->{githeader} = 0;
+			}
+			process_start_of_file($fileinfo);
+
 			next;
 		}
 
@@ -3216,6 +3315,11 @@ sub process {
 		}
 	}
 
+	if (defined $fileinfo) {
+		process_end_of_file($fileinfo);
+	}
+	process_file_list(@fileinfolist);
+
 	if ($is_patch && $chk_signoff && $signoff == 0) {
 		ERROR("Missing Signed-off-by: line(s)\n");
 	}
Signature · 1 line
2.49.0
#11 lore raw

[PULL 13/23] scripts/checkpatch: expand pattern for matching makefiles

In reply to Daniel P. Berrangé

The current regex matches Makefile & Makefile.objs, but the latter is no longer used, anjd we're missing coverage of Makefile.include and Makefile.target. Expand the pattern to match any suffix.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 881dcac29b..82ec71e05d 100755
@@ -1454,7 +1454,7 @@ sub process_start_of_file {
 		my $permhere = $fileinfo->{linestart} . "FILE: " .
 			$fileinfo->{filenew} . "\n";
 		if ($fileinfo->{filenew} =~
-		    /(\bMakefile(?:\.objs)?|\.(c|cc|cpp|h|mak|s|S))$/) {
+		    /(\bMakefile.*|\.(c|cc|cpp|h|mak|s|S))$/) {
 			ERROR("do not set execute permissions for source " .
 			      "files\n" . $permhere);
 		}
Signature · 1 line
2.49.0
#12 lore raw

[PULL 03/23] ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats

In reply to Daniel P. Berrangé

When sending a tight rectangle with the palette filter, if the client format was 8/16bpp, the colours on big endian hosts are not set as we're sending the wrong bytes. We must first cast the 32-bit colour to a 16/8-bit value, and then send the result.

---
 ui/vnc-enc-tight.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
ui/vnc-enc-tight.c +12 -4
index a5bdc19ebb..25c7b2c788 100644
@@ -1001,16 +1001,24 @@ static int send_mono_rect(VncState *vs, int x, int y,
         break;
     }
     case 2:
-        vnc_write(vs, &bg, 2);
-        vnc_write(vs, &fg, 2);
+    {
+        uint16_t bg16 = bg;
+        uint16_t fg16 = fg;
+        vnc_write(vs, &bg16, 2);
+        vnc_write(vs, &fg16, 2);
         tight_encode_mono_rect16(vs->tight->tight.buffer, w, h, bg, fg);
         break;
+    }
     default:
-        vnc_write_u8(vs, bg);
-        vnc_write_u8(vs, fg);
+    {
+        uint8_t bg8 = bg;
+        uint8_t fg8 = fg;
+        vnc_write_u8(vs, bg8);
+        vnc_write_u8(vs, fg8);
         tight_encode_mono_rect8(vs->tight->tight.buffer, w, h, bg, fg);
         break;
     }
+    }
     vs->tight->tight.offset = bytes;
 
     bytes = tight_compress_data(vs, stream, bytes, level, Z_DEFAULT_STRATEGY);
Signature · 1 line
2.49.0
#13 lore raw

[PULL 16/23] scripts/checkpatch: reject license boilerplate on new files

In reply to Daniel P. Berrangé

The previous commit mandates use of SPDX-License-Identifier on common source files, and encourages it on all other files.

Some contributors are none the less still also including the license boilerplate text. This is redundant and will potentially cause trouble if inconsistent with the SPDX declaration.

Match common boilerplate text blurbs and report them as invalid, for newly added files.

---
 scripts/checkpatch.pl | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
scripts/checkpatch.pl +23 -0
index da13102eda..9291bc9209 100755
@@ -365,6 +365,18 @@ our @typeList = (
 	qr{guintptr},
 );
 
+# Match text found in common license boilerplate comments:
+# for new files the SPDX-License-Identifier line is sufficient.
+our @LICENSE_BOILERPLATE = (
+	"licensed under the terms of the GNU GPL",
+	"under the terms of the GNU General Public License",
+	"under the terms of the GNU Lesser General Public",
+	"Permission is hereby granted, free of charge",
+	"GNU GPL, version 2 or later",
+	"See the COPYING file"
+);
+our $LICENSE_BOILERPLATE_RE = join("|", @LICENSE_BOILERPLATE);
+
 # Load common spelling mistakes and build regular expression list.
 my $misspellings;
 my %spelling_fix;
@@ -1497,6 +1509,13 @@ sub process_end_of_file {
 			     "' need 'SPDX-License-Identifier'?");
 		}
 	}
+	if ($fileinfo->{action} eq "new" &&
+	    exists $fileinfo->{facts}->{sawboilerplate}) {
+		ERROR("New file '" . $fileinfo->{filenew} . "' must " .
+		      "not have license boilerplate header text, only " .
+		      "the SPDX-License-Identifier, unless this file was " .
+		      "copied from existing code already having such text.");
+	}
 }
 
 sub process {
@@ -1799,6 +1818,10 @@ sub process {
 			&checkspdx($realfile, $1);
 		}
 
+		if ($rawline =~ /$LICENSE_BOILERPLATE_RE/) {
+			$fileinfo->{facts}->{sawboilerplate} = 1;
+		}
+
 		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
 			my $tag = $1;
 			my @permitted = qw(
Signature · 1 line
2.49.0
#14 lore raw

[PULL 23/23] scripts/checkpatch.pl: mandate SPDX tag for Rust src files

In reply to Daniel P. Berrangé
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index 9291bc9209..833f20f555 100755
@@ -1499,7 +1499,7 @@ sub process_end_of_file {
 	if ($fileinfo->{action} eq "new" &&
 	    !exists $fileinfo->{facts}->{sawspdx}) {
 		if ($fileinfo->{filenew} =~
-		    /(\.(c|h|py|pl|sh|json|inc)|Makefile.*)$/) {
+		    /(\.(c|h|py|pl|sh|json|inc|rs)|Makefile.*)$/) {
 			# source code files MUST have SPDX license declared
 			ERROR("New file '" . $fileinfo->{filenew} .
 			      "' requires 'SPDX-License-Identifier'");
Signature · 1 line
2.49.0
#15 lore raw

[PULL 21/23] util/qemu-sockets: Refactor inet_parse() to use QemuOpts

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

Currently, the inet address parser cannot handle multiple options where one is prefixed with the name of the other. For example, with the 'keep-alive-idle' option added, the current parser cannot parse '127.0.0.1:5000,keep-alive-idle=60,keep-alive' correctly. Instead, it fails with "error parsing 'keep-alive' flag '-idle=60,keep-alive'".

To resolve these issues, this patch rewrites the inet address parsing using the QemuOpts parser, which the inet_parse_flag() function tries to mimic. This new parser supports all previously supported options and on top of that the 'numeric' flag is now also supported. The only difference is, the new parser produces an error if an unknown option is passed, instead of silently ignoring it.

---
 tests/unit/test-util-sockets.c | 196 +++++++++++++++++++++++++++++++++
 util/qemu-sockets.c            | 158 +++++++++++++-------------
 2 files changed, 270 insertions(+), 84 deletions(-)
tests/unit/test-util-sockets.c +196 -0
index 4c9dd0b271..9e39b92e7c 100644
@@ -332,6 +332,177 @@ static void test_socket_unix_abstract(void)
 
 #endif  /* CONFIG_LINUX */
 
+static void inet_parse_test_helper(const char *str,
+                                   InetSocketAddress *exp_addr, bool success)
+{
+    InetSocketAddress addr;
+    Error *error = NULL;
+
+    int rc = inet_parse(&addr, str, &error);
+
+    if (success) {
+        g_assert_cmpint(rc, ==, 0);
+    } else {
+        g_assert_cmpint(rc, <, 0);
+    }
+    if (exp_addr != NULL) {
+        g_assert_cmpstr(addr.host, ==, exp_addr->host);
+        g_assert_cmpstr(addr.port, ==, exp_addr->port);
+        /* Own members: */
+        g_assert_cmpint(addr.has_numeric, ==, exp_addr->has_numeric);
+        g_assert_cmpint(addr.numeric, ==, exp_addr->numeric);
+        g_assert_cmpint(addr.has_to, ==, exp_addr->has_to);
+        g_assert_cmpint(addr.to, ==, exp_addr->to);
+        g_assert_cmpint(addr.has_ipv4, ==, exp_addr->has_ipv4);
+        g_assert_cmpint(addr.ipv4, ==, exp_addr->ipv4);
+        g_assert_cmpint(addr.has_ipv6, ==, exp_addr->has_ipv6);
+        g_assert_cmpint(addr.ipv6, ==, exp_addr->ipv6);
+        g_assert_cmpint(addr.has_keep_alive, ==, exp_addr->has_keep_alive);
+        g_assert_cmpint(addr.keep_alive, ==, exp_addr->keep_alive);
+#ifdef HAVE_IPPROTO_MPTCP
+        g_assert_cmpint(addr.has_mptcp, ==, exp_addr->has_mptcp);
+        g_assert_cmpint(addr.mptcp, ==, exp_addr->mptcp);
+#endif
+    }
+
+    g_free(addr.host);
+    g_free(addr.port);
+}
+
+static void test_inet_parse_nohost_good(void)
+{
+    char host[] = "";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+    };
+    inet_parse_test_helper(":5000", &exp_addr, true);
+}
+
+static void test_inet_parse_empty_bad(void)
+{
+    inet_parse_test_helper("", NULL, false);
+}
+
+static void test_inet_parse_only_colon_bad(void)
+{
+    inet_parse_test_helper(":", NULL, false);
+}
+
+static void test_inet_parse_ipv4_good(void)
+{
+    char host[] = "127.0.0.1";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+    };
+    inet_parse_test_helper("127.0.0.1:5000", &exp_addr, true);
+}
+
+static void test_inet_parse_ipv4_noport_bad(void)
+{
+    inet_parse_test_helper("127.0.0.1", NULL, false);
+}
+
+static void test_inet_parse_ipv6_good(void)
+{
+    char host[] = "::1";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+    };
+    inet_parse_test_helper("[::1]:5000", &exp_addr, true);
+}
+
+static void test_inet_parse_ipv6_noend_bad(void)
+{
+    inet_parse_test_helper("[::1", NULL, false);
+}
+
+static void test_inet_parse_ipv6_noport_bad(void)
+{
+    inet_parse_test_helper("[::1]:", NULL, false);
+}
+
+static void test_inet_parse_ipv6_empty_bad(void)
+{
+    inet_parse_test_helper("[]:5000", NULL, false);
+}
+
+static void test_inet_parse_hostname_good(void)
+{
+    char host[] = "localhost";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+    };
+    inet_parse_test_helper("localhost:5000", &exp_addr, true);
+}
+
+static void test_inet_parse_all_options_good(void)
+{
+    char host[] = "::1";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+        .has_numeric = true,
+        .numeric =  true,
+        .has_to = true,
+        .to = 5006,
+        .has_ipv4 = true,
+        .ipv4 = false,
+        .has_ipv6 = true,
+        .ipv6 = true,
+        .has_keep_alive = true,
+        .keep_alive = true,
+#ifdef HAVE_IPPROTO_MPTCP
+        .has_mptcp = true,
+        .mptcp = false,
+#endif
+    };
+    inet_parse_test_helper(
+        "[::1]:5000,numeric=on,to=5006,ipv4=off,ipv6=on,keep-alive=on"
+#ifdef HAVE_IPPROTO_MPTCP
+        ",mptcp=off"
+#endif
+        , &exp_addr, true);
+}
+
+static void test_inet_parse_all_implicit_bool_good(void)
+{
+    char host[] = "::1";
+    char port[] = "5000";
+    InetSocketAddress exp_addr = {
+        .host = host,
+        .port = port,
+        .has_numeric = true,
+        .numeric =  true,
+        .has_to = true,
+        .to = 5006,
+        .has_ipv4 = true,
+        .ipv4 = true,
+        .has_ipv6 = true,
+        .ipv6 = true,
+        .has_keep_alive = true,
+        .keep_alive = true,
+#ifdef HAVE_IPPROTO_MPTCP
+        .has_mptcp = true,
+        .mptcp = true,
+#endif
+    };
+    inet_parse_test_helper(
+        "[::1]:5000,numeric,to=5006,ipv4,ipv6,keep-alive"
+#ifdef HAVE_IPPROTO_MPTCP
+        ",mptcp"
+#endif
+        , &exp_addr, true);
+}
+
 int main(int argc, char **argv)
 {
     bool has_ipv4, has_ipv6;
@@ -377,6 +548,31 @@ int main(int argc, char **argv)
                     test_socket_unix_abstract);
 #endif
 
+    g_test_add_func("/util/socket/inet-parse/nohost-good",
+                    test_inet_parse_nohost_good);
+    g_test_add_func("/util/socket/inet-parse/empty-bad",
+                    test_inet_parse_empty_bad);
+    g_test_add_func("/util/socket/inet-parse/only-colon-bad",
+                    test_inet_parse_only_colon_bad);
+    g_test_add_func("/util/socket/inet-parse/ipv4-good",
+                    test_inet_parse_ipv4_good);
+    g_test_add_func("/util/socket/inet-parse/ipv4-noport-bad",
+                    test_inet_parse_ipv4_noport_bad);
+    g_test_add_func("/util/socket/inet-parse/ipv6-good",
+                    test_inet_parse_ipv6_good);
+    g_test_add_func("/util/socket/inet-parse/ipv6-noend-bad",
+                    test_inet_parse_ipv6_noend_bad);
+    g_test_add_func("/util/socket/inet-parse/ipv6-noport-bad",
+                    test_inet_parse_ipv6_noport_bad);
+    g_test_add_func("/util/socket/inet-parse/ipv6-empty-bad",
+                    test_inet_parse_ipv6_empty_bad);
+    g_test_add_func("/util/socket/inet-parse/hostname-good",
+                    test_inet_parse_hostname_good);
+    g_test_add_func("/util/socket/inet-parse/all-options-good",
+                    test_inet_parse_all_options_good);
+    g_test_add_func("/util/socket/inet-parse/all-bare-bool-good",
+                    test_inet_parse_all_implicit_bool_good);
+
 end:
     return g_test_run();
 }
util/qemu-sockets.c +74 -84

195 lines not on this page · raw message

Signature · 1 line
2.49.0
#16 lore raw

[PULL 12/23] scripts/checkpatch: use new hook for file permissions check

In reply to Daniel P. Berrangé

The file permissions check is the kind of check intended to be performed in the new start of file hook.

---
 scripts/checkpatch.pl | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
scripts/checkpatch.pl +11 -8
index 817df98231..881dcac29b 100755
@@ -1448,6 +1448,17 @@ sub process_file_list {
 # Called at the start of processing a diff hunk for a file
 sub process_start_of_file {
 	my $fileinfo = shift;
+
+	# Check for incorrect file permissions
+	if ($fileinfo->{action} eq "new" && ($fileinfo->{mode} & 0111)) {
+		my $permhere = $fileinfo->{linestart} . "FILE: " .
+			$fileinfo->{filenew} . "\n";
+		if ($fileinfo->{filenew} =~
+		    /(\bMakefile(?:\.objs)?|\.(c|cc|cpp|h|mak|s|S))$/) {
+			ERROR("do not set execute permissions for source " .
+			      "files\n" . $permhere);
+		}
+	}
 }
 
 # Called at the end of processing a diff hunk for a file
@@ -1719,14 +1730,6 @@ sub process {
 
 		$cnt_lines++ if ($realcnt != 0);
 
-# Check for incorrect file permissions
-		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
-			my $permhere = $here . "FILE: $realfile\n";
-			if ($realfile =~ /(\bMakefile(?:\.objs)?|\.c|\.cc|\.cpp|\.h|\.mak|\.[sS])$/) {
-				ERROR("do not set execute permissions for source files\n" . $permhere);
-			}
-		}
-
 # Only allow Python 3 interpreter
 		if ($realline == 1 &&
 			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {
Signature · 1 line
2.49.0
#17 lore raw

[PULL 18/23] util/qemu-sockets: Refactor setting client sockopts into a separate function

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

This is done in preparation for enabling the SO_KEEPALIVE support for server sockets and adding settings for more TCP keep-alive socket options.

---
 util/qemu-sockets.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
util/qemu-sockets.c +19 -10
index 77477c1cd5..4a878e0527 100644
@@ -205,6 +205,22 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
 #endif
 }
 
+static int inet_set_sockopts(int sock, InetSocketAddress *saddr, Error **errp)
+{
+    if (saddr->keep_alive) {
+        int keep_alive = 1;
+        int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
+                             &keep_alive, sizeof(keep_alive));
+
+        if (ret < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to set keep-alive option on socket");
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static int inet_listen_saddr(InetSocketAddress *saddr,
                              int port_offset,
                              int num,
@@ -475,16 +491,9 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
         return sock;
     }
 
-    if (saddr->keep_alive) {
-        int val = 1;
-        int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
-                             &val, sizeof(val));
-
-        if (ret < 0) {
-            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
-            close(sock);
-            return -1;
-        }
+    if (inet_set_sockopts(sock, saddr, errp) < 0) {
+        close(sock);
+        return -1;
     }
 
     return sock;
Signature · 1 line
2.49.0
#18 lore raw

[PULL 17/23] io: Fix partial struct copy in qio_dns_resolver_lookup_sync_inet()

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

Commit aec21d3175 (qapi: Add InetSocketAddress member keep-alive) introduces the keep-alive flag, but this flag is not copied together with other options in qio_dns_resolver_lookup_sync_inet().

This patch fixes this issue and also prevents future ones by copying the entire structure first and only then overriding a few attributes that need to be different.

Fixes aec21d31756c (qapi: Add InetSocketAddress member keep-alive)
---
 io/dns-resolver.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)
io/dns-resolver.c +5 -16
index 53b0e8407a..3712438f82 100644
@@ -111,22 +111,11 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
                     uaddr, INET6_ADDRSTRLEN, uport, 32,
                     NI_NUMERICHOST | NI_NUMERICSERV);
 
-        newaddr->u.inet = (InetSocketAddress){
-            .host = g_strdup(uaddr),
-            .port = g_strdup(uport),
-            .has_numeric = true,
-            .numeric = true,
-            .has_to = iaddr->has_to,
-            .to = iaddr->to,
-            .has_ipv4 = iaddr->has_ipv4,
-            .ipv4 = iaddr->ipv4,
-            .has_ipv6 = iaddr->has_ipv6,
-            .ipv6 = iaddr->ipv6,
-#ifdef HAVE_IPPROTO_MPTCP
-            .has_mptcp = iaddr->has_mptcp,
-            .mptcp = iaddr->mptcp,
-#endif
-        };
+        newaddr->u.inet = *iaddr;
+        newaddr->u.inet.host = g_strdup(uaddr),
+        newaddr->u.inet.port = g_strdup(uport),
+        newaddr->u.inet.has_numeric = true,
+        newaddr->u.inet.numeric = true,
 
         (*addrs)[i] = newaddr;
     }
Signature · 1 line
2.49.0
#19 lore raw

[PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

To get a listening socket, we need to first create a socket, try binding it to a certain port, and lastly starting listening to it. Each of these operations can fail due to various reasons, one of them being that the requested address/port is already in use. In such case, the function tries the same process with a new port number.

This patch refactors the port number loop, so the success path is no longer buried inside the 'if' statements in the middle of the loop. Now, the success path is not nested and ends at the end of the iteration after successful socket creation, binding, and listening. In case any of the operations fails, it either continues to the next iteration (and the next port) or jumps out of the loop to handle the error and exits the function.

---
 util/qemu-sockets.c | 51 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)
util/qemu-sockets.c +27 -24
index 4a878e0527..329fdbfd97 100644
@@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
+            if (slisten >= 0) {
+                /*
+                 * We have a socket we tried with the previous port. It cannot
+                 * be rebound, we need to close it and create a new one.
+                 */
+                close(slisten);
+                slisten = -1;
+            }
             inet_setport(e, p);
 
             slisten = create_fast_reuse_socket(e);
             if (slisten < 0) {
-                /* First time we expect we might fail to create the socket
+                /*
+                 * First time we expect we might fail to create the socket
                  * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
                  * Later iterations should always succeed if first iteration
                  * worked though, so treat that as fatal.
@@ -317,40 +326,38 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
                 } else {
                     error_setg_errno(errp, errno,
                                      "Failed to recreate failed listening socket");
-                    goto listen_failed;
+                    goto fail;
                 }
             }
             socket_created = true;
 
             rc = try_bind(slisten, saddr, e);
             if (rc < 0) {
-                if (errno != EADDRINUSE) {
-                    error_setg_errno(errp, errno, "Failed to bind socket");
-                    goto listen_failed;
-                }
-            } else {
-                if (!listen(slisten, num)) {
-                    goto listen_ok;
+                if (errno == EADDRINUSE) {
+                    /* This port is already used, try the next one */
+                    continue;
                 }
-                if (errno != EADDRINUSE) {
-                    error_setg_errno(errp, errno, "Failed to listen on socket");
-                    goto listen_failed;
+                error_setg_errno(errp, errno, "Failed to bind socket");
+                goto fail;
+            }
+            if (listen(slisten, num)) {
+                if (errno == EADDRINUSE) {
+                    /* This port is already used, try the next one */
+                    continue;
                 }
+                error_setg_errno(errp, errno, "Failed to listen on socket");
+                goto fail;
             }
-            /* Someone else managed to bind to the same port and beat us
-             * to listen on it! Socket semantics does not allow us to
-             * recover from this situation, so we need to recreate the
-             * socket to allow bind attempts for subsequent ports:
-             */
-            close(slisten);
-            slisten = -1;
+            /* We have a listening socket */
+            freeaddrinfo(res);
+            return slisten;
         }
     }
     error_setg_errno(errp, errno,
                      socket_created ?
                      "Failed to find an available port" :
                      "Failed to create a socket");
-listen_failed:
+fail:
     saved_errno = errno;
     if (slisten >= 0) {
         close(slisten);
@@ -358,10 +365,6 @@ listen_failed:
     freeaddrinfo(res);
     errno = saved_errno;
     return -1;
-
-listen_ok:
-    freeaddrinfo(res);
-    return slisten;
 }
 
 #ifdef _WIN32
Signature · 1 line
2.49.0
#20 lore raw

Re: [PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

util/qemu-sockets.c

Quoted 28 lines · Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

To get a listening socket, we need to first create a socket, try binding it to a certain port, and lastly starting listening to it. Each of these operations can fail due to various reasons, one of them being that the requested address/port is already in use. In such case, the function tries the same process with a new port number.

This patch refactors the port number loop, so the success path is no longer buried inside the 'if' statements in the middle of the loop. Now, the success path is not nested and ends at the end of the iteration after successful socket creation, binding, and listening. In case any of the operations fails, it either continues to the next iteration (and the next port) or jumps out of the loop to handle the error and exits the function.

---
 util/qemu-sockets.c | 51 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)
util/qemu-sockets.c +0 -0
index 4a878e0527..329fdbfd97 100644

Hi; Coverity complains about this code (CID 1610306):

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 312

Quoted 11 lines
util/qemu-sockets.c +7 -0
@@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
+            if (slisten >= 0) {
+                /*
+                 * We have a socket we tried with the previous port. It cannot
+                 * be rebound, we need to close it and create a new one.
+                 */
+                close(slisten);
+                slisten = -1;

Here we set slisten to -1 ...

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 314

Quoted 2 lines
util/qemu-sockets.c +1 -0
+            }
             inet_setport(e, p);

...but then two lines later we unconditionally set slisten to something else, so the -1 assignment is overwritten without being used.

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 316

Quoted 1 line
util/qemu-sockets.c +0 -0
             slisten = create_fast_reuse_socket(e);

What was the intention here ?

thanks
-- PMM

#21 lore raw

Re: [PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

Quoted 50 lines · Peter Maydell

util/qemu-sockets.c

Quoted 28 lines · Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

To get a listening socket, we need to first create a socket, try binding it to a certain port, and lastly starting listening to it. Each of these operations can fail due to various reasons, one of them being that the requested address/port is already in use. In such case, the function tries the same process with a new port number.

This patch refactors the port number loop, so the success path is no longer buried inside the 'if' statements in the middle of the loop. Now, the success path is not nested and ends at the end of the iteration after successful socket creation, binding, and listening. In case any of the operations fails, it either continues to the next iteration (and the next port) or jumps out of the loop to handle the error and exits the function.

---
 util/qemu-sockets.c | 51 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)
util/qemu-sockets.c +0 -0
index 4a878e0527..329fdbfd97 100644

Hi; Coverity complains about this code (CID 1610306):

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 312

Quoted 11 lines
util/qemu-sockets.c +7 -0
@@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
+            if (slisten >= 0) {
+                /*
+                 * We have a socket we tried with the previous port. It cannot
+                 * be rebound, we need to close it and create a new one.
+                 */
+                close(slisten);
+                slisten = -1;

Here we set slisten to -1 ...

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 314

Quoted 2 lines
util/qemu-sockets.c +1 -0
+            }
             inet_setport(e, p);
Quoted 7 lines

...but then two lines later we unconditionally set slisten to something else, so the -1 assignment is overwritten without being used.

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 316

Quoted 1 line
util/qemu-sockets.c +0 -0
             slisten = create_fast_reuse_socket(e);

What was the intention here ?

Hi Peter!

The intention was to not leave an invalid socket number in the variable after it's closed, similarly how it was done before refactoring:
https://gitlab.com/qemu-project/qemu/-/blob/b8b5278aca78be4a1c2e7cbb11c6be176f63706d/util/qemu-sockets.c#L346

I haven't noticed it's technically not needed anymore; unless there is something added in-between in the future. I can send a patch that removes it if necessary.

Best regards

Juraj Marcin

Quoted 4 lines

thanks
-- PMM

#22 lore raw

Re: [PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

In reply to Peter Maydell
Quoted 50 lines · Peter Maydell

util/qemu-sockets.c

Quoted 28 lines · Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

To get a listening socket, we need to first create a socket, try binding it to a certain port, and lastly starting listening to it. Each of these operations can fail due to various reasons, one of them being that the requested address/port is already in use. In such case, the function tries the same process with a new port number.

This patch refactors the port number loop, so the success path is no longer buried inside the 'if' statements in the middle of the loop. Now, the success path is not nested and ends at the end of the iteration after successful socket creation, binding, and listening. In case any of the operations fails, it either continues to the next iteration (and the next port) or jumps out of the loop to handle the error and exits the function.

---
 util/qemu-sockets.c | 51 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)
util/qemu-sockets.c +0 -0
index 4a878e0527..329fdbfd97 100644

Hi; Coverity complains about this code (CID 1610306):

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 312

Quoted 11 lines
util/qemu-sockets.c +7 -0
@@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
+            if (slisten >= 0) {
+                /*
+                 * We have a socket we tried with the previous port. It cannot
+                 * be rebound, we need to close it and create a new one.
+                 */
+                close(slisten);
+                slisten = -1;

Here we set slisten to -1 ...

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 314

Quoted 2 lines
util/qemu-sockets.c +1 -0
+            }
             inet_setport(e, p);
Quoted 7 lines

...but then two lines later we unconditionally set slisten to something else, so the -1 assignment is overwritten without being used.

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 316

Quoted 1 line
util/qemu-sockets.c +0 -0
             slisten = create_fast_reuse_socket(e);

What was the intention here ?

The overwriting is correct and intentional - it is best practice to never leave a variable initialized to an FD number that is now invalid. IMHO that best practice applies, even if we are going to re-initialize the variable a short while later, because this mitigates risk from later code refactoring.

TL;DR: coverity is correctly identifying a redundant assignment, but the assignment is none the less justified.

With regards,
Daniel

Signature · 3 lines
|: 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 :|
#23 lore raw

Re: [PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

Quoted 60 lines · Daniel P. Berrangé
Quoted 50 lines · Peter Maydell

util/qemu-sockets.c

Quoted 28 lines · Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

To get a listening socket, we need to first create a socket, try binding it to a certain port, and lastly starting listening to it. Each of these operations can fail due to various reasons, one of them being that the requested address/port is already in use. In such case, the function tries the same process with a new port number.

This patch refactors the port number loop, so the success path is no longer buried inside the 'if' statements in the middle of the loop. Now, the success path is not nested and ends at the end of the iteration after successful socket creation, binding, and listening. In case any of the operations fails, it either continues to the next iteration (and the next port) or jumps out of the loop to handle the error and exits the function.

---
 util/qemu-sockets.c | 51 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 24 deletions(-)
util/qemu-sockets.c +0 -0
index 4a878e0527..329fdbfd97 100644

Hi; Coverity complains about this code (CID 1610306):

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 312

Quoted 11 lines
util/qemu-sockets.c +7 -0
@@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
+            if (slisten >= 0) {
+                /*
+                 * We have a socket we tried with the previous port. It cannot
+                 * be rebound, we need to close it and create a new one.
+                 */
+                close(slisten);
+                slisten = -1;

Here we set slisten to -1 ...

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 314

Quoted 2 lines
util/qemu-sockets.c +1 -0
+            }
             inet_setport(e, p);
Quoted 7 lines

...but then two lines later we unconditionally set slisten to something else, so the -1 assignment is overwritten without being used.

util/qemu-sockets.c static int inet_listen_saddr(InetSocketAddress *saddr, · line 316

Quoted 1 line
util/qemu-sockets.c +0 -0
             slisten = create_fast_reuse_socket(e);

What was the intention here ?

Quoted 8 lines

The overwriting is correct and intentional - it is best practice to never leave a variable initialized to an FD number that is now invalid. IMHO that best practice applies, even if we are going to re-initialize the variable a short while later, because this mitigates risk from later code refactoring.

TL;DR: coverity is correctly identifying a redundant assignment, but the assignment is none the less justified.

OK; I will mark this as a false-positive.

-- PMM

#24 lore raw

[PULL 02/23] ui/vnc: take account of client byte order in pixman format

In reply to Daniel P. Berrangé

The set_pixel_conversion() method is responsible for determining whether the VNC client pixel format matches the server format, and thus whether we can use the fast path "copy" impl for sending pixels, or must use the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT, which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting the VNC pixel format into a pixman format.

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the VNC client endianness, and is thus returning a pixman format that is only valid with the host endianness matches that of the VNC client.

This has been broken since pixman was introduced to the VNC server:

commit 9f64916da20eea67121d544698676295bbb105a7
Author: Gerd Hoffmann <[email protected]>
Date: Wed Oct 10 13:29:43 2012 +0200

pixman/vnc: use pixman images in vnc.

The flaw can be demonstrated using the Tigervnc client by using

vncviewer -AutoSelect=0 -PreferredEncoding=raw server:display

connecting from a LE client to a QEMU on a BE server, or the reverse.

The bug was masked, however, because almost all VNC clients will advertize support for the "tight" encoding and the QEMU VNC server will prefer "tight" if advertized.

The tight_pack24 method is responsible for taking a set of pixels which have already been converted into client endianness and then repacking them into the TPIXEL format which the RFB spec defines as

"TPIXEL is only 3 bytes long, where the first byte is the
red component, the second byte is the green component, and the third byte is the blue component of the pixel color value"

IOW, the TPIXEL format is fixed on the wire, regardless of what the VNC client declare as its endianness.

Since the VNC pixel encoding code was failing to honour the endian flag of the client, the tight_pack24 method was always operating on data in native endianness. Its impl cancelled out the VNC pixel encoding bug.

With the VNC pixel encoding code now fixed, the tight_pack24 method needs to take into account that it is operating on data in client endianness, not native endianness. It thus may need to invert the pixel shifts.

---
 include/ui/qemu-pixman.h |  4 ++--
 ui/qemu-pixman.c         | 15 ++++++++-------
 ui/vnc-enc-tight.c       |  2 +-
 ui/vnc.c                 |  3 ++-
 4 files changed, 13 insertions(+), 11 deletions(-)
include/ui/qemu-pixman.h +2 -2
index 193bc046d1..2ca0ed7029 100644
@@ -75,12 +75,12 @@ PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
 pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
 pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
 uint32_t qemu_pixman_to_drm_format(pixman_format_code_t pixman);
-int qemu_pixman_get_type(int rshift, int gshift, int bshift);
+int qemu_pixman_get_type(int rshift, int gshift, int bshift, int endian);
 bool qemu_pixman_check_format(DisplayChangeListener *dcl,
                               pixman_format_code_t format);
 
 #ifdef CONFIG_PIXMAN
-pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
+pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf, int endian);
 pixman_image_t *qemu_pixman_linebuf_create(pixman_format_code_t format,
                                            int width);
 void qemu_pixman_linebuf_fill(pixman_image_t *linebuf, pixman_image_t *fb,
ui/qemu-pixman.c +8 -7
index 6ef4376f4e..ef4e71da11 100644
@@ -126,33 +126,34 @@ uint32_t qemu_pixman_to_drm_format(pixman_format_code_t pixman_format)
     return 0;
 }
 
-int qemu_pixman_get_type(int rshift, int gshift, int bshift)
+int qemu_pixman_get_type(int rshift, int gshift, int bshift, int endian)
 {
     int type = PIXMAN_TYPE_OTHER;
+    bool native_endian = (endian == G_BYTE_ORDER);
 
     if (rshift > gshift && gshift > bshift) {
         if (bshift == 0) {
-            type = PIXMAN_TYPE_ARGB;
+            type = native_endian ? PIXMAN_TYPE_ARGB : PIXMAN_TYPE_BGRA;
         } else {
-            type = PIXMAN_TYPE_RGBA;
+            type = native_endian ? PIXMAN_TYPE_RGBA : PIXMAN_TYPE_ABGR;
         }
     } else if (rshift < gshift && gshift < bshift) {
         if (rshift == 0) {
-            type = PIXMAN_TYPE_ABGR;
+            type = native_endian ? PIXMAN_TYPE_ABGR : PIXMAN_TYPE_RGBA;
         } else {
-            type = PIXMAN_TYPE_BGRA;
+            type = native_endian ? PIXMAN_TYPE_BGRA : PIXMAN_TYPE_ARGB;
         }
     }
     return type;
 }
 
 #ifdef CONFIG_PIXMAN
-pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf)
+pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf, int endian)
 {
     pixman_format_code_t format;
     int type;
 
-    type = qemu_pixman_get_type(pf->rshift, pf->gshift, pf->bshift);
+    type = qemu_pixman_get_type(pf->rshift, pf->gshift, pf->bshift, endian);
     format = PIXMAN_FORMAT(pf->bits_per_pixel, type,
                            pf->abits, pf->rbits, pf->gbits, pf->bbits);
     if (!pixman_format_supported_source(format)) {
ui/vnc-enc-tight.c +1 -1
index f8aaa8f346..a5bdc19ebb 100644
@@ -891,7 +891,7 @@ static void tight_pack24(VncState *vs, uint8_t *buf, size_t count, size_t *ret)
 
     buf8 = buf;
 
-    if (1 /* FIXME */) {
+    if (vs->client_endian == G_BYTE_ORDER) {
         rshift = vs->client_pf.rshift;
         gshift = vs->client_pf.gshift;
         bshift = vs->client_pf.bshift;
ui/vnc.c +2 -1
index ab18172c4d..d095cd7da3 100644
@@ -2240,7 +2240,8 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
 
 static void set_pixel_conversion(VncState *vs)
 {
-    pixman_format_code_t fmt = qemu_pixman_get_format(&vs->client_pf);
+    pixman_format_code_t fmt = qemu_pixman_get_format(&vs->client_pf,
+                                                      vs->client_endian);
 
     if (fmt == VNC_SERVER_FB_FORMAT) {
         vs->write_pixels = vnc_write_pixels_copy;
Signature · 1 line
2.49.0
#25 lore raw

Re: [PULL 02/23] ui/vnc: take account of client byte order in pixman format

Quoted 11 lines · Daniel P. Berrangé

The set_pixel_conversion() method is responsible for determining whether the VNC client pixel format matches the server format, and thus whether we can use the fast path "copy" impl for sending pixels, or must use the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT, which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting the VNC pixel format into a pixman format.

Quoted 7 lines

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the VNC client endianness, and is thus returning a pixman format that is only valid with the host endianness matches that of the VNC client.

...

Hi Daniel,

this patch breaks the output in the TigerVNC viewer for me.
If I run "./qemu-system-x86_64 -vnc :1" on my laptop, and then connect to it via "vncviewer :1", the output of the BIOS now appears in yellow letters (instead of grey ones).

FWIW, the output of TigerVNC viewer is:

TigerVNC viewer v1.15.0
Built on: 2025-04-08 00:00
Copyright (C) 1999-2025 TigerVNC team and many others (see README.rst) See https://www.tigervnc.org for information on TigerVNC.

Tue Jun 3 13:17:50 2025
DecodeManager: Detected 16 CPU core(s)
DecodeManager: Creating 4 decoder thread(s)
CConn: Connected to host localhost port 5901
CConnection: Server supports RFB protocol version 3.8
CConnection: Using RFB protocol version 3.8
CConnection: Choosing security type None(1)
CConn: Using pixel format depth 24 (32bpp) little-endian rgb888

Could you please have a look what's going wrong here?

Thanks,
Thomas

#26 lore raw

Re: [PULL 02/23] ui/vnc: take account of client byte order in pixman format

Quoted 44 lines · Thomas Huth
Quoted 11 lines · Daniel P. Berrangé

The set_pixel_conversion() method is responsible for determining whether the VNC client pixel format matches the server format, and thus whether we can use the fast path "copy" impl for sending pixels, or must use the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT, which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting the VNC pixel format into a pixman format.

Quoted 7 lines

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the VNC client endianness, and is thus returning a pixman format that is only valid with the host endianness matches that of the VNC client.

...

Hi Daniel,

this patch breaks the output in the TigerVNC viewer for me.
If I run "./qemu-system-x86_64 -vnc :1" on my laptop, and then connect to it via "vncviewer :1", the output of the BIOS now appears in yellow letters (instead of grey ones).

FWIW, the output of TigerVNC viewer is:

TigerVNC viewer v1.15.0
Built on: 2025-04-08 00:00
Copyright (C) 1999-2025 TigerVNC team and many others (see README.rst) See https://www.tigervnc.org for information on TigerVNC.

Tue Jun 3 13:17:50 2025
DecodeManager: Detected 16 CPU core(s)
DecodeManager: Creating 4 decoder thread(s)
CConn: Connected to host localhost port 5901
CConnection: Server supports RFB protocol version 3.8
CConnection: Using RFB protocol version 3.8
CConnection: Choosing security type None(1)
CConn: Using pixel format depth 24 (32bpp) little-endian rgb888

Quoted 1 line

Could you please have a look what's going wrong here?

Yes, I can reproduce too, will check this out.

With regards,
Daniel

Signature · 3 lines
|: 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 :|
#27 lore raw

Re: [PULL 02/23] ui/vnc: take account of client byte order in pixman format

In reply to Thomas Huth
Quoted 19 lines · Thomas Huth
Quoted 11 lines · Daniel P. Berrangé

The set_pixel_conversion() method is responsible for determining whether the VNC client pixel format matches the server format, and thus whether we can use the fast path "copy" impl for sending pixels, or must use the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT, which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting the VNC pixel format into a pixman format.

Quoted 7 lines

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the VNC client endianness, and is thus returning a pixman format that is only valid with the host endianness matches that of the VNC client.

Quoted 8 lines

...

Hi Daniel,

this patch breaks the output in the TigerVNC viewer for me.
If I run "./qemu-system-x86_64 -vnc :1" on my laptop, and then connect to it via "vncviewer :1", the output of the BIOS now appears in yellow letters (instead of grey ones).

It turns out that historically we never set the 'client_be' flag when a client does NOT send a "set pixel format" message. By luck this was OK for little endian platforms as the default value of 0 matched little endian.

When I replaced 'client_be' with "client_endian", the default value of 0 matches neither big or little endian.

I didn't see this with remote-viewer as it unconditionally sends "set pixel format", but tigervnc always uses the server's default pixel format.

So this patch is fine, but it exposes a pre-existing latent bug there was probably causing problems on big endian platforms in the past, but now causes problems on little endian platforms.

With regards,
Daniel

Signature · 3 lines
|: 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 :|
#28 lore raw

Re: [PULL 02/23] ui/vnc: take account of client byte order in pixman format

Quoted 37 lines · Daniel P. Berrangé
Quoted 19 lines · Thomas Huth
Quoted 11 lines · Daniel P. Berrangé

The set_pixel_conversion() method is responsible for determining whether the VNC client pixel format matches the server format, and thus whether we can use the fast path "copy" impl for sending pixels, or must use the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT, which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting the VNC pixel format into a pixman format.

Quoted 7 lines

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the VNC client endianness, and is thus returning a pixman format that is only valid with the host endianness matches that of the VNC client.

Quoted 8 lines

...

Hi Daniel,

this patch breaks the output in the TigerVNC viewer for me.
If I run "./qemu-system-x86_64 -vnc :1" on my laptop, and then connect to it via "vncviewer :1", the output of the BIOS now appears in yellow letters (instead of grey ones).

It turns out that historically we never set the 'client_be' flag when a client does NOT send a "set pixel format" message. By luck this was OK for little endian platforms as the default value of 0 matched little endian.

When I replaced 'client_be' with "client_endian", the default value of 0 matches neither big or little endian.

Quoted 7 lines

I didn't see this with remote-viewer as it unconditionally sends "set pixel format", but tigervnc always uses the server's default pixel format.

So this patch is fine, but it exposes a pre-existing latent bug there was probably causing problems on big endian platforms in the past, but now causes problems on little endian platforms.

Nice :)

#29 lore raw

[PULL 09/23] scripts/checkpatch.pl: fix various indentation mistakes

In reply to Daniel P. Berrangé

Various checks in the code were under-indented relative to other surrounding code. Some places used 4-space indents instead of single tab, while other places simply used too few tabs.

---
 scripts/checkpatch.pl | 101 ++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 49 deletions(-)
scripts/checkpatch.pl +52 -49
index d355c0dad5..2c19f6f0f2 100755
@@ -1681,19 +1681,20 @@ sub process {
 
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
-		    &checkspdx($realfile, $1);
+			&checkspdx($realfile, $1);
 		}
 
 		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
-		    my $tag = $1;
-		    my @permitted = qw(
-			SPDX-License-Identifier
-		    );
-
-		    unless (grep { /^$tag$/ } @permitted) {
-			ERROR("Tag $tag not permitted in QEMU code, valid " .
-			      "choices are: " . join(", ", @permitted));
-		    }
+			my $tag = $1;
+			my @permitted = qw(
+				SPDX-License-Identifier
+			);
+
+			unless (grep { /^$tag$/ } @permitted) {
+			    ERROR("Tag $tag not permitted in QEMU code, " .
+				  "valid choices are: " .
+				  join(", ", @permitted));
+			}
 		}
 
 # Check for wrappage within a valid hunk of the file
@@ -2274,7 +2275,7 @@ sub process {
 
 # missing space after union, struct or enum definition
 		if ($line =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?(?:\s+$Ident)?[=\{]/) {
-		    ERROR("missing space after $1 definition\n" . $herecurr);
+			ERROR("missing space after $1 definition\n" . $herecurr);
 		}
 
 # check for spacing round square brackets; allowed:
@@ -2569,7 +2570,7 @@ sub process {
 
 		if ($line =~ /^.\s*(Q(?:S?LIST|SIMPLEQ|TAILQ)_HEAD)\s*\(\s*[^,]/ &&
 		    $line !~ /^.typedef/) {
-		    ERROR("named $1 should be typedefed separately\n" . $herecurr);
+			ERROR("named $1 should be typedefed separately\n" . $herecurr);
 		}
 
 # Need a space before open parenthesis after if, while etc
@@ -3118,48 +3119,50 @@ sub process {
 
 # Qemu error function tests
 
-	# Find newlines in error messages
-	my $qemu_error_funcs = qr{error_setg|
-				error_setg_errno|
-				error_setg_win32|
-				error_setg_file_open|
-				error_set|
-				error_prepend|
-				warn_reportf_err|
-				error_reportf_err|
-				error_vreport|
-				warn_vreport|
-				info_vreport|
-				error_report|
-				warn_report|
-				info_report|
-				g_test_message}x;
-
-	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
-		ERROR("Error messages should not contain newlines\n" . $herecurr);
-	}
+		# Find newlines in error messages
+		my $qemu_error_funcs = qr{error_setg|
+					 error_setg_errno|
+					 error_setg_win32|
+					 error_setg_file_open|
+					 error_set|
+					 error_prepend|
+					 warn_reportf_err|
+					 error_reportf_err|
+					 error_vreport|
+					 warn_vreport|
+					 info_vreport|
+					 error_report|
+					 warn_report|
+					 info_report|
+					 g_test_message}x;
+
+		if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
+			ERROR("Error messages should not contain newlines\n" . $herecurr);
+		}
 
-	# Continue checking for error messages that contains newlines. This
-	# check handles cases where string literals are spread over multiple lines.
-	# Example:
-	# error_report("Error msg line #1"
-	#              "Error msg line #2\n");
-	my $quoted_newline_regex = qr{\+\s*\".*\\n.*\"};
-	my $continued_str_literal = qr{\+\s*\".*\"};
+		# Continue checking for error messages that contains newlines.
+		# This check handles cases where string literals are spread
+		# over multiple lines.
+		# Example:
+		# error_report("Error msg line #1"
+		#              "Error msg line #2\n");
+		my $quoted_newline_regex = qr{\+\s*\".*\\n.*\"};
+		my $continued_str_literal = qr{\+\s*\".*\"};
 
-	if ($rawline =~ /$quoted_newline_regex/) {
-		# Backtrack to first line that does not contain only a quoted literal
-		# and assume that it is the start of the statement.
-		my $i = $linenr - 2;
+		if ($rawline =~ /$quoted_newline_regex/) {
+			# Backtrack to first line that does not contain only
+			# a quoted literal and assume that it is the start
+			# of the statement.
+			my $i = $linenr - 2;
 
-		while (($i >= 0) & $rawlines[$i] =~ /$continued_str_literal/) {
-			$i--;
-		}
+			while (($i >= 0) & $rawlines[$i] =~ /$continued_str_literal/) {
+				$i--;
+			}
 
-		if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
-			ERROR("Error messages should not contain newlines\n" . $herecurr);
+			if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
+				ERROR("Error messages should not contain newlines\n" . $herecurr);
+			}
 		}
-	}
 
 # check for non-portable libc calls that have portable alternatives in QEMU
 		if ($line =~ /\bffs\(/) {
Signature · 1 line
2.49.0
#30 lore raw

[PULL 15/23] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

In reply to Daniel P. Berrangé

Going forward we want all newly created source files to have an SPDX-License-Identifier tag present.

Initially mandate this for C, Python, Perl, Shell source files, as well as JSON (QAPI) and Makefiles, while encouraging users to consider it for other file types.

The new attempt at detecting missing SPDX-License-Identifier relies on the hooks for relying triggering logic at the end of scanning a new file in the diff.

---
 scripts/checkpatch.pl | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
scripts/checkpatch.pl +15 -0
index c05559a108..da13102eda 100755
@@ -1483,6 +1483,20 @@ sub process_start_of_file {
 # Called at the end of processing a diff hunk for a file
 sub process_end_of_file {
 	my $fileinfo = shift;
+
+	if ($fileinfo->{action} eq "new" &&
+	    !exists $fileinfo->{facts}->{sawspdx}) {
+		if ($fileinfo->{filenew} =~
+		    /(\.(c|h|py|pl|sh|json|inc)|Makefile.*)$/) {
+			# source code files MUST have SPDX license declared
+			ERROR("New file '" . $fileinfo->{filenew} .
+			      "' requires 'SPDX-License-Identifier'");
+		} else {
+			# Other files MAY have SPDX license if appropriate
+			WARN("Does new file '" . $fileinfo->{filenew} .
+			     "' need 'SPDX-License-Identifier'?");
+		}
+	}
 }
 
 sub process {
@@ -1781,6 +1795,7 @@ sub process {
 
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
+			$fileinfo->{facts}->{sawspdx} = 1;
 			&checkspdx($realfile, $1);
 		}
 
Signature · 1 line
2.49.0
#31 lore raw

[PULL 20/23] util/qemu-sockets: Add support for keep-alive flag to passive sockets

In reply to Daniel P. Berrangé

From: Juraj Marcin <[email protected]>

Commit aec21d3175 (qapi: Add InetSocketAddress member keep-alive) introduces the keep-alive flag, which enables the SO_KEEPALIVE socket option, but only on client-side sockets. However, this option is also useful for server-side sockets, so they can check if a client is still reachable or drop the connection otherwise.

This patch enables the SO_KEEPALIVE socket option on passive server-side sockets if the keep-alive flag is enabled. This socket option is then inherited by active server-side sockets communicating with connected clients.

---
 qapi/sockets.json   | 4 ++--
 util/qemu-sockets.c | 9 +++------
 2 files changed, 5 insertions(+), 8 deletions(-)
qapi/sockets.json +2 -2
index 6a95023315..62797cd027 100644
@@ -56,8 +56,8 @@
 # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and
 #     IPv6
 #
-# @keep-alive: enable keep-alive when connecting to this socket.  Not
-#     supported for passive sockets.  (Since 4.2)
+# @keep-alive: enable keep-alive when connecting to/listening on this socket.
+#     (Since 4.2, not supported for listening sockets until 10.1)
 #
 # @mptcp: enable multi-path TCP.  (Since 6.1)
 #
util/qemu-sockets.c +3 -6
index 329fdbfd97..4fbf1ed5bf 100644
@@ -236,12 +236,6 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
     int saved_errno = 0;
     bool socket_created = false;
 
-    if (saddr->keep_alive) {
-        error_setg(errp, "keep-alive option is not supported for passive "
-                   "sockets");
-        return -1;
-    }
-
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_PASSIVE;
     if (saddr->has_numeric && saddr->numeric) {
@@ -349,6 +343,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
                 goto fail;
             }
             /* We have a listening socket */
+            if (inet_set_sockopts(slisten, saddr, errp) < 0) {
+                goto fail;
+            }
             freeaddrinfo(res);
             return slisten;
         }
Signature · 1 line
2.49.0