Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On Sat, Jan 15, 2022 at 02:22 Laine Stump wrote: > On 1/14/22 3:29 PM, Ján Tomko wrote: > > On a Friday in 2022, Laine Stump wrote: > >> Since it's Friday and we're talking about personal preferences - I > >> personally dislike the use of i and j (and anything else with a single > >> letter) as variable names, because it makes using a text search for > >> occurences pointless. Sure, longer variable names could also be a > >> substring of something else, and any variable could be re-used > >> elsewhere, but even then a search is mildly usable. > > > > Well, you need to search for the word i instead of the letter i. > > > > grep has the '-w' switch for that, or you can specify some boundaries: > > \bi\b > > \ > > > > vim searches for the word under the cursor with '*' by default > > > > Surely other search tools have some equivalent. > > This forced me to go look for it in emacs, and after 28 years, I've > learned about isearch-forward-symbol-at-point, which is by default bound > to [alt-s .]. But that's just another different keystroke I have to > remember. Much easier if I can just use an expansion of the ctl-s > (incremental search) that I already know and use for pretty much all > searching within a single file. Haha ! I use emacs as well and I never knew about this too. Will try it too. Thanks!
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On 1/14/22 3:29 PM, Ján Tomko wrote: On a Friday in 2022, Laine Stump wrote: Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. Well, you need to search for the word i instead of the letter i. grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \ vim searches for the word under the cursor with '*' by default Surely other search tools have some equivalent. This forced me to go look for it in emacs, and after 28 years, I've learned about isearch-forward-symbol-at-point, which is by default bound to [alt-s .]. But that's just another different keystroke I have to remember. Much easier if I can just use an expansion of the ctl-s (incremental search) that I already know and use for pretty much all searching within a single file. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-)) II thank you. JJano KKind of you, LLaine
Re: [libvirt PATCH 0/3] Bump some min versions
On a Friday in 2022, Daniel P. Berrangé wrote: Given previous platforms we've dropped, we can bump the min compilers. Daniel P. Berrangé (3): configure: bump min required GCC to 7.4.0 configure: bump min required CLang to 6.0 / XCode 10.0 examples: drop some conditionals checks from macros config.h | 14 +++--- examples/c/misc/event-test.c | 14 ++ 2 files changed, 9 insertions(+), 19 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On a Friday in 2022, Laine Stump wrote: Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. Well, you need to search for the word i instead of the letter i. grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \ vim searches for the word under the cursor with '*' by default Surely other search tools have some equivalent. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-)) II thank you. JJano signature.asc Description: PGP signature
[libvirt PATCH 5/5] tests: add a test for selecting a firmware without NVRAM
This demonstrates that when the XML config contains
the firmware auto-selection code will ignore the high priority pflash
OVMF builds tagged with the 'amd-sev' feature, and instead pick the
ROM builds without a varstore.
Signed-off-by: Daniel P. Berrangé
---
.../os-firmware-efi-sev.x86_64-6.0.0.args | 43 +++
.../qemuxml2argvdata/os-firmware-efi-sev.xml | 74 +++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 118 insertions(+)
create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-sev.x86_64-6.0.0.args
create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-sev.xml
diff --git a/tests/qemuxml2argvdata/os-firmware-efi-sev.x86_64-6.0.0.args
b/tests/qemuxml2argvdata/os-firmware-efi-sev.x86_64-6.0.0.args
new file mode 100644
index 00..fdb64fef75
--- /dev/null
+++ b/tests/qemuxml2argvdata/os-firmware-efi-sev.x86_64-6.0.0.args
@@ -0,0 +1,43 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-fedora \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-fedora/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-fedora/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-fedora/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=fedora,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-fedora/master-key.aes"}'
\
+-machine
pc-q35-4.0,usb=off,dump-guest-core=off,confidential-guest-support=lsec0,memory-backend=pc.ram
\
+-accel kvm \
+-cpu qemu64 \
+-bios /usr/share/OVMF/OVMF.sev.fd \
+-m 8 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":8388608}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-global ICH9-LPC.disable_s3=0 \
+-global ICH9-LPC.disable_s4=1 \
+-boot menu=on,strict=on \
+-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
+-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
+-device ioh3420,port=8,chassis=3,id=pci.3,bus=pcie.0,addr=0x1 \
+-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \
+-device
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,addr=0x1d
\
+-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \
+-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 \
+-object
'{"qom-type":"sev-guest","id":"lsec0","cbitpos":47,"reduced-phys-bits":1,"policy":1,"dh-cert-file":"/tmp/lib/domain--1-fedora/dh_cert.base64","session-file":"/tmp/lib/domain--1-fedora/session.base64"}'
\
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/os-firmware-efi-sev.xml
b/tests/qemuxml2argvdata/os-firmware-efi-sev.xml
new file mode 100644
index 00..eb8292b59d
--- /dev/null
+++ b/tests/qemuxml2argvdata/os-firmware-efi-sev.xml
@@ -0,0 +1,74 @@
+
+ fedora
+ 63840878-0deb-4095-97e6-fc444d9bc9fa
+ 8192
+ 8192
+ 1
+
+hvm
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+
+
+
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+47
+1
+0x0001
+AQAOQAOQAOQAOQAOAAA
+IHAVENOIDEABUTJUSTPROVIDINGASTRING
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index cc67d806e4..16765f2471 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3455,6 +3455,7 @@ mymain(void)
DO_TEST_CAPS_LATEST("os-firmware-efi");
DO_TEST_CAPS_LATEST("os-firmware-efi-secboot");
DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys");
+DO_TEST_CAPS_VER("os-firmware-efi-sev", "6.0.0");
DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64");
DO_TEST_CAPS_LATEST("vhost-user-vga");
--
2.33.1
[libvirt PATCH 4/5] tests: add firmware descriptor for SEV dedicated build
This is different from most OVMF firmware builds in that there
is no separate NVRAM variables store. The main image is readonly
and does not persist variables. As such it uses the old style
-bios config with QEMU rather than pflash.
Signed-off-by: Daniel P. Berrangé
---
.../usr/share/qemu/firmware/62-ovmf-sev.json | 27 +++
tests/qemufirmwaretest.c | 4 ++-
2 files changed, 30 insertions(+), 1 deletion(-)
create mode 100644
tests/qemufirmwaredata/usr/share/qemu/firmware/62-ovmf-sev.json
diff --git a/tests/qemufirmwaredata/usr/share/qemu/firmware/62-ovmf-sev.json
b/tests/qemufirmwaredata/usr/share/qemu/firmware/62-ovmf-sev.json
new file mode 100644
index 00..02e5e1dae8
--- /dev/null
+++ b/tests/qemufirmwaredata/usr/share/qemu/firmware/62-ovmf-sev.json
@@ -0,0 +1,27 @@
+{
+"description": "OVMF for x86_64, with SEV, without SB, without SMM, with
NO varstore",
+"interface-types": [
+"uefi"
+],
+"mapping": {
+"device": "memory",
+"filename": "/usr/share/OVMF/OVMF.sev.fd"
+},
+"targets": [
+{
+"architecture": "x86_64",
+"machines": [
+"pc-q35-*"
+]
+}
+],
+"features": [
+"acpi-s3",
+"amd-sev",
+"amd-sev-es",
+"verbose-dynamic"
+],
+"tags": [
+
+]
+}
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
index cad4b6d383..45c27554f6 100644
--- a/tests/qemufirmwaretest.c
+++ b/tests/qemufirmwaretest.c
@@ -62,6 +62,7 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED)
SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json",
PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json",
PREFIX "/share/qemu/firmware/61-ovmf.json",
+PREFIX "/share/qemu/firmware/62-ovmf-sev.json",
PREFIX "/share/qemu/firmware/70-aavmf.json",
NULL
};
@@ -250,7 +251,8 @@ mymain(void)
DO_SUPPORTED_TEST("pc-q35-3.1", VIR_ARCH_X86_64, true,
"/usr/share/seabios/bios-256k.bin:NULL:"
"/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.secboot.fd:"
-
"/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
+
"/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:"
+ "/usr/share/OVMF/OVMF.sev.fd:NULL",
VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS,
VIR_DOMAIN_OS_DEF_FIRMWARE_EFI);
DO_SUPPORTED_TEST("pc-q35-3.1", VIR_ARCH_I686, false,
--
2.33.1
[libvirt PATCH 3/5] qemu: filter firmware selection based on loader type
If the '' type attribute is set, then use this to filter
the available firmware files. This allows forcing use of a firmware
with or without NVRAM, where both options are available. This will
be used for AMD SEV when doing a measured boot, where NVRAM must
be forbidden.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_firmware.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index 84c80eaacb..2c3b28ae13 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1070,6 +1070,31 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
return false;
}
+if (def->os.loader) {
+VIR_DEBUG("Check loader type '%s' match for device '%s'",
+ virDomainLoaderTypeToString(def->os.loader->type),
+ qemuFirmwareDeviceTypeToString(fw->mapping.device));
+switch (def->os.loader->type) {
+case VIR_DOMAIN_LOADER_TYPE_NONE:
+break;
+
+case VIR_DOMAIN_LOADER_TYPE_ROM:
+if (fw->mapping.device != QEMU_FIRMWARE_DEVICE_MEMORY)
+return false;
+break;
+
+case VIR_DOMAIN_LOADER_TYPE_PFLASH:
+if (fw->mapping.device != QEMU_FIRMWARE_DEVICE_FLASH)
+return false;
+break;
+
+case VIR_DOMAIN_LOADER_TYPE_LAST:
+break;
+}
+} else {
+VIR_DEBUG("Skip loader type match");
+}
+
if (def->sec) {
switch ((virDomainLaunchSecurity) def->sec->sectype) {
case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
--
2.33.1
[libvirt PATCH 2/5] conf: parse loader 'type' even when doing firmware auto select
The loader 'type' is a property that is useful to filter on when selecting firmware. For example, with AMD SEV it is desirable to be able to request selecting of firmware without NVRAM using: Signed-off-by: Daniel P. Berrangé --- docs/formatdomain.rst | 12 src/conf/domain_conf.c | 8 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index cd818c1ded..3c4ee70835 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -149,6 +149,16 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. ... + + ... + + hvm + + + + ... + ``firmware`` The ``firmware`` attribute allows management applications to automatically fill and elements and possibly enable some @@ -219,6 +229,8 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. firmwares may implement the Secure boot feature. Some UEFI images intended for use with confidential computing environments like AMD SEV will disable persistence of variables, and would thus require ``type`` to be ``rom``. + If set, the ``type`` attribute will also influence what firmware path is + used when firmware auto-select is performed. :since:`Since 8.1.0`. Attribute ``secure`` can be used to tell the hypervisor that the firmware is capable of Secure Boot feature. It cannot be used to enable or disable the feature itself in the firmware. diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a805f7f6a3..4f0d8e27cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18044,10 +18044,6 @@ virDomainLoaderDefParseXML(xmlNodePtr node, >readonly) < 0) return -1; -if (virXMLPropEnum(node, "type", virDomainLoaderTypeFromString, - VIR_XML_PROP_NONZERO, >type) < 0) -return -1; - if (!(loader->path = virXMLNodeContentString(node))) return -1; @@ -18055,6 +18051,10 @@ virDomainLoaderDefParseXML(xmlNodePtr node, VIR_FREE(loader->path); } +if (virXMLPropEnum(node, "type", virDomainLoaderTypeFromString, + VIR_XML_PROP_NONZERO, >type) < 0) +return -1; + if (virXMLPropTristateBool(node, "secure", VIR_XML_PROP_NONE, >secure) < 0) return -1; -- 2.33.1
[libvirt PATCH 1/5] docs: explain that some UEFI images can use 'rom' instead of 'pflash'
The normal requirements for UEFI firmware images are to support persistence of variables, either in the main image, or more typically in a separate NVRAM file. In a confidential computing environment, however, persistence of variables can cause trust issues and prevent measurement of the firmware during boot up. For these scenarios some UEFI images will disable persistence of variables. To use such images the loader type must be set to 'rom' instead of 'pflash'. Signed-off-by: Daniel P. Berrangé --- docs/formatdomain.rst | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c0b2d935f3..cd818c1ded 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -214,10 +214,14 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the fact that the image should be writable or read-only. The second attribute ``type`` accepts values ``rom`` and ``pflash``. It tells the hypervisor where in the guest memory the file should be mapped. For instance, if the loader - path points to an UEFI image, ``type`` should be ``pflash``. Moreover, some - firmwares may implement the Secure boot feature. Attribute ``secure`` can be - used to tell the hypervisor that the firmware is capable of Secure Boot feature. - It cannot be used to enable or disable the feature itself in the firmware. + path points to an UEFI image, ``type`` would normally be ``pflash`` to + enable support for persistence of firmware variables. Moreover, some + firmwares may implement the Secure boot feature. Some UEFI images intended + for use with confidential computing environments like AMD SEV will disable + persistence of variables, and would thus require ``type`` to be ``rom``. + Attribute ``secure`` can be used to tell the hypervisor that the firmware + is capable of Secure Boot feature. It cannot be used to enable or disable + the feature itself in the firmware. :since:`Since 2.1.0` ``nvram`` Some UEFI firmwares may want to use a non-volatile memory to store some -- 2.33.1
[libvirt PATCH 0/5] Support AMD SEV firmware with -bios instead of pflash
The firmware distros have given people for use with AMD SEV thus far has just been one of the regular OVMF builds. This is sufficient for booting a guest with SEV enabled, but is useless if you want to actually validate the guest measurement. The NVRAM store is untrustworthy since it is not included in the measurement. We need to supply a dedicated build of OVMF without NVRAM support enabled. While it is possible to use with pflash, we then get a problem with firmware selection as there is no easy way to make it prefer the firmware without NVRAM. Also the firmware descriptor treats the NVRAM template as a mandatory field today and libvirt enforces that. While we could invent a new feature flag 'sev-stateless' for the firmware descriptors, and/or make the NVRAM template path optional, it makes more sense if the firmware descriptor just reports the SEV firmware as type=memory instead of type=flash. If the libvirt XML parses the attribute when doing firmware auto-selection, we trivially enable a way for a mgmt app to indicate that it wants the SEV firmware without NVRAM support. This series does all the plumbing we need. The only minor issue is that QEMU support for -bios with SEV enabled firmware is broken: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02957.html Daniel P. Berrangé (5): docs: explain that some UEFI images can use 'rom' instead of 'pflash' conf: parse loader 'type' even when doing firmware auto select qemu: filter firmware selection based on loader type tests: add firmware descriptor for SEV dedicated build tests: add a test for selecting a firmware without NVRAM docs/formatdomain.rst | 24 +- src/conf/domain_conf.c| 8 +- src/qemu/qemu_firmware.c | 25 +++ .../usr/share/qemu/firmware/62-ovmf-sev.json | 27 +++ tests/qemufirmwaretest.c | 4 +- .../os-firmware-efi-sev.x86_64-6.0.0.args | 43 +++ .../qemuxml2argvdata/os-firmware-efi-sev.xml | 74 +++ tests/qemuxml2argvtest.c | 1 + 8 files changed, 197 insertions(+), 9 deletions(-) create mode 100644 tests/qemufirmwaredata/usr/share/qemu/firmware/62-ovmf-sev.json create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-sev.x86_64-6.0.0.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-sev.xml -- 2.33.1
[libvirt PATCH 2/3] configure: bump min required CLang to 6.0 / XCode 10.0
Several distros have been dropped since the last time we bumped the minimum required CLang version. Per repology, currently shipping versions are: RHEL-8: 10.0.1 Debian Buster: 7.0.1 openSUSE Leap 15.2: 9.0.1 Ubuntu LTS 18.04: 6.0.0 Ubuntu LTS 20.04: 10.0.0 FreeBSD 12: 8.0.1 Fedora 33: 11.0.0 Fedora 34: 11.1.0 With this list Ubuntu LTS 18.04 is the constraint at 6.0.0 An LLVM version of 6.0.0 corresponds to macOS XCode version of 10.0 which dates from Sept 2018. Signed-off-by: Daniel P. Berrangé --- config.h | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/config.h b/config.h index 2ef32c8627..cc887db222 100644 --- a/config.h +++ b/config.h @@ -36,12 +36,12 @@ #if defined(__clang_major__) && defined(__clang_minor__) # ifdef __apple_build_version__ -# if __clang_major__ < 5 || (__clang_major__ == 5 && __clang_minor__ < 1) -# error You need at least XCode Clang v5.1 to compile libvirt +# if __clang_major__ < 10 || (__clang_major__ == 10 && __clang_minor__ < 0) +# error You need at least XCode Clang v10.0 to compile libvirt # endif # else -# if __clang_major__ < 3 || (__clang_major__ == 3 && __clang_minor__ < 4) -# error You need at least Clang v3.4 to compile libvirt +# if __clang_major__ < 6 || (__clang_major__ == 6 && __clang_minor__ < 4) +# error You need at least Clang v6.0 to compile libvirt # endif # endif #elif defined(__GNUC__) && defined(__GNUC_MINOR__) @@ -49,5 +49,5 @@ # error You need at least GCC v7.4.0 to compile libvirt # endif #else -# error You either need at least GCC 7.4.0 or Clang 3.4 or XCode Clang 5.1 to compile libvirt +# error You either need at least GCC 7.4.0 or Clang 6.0 or XCode Clang 10.0 to compile libvirt #endif -- 2.33.1
[libvirt PATCH 3/3] examples: drop some conditionals checks from macros
We no longer need to worry about GCC version older than 7.4.0. The other
remaining conditionals checks were also overkill for the example code.
In the unlikely event that someone tries to re-use the code in a
scenario where further conditions apply they can figure out.
Signed-off-by: Daniel P. Berrangé
---
examples/c/misc/event-test.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c
index 1eec76c79d..1165469a65 100644
--- a/examples/c/misc/event-test.c
+++ b/examples/c/misc/event-test.c
@@ -12,18 +12,8 @@
#define G_N_ELEMENTS(Array) (sizeof(Array) / sizeof(*(Array)))
#define STREQ(a, b) (strcmp(a, b) == 0)
#define NULLSTR(s) ((s) ? (s) : "")
-
-#if (4 < __GNUC__ + (6 <= __GNUC_MINOR__) \
- && (201112L <= __STDC_VERSION__ || !defined __STRICT_ANSI__) \
- && !defined __cplusplus)
-# define G_STATIC_ASSERT(cond) _Static_assert(cond, "verify (" #cond ")")
-#else
-# define G_STATIC_ASSERT(cond)
-#endif
-
-#ifndef G_GNUC_UNUSED
-# define G_GNUC_UNUSED __attribute__((__unused__))
-#endif
+#define G_STATIC_ASSERT(cond) _Static_assert(cond, "verify (" #cond ")")
+#define G_GNUC_UNUSED __attribute__((__unused__))
int run = 1;
--
2.33.1
[libvirt PATCH 0/3] Bump some min versions
Given previous platforms we've dropped, we can bump the min compilers. Daniel P. Berrangé (3): configure: bump min required GCC to 7.4.0 configure: bump min required CLang to 6.0 / XCode 10.0 examples: drop some conditionals checks from macros config.h | 14 +++--- examples/c/misc/event-test.c | 14 ++ 2 files changed, 9 insertions(+), 19 deletions(-) -- 2.33.1
[libvirt PATCH 1/3] configure: bump min required GCC to 7.4.0
Several distros have been dropped since the last time we bumped the minimum required GCC version. Per repology, currently shipping versions are: RHEL-8: 8.3.1 Debian Buster: 8.3.0 openSUSE Leap 15.2: 7.5.0 Ubuntu LTS 18.04: 7.5.0 Ubuntu LTS 20.04: 9.3.0 FreeBSD: 10.3.0 Fedora 33: 9.2.0 Fedora 34: 11.0.1 OpenBSD: 8.4.0 macOS HomeBrew: 11.1.0 With this list Ubuntu LTS 18.04 / openSUSE Leap 15.2 are the constraint at 7.5.0. When QEMU bumped GCC to 7.5.0, however, it was reported that this is a problem for NetBSD which still ships 7.4.0. NetBSD is not an officially targetted platform for libvirt. Given that QEMU saw complaints about this and the feature difference between GCC 7.4.0 and 7.5.0 is minor, I'm being friendly and sticking 7.4.0. Signed-off-by: Daniel P. Berrangé --- config.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config.h b/config.h index 0eacfd139d..2ef32c8627 100644 --- a/config.h +++ b/config.h @@ -45,9 +45,9 @@ # endif # endif #elif defined(__GNUC__) && defined(__GNUC_MINOR__) -# if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 8) -# error You need at least GCC v4.8 to compile libvirt +# if __GNUC__ < 7 || (__GNUC__ == 7 && __GNUC_MINOR__ < 4) +# error You need at least GCC v7.4.0 to compile libvirt # endif #else -# error You either need at least GCC 4.8 or Clang 3.4 or XCode Clang 5.1 to compile libvirt +# error You either need at least GCC 7.4.0 or Clang 3.4 or XCode Clang 5.1 to compile libvirt #endif -- 2.33.1
Re: [PATCH 0/2 for 8.0] Update index after allocating port for isa-serial device
On 14/01/22 8:17 pm, Michal Prívozník wrote: On 1/13/22 15:43, Michal Prívozník wrote: On 1/13/22 08:33, Divya Garg wrote: Issue Divya Garg (2): Add the port allocation logic for isa-serial devices. qemu: add index for isa-serial device using target.port 61 files changed, 156 insertions(+), 90 deletions(-) Hey, couple of points. Usually, when sending new version we do so in a new thread. And we don't CC random people. Us, developers, are subscribed to the list. Anyway, I'm raising only very basic nitpicks with your patches. I'll squash in the fixes I'm suggesting before pushing. Speaking of which, we are currently in freeze, preparing for tomorrow's release. so I'll push these tomorrow. Reviewed-by: Michal Privoznik This is now pushed. Congratulations on your first libvirt contribution! Michal Thankyou Michal !!
Re: [PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.
Thanks Michal !! For reviewing and merging my patch. I will keep all the
nits
pointed by you in mind for my next patches. :)
On 13/01/22 8:50 pm, Michal Prívozník wrote:
On 1/13/22 08:33, Divya Garg wrote:
This commit takes care of following cases:
-> Check availability of requested ports.
->The total number of requested ports should not be more than
VIR_MAX_ISA_SERIAL_PORTS.
->The ports requested should be less than VIR_MAX_ISA_SERIAL_PORTS.
->VIR_MAX_ISA_SERIAL_PORTS should correspond to MAX_ISA_SERIAL_PORTS
specified in qemu code commit def337ffda34d331404bd7f1a42726b71500df22.
-> Prevent duplicate device assignments to the same port.
-> In case no ports are provided in the XML, this patch scans the list of unused
isa-serial indices to automatically assign available ports for this VM.
Signed-off-by: Divya Garg
---
src/conf/domain_conf.c| 61 ---
src/conf/domain_conf.h| 6 ++
...g-console-compat-2-live+console-virtio.xml | 4 +-
.../qemuhotplug-console-compat-2-live.xml | 4 +-
.../serial-tcp-tlsx509-chardev-notls.xml | 2 +-
.../serial-tcp-tlsx509-chardev-verify.xml | 2 +-
.../serial-tcp-tlsx509-chardev.xml| 2 +-
.../serial-tcp-tlsx509-secret-chardev.xml | 2 +-
.../serial-tcp-tlsx509-chardev.xml| 2 +-
9 files changed, 68 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5691b8d2d5..e468e98045 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5330,6 +5330,56 @@ virDomainHostdevDefPostParse(virDomainHostdevDef *dev,
}
Only a few nitpicks.
+static int
+virDomainChrIsaSerialDefPostParse(virDomainDef *def)
+{
+size_t i, j;
We like to define variables on separate lines, because it then allows
for smaller, easier to understand diffs when changing them. Then, a
variable should be declared at the lowest level possible, which in case
of @j is inside the other for() loop.
Ohh yeah right !! I will keep these details in mind for my next patch.
+size_t isa_serial_count = 0;
+bool used_serial_port[VIR_MAX_ISA_SERIAL_PORTS] = {false};
I prefer spaces around curly-braces.
Sure. Actually all the space related issue was coming up with the tests.
Hence I thought it will be okay not to add spaces.
+
+/* Perform all the required checks. */
+for (i = 0; i < def->nserials; i++) {
+
+if (def->serials[i]->targetType !=
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL)
Not your fault, we have a code like this, but I find it more readable if
the condition is on one line. There is a recommendation on lines not
being 80 chars long, but there are few exceptions to the rule (well,
recommendation) and improved code readability is one of them.
Noted
+continue;
+
+if (isa_serial_count++ >= VIR_MAX_ISA_SERIAL_PORTS ||
+def->serials[i]->target.port >= VIR_MAX_ISA_SERIAL_PORTS) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Maximum supported number of ISA serial ports is
'%d'."), VIR_MAX_ISA_SERIAL_PORTS);
Here, and ...
+return -1;
+}
+
+if (def->serials[i]->target.port != -1) {
+if (used_serial_port[def->serials[i]->target.port]) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("target port '%d' already allocated."),
def->serials[i]->target.port);
... here, however, the lines could be broken after the error message.
In the coding convention it was written not to break the error messages
hence followed that rule. But next time will break after error message.
+return -1;
+}
+used_serial_port[def->serials[i]->target.port] = true;
+}
+}
+
+/* Assign the ports to the devices. */
+for (i = 0; i < def->nserials; i++) {
+
+if (def->serials[i]->targetType !=
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL ||
+def->serials[i]->target.port != -1)
+continue;
+
+for (j = 0; j < VIR_MAX_ISA_SERIAL_PORTS; j++) {
+if (!used_serial_port[j]) {
+def->serials[i]->target.port = j;
+used_serial_port[j] = true;
+break;
+}
+}
+}
+return 0;
+}
+
If you'd look around, we like to separate functions with two empty
lines. Yes, we still do have plenty of ancient code with one line
separation, but for new code we definitely should use two lines.
Then again, an exception would be the function would be added around a
code that's still using one line.
Noted.
static void
virDomainChrDefPostParse(virDomainChrDef *chr,
const virDomainDef *def)
@@ -6197,6 +6247,9 @@ virDomainDefPostParse(virDomainDef *def,
goto cleanup;
}
+if (virDomainChrIsaSerialDefPostParse(def) < 0)
+
Re: [PATCH 0/2 for 8.0] Update index after allocating port for isa-serial device
On 13/01/22 8:13 pm, Michal Prívozník wrote: On 1/13/22 08:33, Divya Garg wrote: Issue Divya Garg (2): Add the port allocation logic for isa-serial devices. qemu: add index for isa-serial device using target.port 61 files changed, 156 insertions(+), 90 deletions(-) Hey, couple of points. Usually, when sending new version we do so in a new thread. And we don't CC random people. Us, developers, are subscribed to the list. Hi Michal ! Actually I was told to CC all those who commented on the patch atleast once ! Hence added them in CC. But in future I will take care of this. Thankyou !! Anyway, I'm raising only very basic nitpicks with your patches. I'll squash in the fixes I'm suggesting before pushing. Speaking of which, we are currently in freeze, preparing for tomorrow's release. so I'll push these tomorrow. Thankyou so much !! Reviewed-by: Michal Privoznik Michal
Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port
On 13/01/22 8:13 pm, Michal Prívozník wrote:
On 1/13/22 08:33, Divya Garg wrote:
VM XML accepts target.port but this does not get passed while building the qemu
command line for this VM.
Signed-off-by: Divya Garg
---
src/qemu/qemu_command.c | 25 +++
tests/qemuxml2argvdata/bios.args | 2 +-
.../qemuxml2argvdata/console-compat-auto.args | 2 +-
.../console-compat-auto.x86_64-latest.args| 2 +-
.../console-compat-chardev.args | 2 +-
.../console-compat-chardev.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/console-compat.args| 2 +-
.../console-compat.x86_64-latest.args | 2 +-
.../qemuxml2argvdata/console-virtio-many.args | 2 +-
tests/qemuxml2argvdata/controller-order.args | 2 +-
.../name-escape.x86_64-2.11.0.args| 4 +--
.../name-escape.x86_64-latest.args| 4 +--
.../q35-virt-manager-basic.args | 2 +-
.../serial-dev-chardev-iobase.args| 2 +-
...rial-dev-chardev-iobase.x86_64-latest.args | 2 +-
.../qemuxml2argvdata/serial-dev-chardev.args | 2 +-
.../serial-dev-chardev.x86_64-latest.args | 2 +-
.../qemuxml2argvdata/serial-file-chardev.args | 2 +-
.../serial-file-chardev.x86_64-latest.args| 2 +-
tests/qemuxml2argvdata/serial-file-log.args | 2 +-
.../serial-file-log.x86_64-latest.args| 2 +-
.../qemuxml2argvdata/serial-many-chardev.args | 4 +--
.../serial-many-chardev.x86_64-latest.args| 4 +--
.../qemuxml2argvdata/serial-pty-chardev.args | 2 +-
.../serial-pty-chardev.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/serial-spiceport.args | 2 +-
.../serial-spiceport.x86_64-latest.args | 2 +-
.../qemuxml2argvdata/serial-tcp-chardev.args | 2 +-
.../serial-tcp-chardev.x86_64-latest.args | 2 +-
.../serial-tcp-telnet-chardev.args| 2 +-
...rial-tcp-telnet-chardev.x86_64-latest.args | 2 +-
.../serial-tcp-tlsx509-chardev-notls.args | 4 +--
...p-tlsx509-chardev-notls.x86_64-latest.args | 4 +--
.../serial-tcp-tlsx509-chardev-verify.args| 4 +--
...-tlsx509-chardev-verify.x86_64-latest.args | 4 +--
.../serial-tcp-tlsx509-chardev.args | 4 +--
...ial-tcp-tlsx509-chardev.x86_64-latest.args | 4 +--
.../serial-tcp-tlsx509-secret-chardev.args| 4 +--
...-tlsx509-secret-chardev.x86_64-latest.args | 4 +--
.../qemuxml2argvdata/serial-udp-chardev.args | 4 +--
.../serial-udp-chardev.x86_64-latest.args | 4 +--
.../qemuxml2argvdata/serial-unix-chardev.args | 4 +--
.../serial-unix-chardev.x86_64-latest.args| 4 +--
tests/qemuxml2argvdata/serial-vc-chardev.args | 2 +-
.../serial-vc-chardev.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/user-aliases.args | 4 +--
.../virtio-9p-createmode.x86_64-latest.args | 2 +-
.../virtio-9p-multidevs.x86_64-latest.args| 2 +-
.../x86_64-pc-graphics.x86_64-latest.args | 2 +-
.../x86_64-pc-headless.x86_64-latest.args | 2 +-
.../x86_64-q35-graphics.x86_64-latest.args| 2 +-
.../x86_64-q35-headless.x86_64-latest.args| 2 +-
52 files changed, 88 insertions(+), 73 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d822533ccb..4130df0ed9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
g_autoptr(virJSONValue) props = NULL;
g_autofree char *chardev = g_strdup_printf("char%s", serial->info.alias);
virQEMUCapsFlags caps;
+const char *typestr;
+int ret;
This is a bit confusing. Usually, we use @ret for keeping return value
from the function its defined in. In this specific case, @ret would hold
the retval of qemuBuildSerialChrDeviceProps() and thus would have to be
type of virJSONValue*. If you want to keep an intermediary retval of a
function called from within we use other names, like rc, rv, tmp.
Sure ! I will keep these things in mind in future.
But fortunatelly, we can do without @ret and even without @typestr.
Yeah earlier I was doing it in this manner only but just to increase the
readibility I did this change. But understood how to even optimise the
lines.
Thankyou !!
switch ((virDomainChrSerialTargetModel) serial->targetModel) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
@@ -10750,11 +10752,24 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
return NULL;
}
-if (virJSONValueObjectAdd(,
- "s:driver",
virDomainChrSerialTargetModelTypeToString(serial->targetModel),
- "s:chardev", chardev,
- "s:id", serial->info.alias,
- NULL) < 0)
There's no need to remove this code, especially when you're replacing it
with itself. The pattern that we can use here is:
if (virJSONValueObjectAdd(,
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On 1/14/22 10:56 AM, Ján Tomko wrote: On a Friday in 2022, Tim Wiederhake wrote: This was not mentioned before. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: + int x; + int y; + + BAD: + int x, y; + Please use longer variable names and initialize some too, to illustrate it better, e.g.: int count = 0, nnodes; Personally I don't mind: size_t i, j; that much - even though removing one does cause churn, they are simple to read. I also don't mind combining simple things like that, but am willing to go full-isolated just for consistency's sake. Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-))
Re: [libvirt PATCH 1/4] docs: coding-style: Clarify on virXXXPtr types
On Fri, Jan 14, 2022 at 09:24:23AM -0800, Andrea Bolognani wrote: > On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote: > > On a Friday in 2022, Tim Wiederhake wrote: > > > + Historically, libvirt declared pointer types 'virXXXPtr' for > > > + both public and internal types. Do not introduce new such > > > + typedefs for internal types. > > > > This weakly suggests that they should be introduced for new public > > types. I have no preference, but I think we should mention it here > > if we're mentioning internal types. > > Agreed. More specifically, I think we should encourage defining the > Ptr alias for public types because, much as I dislike their > existence, having a mix of both conventions in the public API would > be IMO worse than sticking with the status quo. Yes, the public API must carry on using its existing practice. Only the internal usage is eliminated. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 1/4] docs: coding-style: Clarify on virXXXPtr types
On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote: > On a Friday in 2022, Tim Wiederhake wrote: > > + Historically, libvirt declared pointer types 'virXXXPtr' for > > + both public and internal types. Do not introduce new such > > + typedefs for internal types. > > This weakly suggests that they should be introduced for new public > types. I have no preference, but I think we should mention it here > if we're mentioning internal types. Agreed. More specifically, I think we should encourage defining the Ptr alias for public types because, much as I dislike their existence, having a mix of both conventions in the public API would be IMO worse than sticking with the status quo. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 1/4] docs: coding-style: Clarify on virXXXPtr types
On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote: > On a Friday in 2022, Tim Wiederhake wrote: > > Signed-off-by: Tim Wiederhake > > --- > > docs/coding-style.rst | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/docs/coding-style.rst b/docs/coding-style.rst > > index 470c61860f..dca9de1915 100644 > > --- a/docs/coding-style.rst > > +++ b/docs/coding-style.rst > > @@ -54,6 +54,7 @@ Struct type names > >and each following word should have its first letter in > >uppercase. The struct name should be the same as the typedef > >name with a leading underscore. > > + > >:: > > > > typedef struct _virHashTable virHashTable; > > @@ -61,6 +62,10 @@ Struct type names > > ... > > }; > > > > + Historically, libvirt declared pointer types 'virXXXPtr' for > > + both public and internal types. Do not introduce new such > > + typedefs for internal types. > > This weakly suggests that they should be introduced for new public > types. I have no preference, but I think we should mention it here > if we're mentioning internal types. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3 7/7] virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
On Wed, Jan 12, 2022 at 09:47:58AM +0100, Michal Privoznik wrote: > After previous cleanups, there's just one caller of > dnsmasqCapsNewEmpty() and it is dnsmasqCapsNewFromBinary(). > And the former is pretty short. Therefore, it is not necessary > for the code to live in two separate functions. Dissolve the > former in the latter. > > Signed-off-by: Michal Privoznik > --- > src/util/virdnsmasq.c | 16 +++- > 1 file changed, 3 insertions(+), 13 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 0/3] qemu: Add caps for qemu-7.0 dev cycle and re-enable JSON for -device
On a Friday in 2022, Peter Krempa wrote: Note that I've truncated the auto-generated stuff out of patches 2/3 and 3/3. For full version: git fetch https://gitlab.com/pipo.sk/libvirt.git device-json-reenable Also note that I've based this on the 'staging' branch of qemu.git currently, which means that there's a small possibility that the pull request will be rejected. I'll make sure to verify that before pushing. Unless you don't: Reviewed-by: Ján Tomko Jano Peter Krempa (3): qemuxml2(argv|xml)data: x86-kvm-32-on-64: Add machine type tests: qemucapabilities: Add test data for the qemu-7.0 development cycle qemu: capabilities: Re-enable JSON syntax for -device signature.asc Description: PGP signature
Re: [PATCH 1/3] qemuxml2(argv|xml)data: x86-kvm-32-on-64: Add machine type
On a Friday in 2022, Peter Krempa wrote: The machine type doesn't change the test result and prevents tests being changed every time we are about to update real capabilities to a new qemu. Signed-off-by: Peter Krempa --- You should've truncated this patch too, for consistency ;) Jano tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args | 2 +- tests/qemuxml2argvdata/x86-kvm-32-on-64.xml | 2 +- tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) signature.asc Description: PGP signature
Re: [PATCH] tests: Report expected monitor command for simulated commands
On a Wednesday in 2022, Michal Privoznik wrote: There are two tests currently that simulate QMP talk: qemucapabilitiestest and qemuhotplugtest. In both cases they check whether currently executed command is the one for which reply was provided. If not an error message is reported. However, the error message contains only the actual command and not the expected one. This makes it harder to navigate through .replies files. Signed-off-by: Michal Privoznik --- tests/qemumonitortestutils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH partially-for-8.0 00/17] qemu: Fix use-after free when redefining snapshots and cleanup the code
On a Wednesday in 2022, Peter Krempa wrote: Patches 1 and 2 should be pushed for 8.0 as the bug was introduced in this dev cycle and the patches are specifically kept very simple. The rest of the series refactors the snapshot validation and helper code to have less weird semantics which lead to this bug. Peter Krempa (17): qemuSnapshotRedefine: Rename 'def' to 'snapdef' qemuSnapshotRedefine: Fix use of snapshot definition after free virDomainMomentAssignDef: Simplify error handling virDomainSnapshotRedefineValidate: Fix validation of VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag virDomainSnapshotAlignDisks: Improve function comment virDomainSnapshotAlignDisks: Convert @default_snapshot to virDomainSnapshotLocation virDomainSnapshotAlignDisks: Move 'require_match' selection logic inside virDomainSnapshotAlignDisks: Allow alternate domain definition when redefining virDomainSnapshotRedefineValidate: Unexport virDomainSnapshotRedefinePrep: Use 'snapdef' for snapshot definition object virDomainSnapshotRedefineValidate: Don't modify the snapshot definition testDomainSnapshotCreateXML: Extract snapshot redefinition code qemuSnapshotCreate: Use 'snapdef' instead of 'def' qemuSnapshotCreate: Standardize handling of the reference on @snapdef qemuDomainSnapshotLoad: Refactor handling of snapshot definition object virDomainSnapshotAssignDef: Clear second argument when it is consumed virDomainSnapshotRedefinePrep: Don't do partial redefine src/conf/snapshot_conf.c| 120 +++- src/conf/snapshot_conf.h| 13 +-- src/conf/virdomainmomentobjlist.c | 9 +-- src/conf/virdomainsnapshotobjlist.c | 29 ++- src/conf/virdomainsnapshotobjlist.h | 5 +- src/libvirt_private.syms| 1 + src/qemu/qemu_driver.c | 18 ++--- src/qemu/qemu_snapshot.c| 35 src/test/test_driver.c | 89 - src/vz/vz_sdk.c | 3 +- 10 files changed, 180 insertions(+), 142 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v3 6/7] virdnsmasq: Drop dnsmasqCapsNewFromBuffer()
On Wed, Jan 12, 2022 at 09:47:57AM +0100, Michal Privoznik wrote: > The function is no longer used. Remove it. > > Signed-off-by: Michal Privoznik > --- > src/libvirt_private.syms | 1 - > src/util/virdnsmasq.c| 14 -- > src/util/virdnsmasq.h| 1 - > 3 files changed, 16 deletions(-) Reviewed-by: Andrea Bolognani Further cleanup opportunities: * drop the 'force' argument of dnsmasqCapsRefreshInternal(), as the only caller always passes true; * drop the 'noRefresh' field of the dnsmasqCaps struct, which AFAICT doesn't really do anything even before your changes; * rework dnsmasqCapsSetFromBuffer() so that it takes the output of the two calls to dnsmasq as separate arguments, instead of doing the silly dance of joining the two in the caller only to have the function split them again immediately. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3 3/7] virdnsmasq: Lookup DNSMASQ in PATH
On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
> +VIR_TEST_MAIN_PRELOAD(mymain,
> + VIR_TEST_MOCK("network"))
Also I think "virdnsmasqmock" would be a better name for this mock
library, as it mocks stuff that influences the "virdnsmasq" module.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 09/17] virDomainSnapshotRedefineValidate: Unexport
On a Wednesday in 2022, Peter Krempa wrote: The function isn't used outside of src/conf/snapshot_conf.c Exported by 21b2651e72ee33211e057d5c3921d9af65465f8e for the code motion in 9b75154c07dc50fb50296f35543f5dba0337cbb8 but its user was later reverted in 57d252c7401f981f1caea90250d72e0f87caac19 Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 6 -- 2 files changed, 1 insertion(+), 7 deletions(-) signature.asc Description: PGP signature
Re: [PATCH v3 5/7] networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps
On Wed, Jan 12, 2022 at 09:47:56AM +0100, Michal Privoznik wrote:
> DISCLAIMER: dnsmasq capabilities are empty as of v8.0.0-rc1~145.
>
> In a real environment the dnsmasq capabilities are constructed
> using dnsmasqCapsNewFromBinary(). We also have
> dnsmasqCapsNewFromBuffer() to bypass checks that real code is
> doing and just get capabilities object. The latter is used from
> test suite.
>
> However, with a little bit of mocking we can test the real life
> code. All that's needed is to simulate dnsmasq's output for
> --version and --help and mock a stat() that's done in
> dnsmasqCapsRefreshInternal().
>
> Signed-off-by: Michal Privoznik
> ---
> tests/networkmock.c | 16
> tests/networkxml2conftest.c | 38 -
> 2 files changed, 53 insertions(+), 1 deletion(-)
This all works, but I wonder if we couldn't just create a trivial
shell script that behaves minimally the way we expect dnsmasq to, and
change our virFindFileInPath() mock so that it returns the absolute
path to it? That way we wouldn't need to implement any additional
mocking and the code would end up being much simpler. Diff below.
Also note that there is a pre-existing issue with the test, in that
we don't check that the value returned by dnsmasqCapsNewFrom*() is
non-NULL, and as a result if you change the version number in the
test string to something like 0.1 the test will still pass where it
definitely shouldn't.
diff --git a/tests/networkmock.c b/tests/networkmock.c
index a9c13311a6..dc1209a367 100644
--- a/tests/networkmock.c
+++ b/tests/networkmock.c
@@ -23,7 +23,7 @@ char *
virFindFileInPath(const char *file)
{
if (file && g_strrstr(file, "dnsmasq"))
-return g_strdup(file);
+return g_strdup_printf("%s/virdnsmasqmock.sh", abs_srcdir);
/* We should not need any other binaries so return NULL. */
return NULL;
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c
index 8a6657654a..fd2116756e 100644
--- a/tests/networkxml2conftest.c
+++ b/tests/networkxml2conftest.c
@@ -114,7 +114,7 @@ mymain(void)
int ret = 0;
g_autoptr(dnsmasqCaps) full = NULL;
-full = dnsmasqCapsNewFromBuffer("Dnsmasq version
2.67\n--bind-dynamic\n--ra-param");
+full = dnsmasqCapsNewFromBinary();
#define DO_TEST(xname, xcaps) \
do { \
diff --git a/tests/virdnsmasqmock.sh b/tests/virdnsmasqmock.sh
new file mode 100755
index 00..faaa94268c
--- /dev/null
+++ b/tests/virdnsmasqmock.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+case "$1" in
+ "--version")
+ echo "Dnsmasq version 2.67"
+ ;;
+ "--help")
+ echo "--bind-dynamic"
+ echo "--ra-param"
+ ;;
+ *)
+ exit 1
+ ;;
+esac
+
+exit 0
--
Andrea Bolognani / Red Hat / Virtualization
[PATCH 3/3] qemu: capabilities: Re-enable JSON syntax for -device
Now that qemu fixed device unplug when JSON syntax is used with -device we can re-enable the feature. Since the old capability string representation is condemned by suggesting filtering it as a workaround we must introduce a new string. To achieve this the original capability position is renamed to X_QEMU_CAPS_DEVICE_JSON_BROKEN_HOTPLUG and a new position with the original name QEMU_CAPS_DEVICE_JSON is introduced to prevent us having to change the rest of the code. Signed-off-by: Peter Krempa --- Warning! patch truncated, see cover letter. src/qemu/qemu_capabilities.c | 6 ++- src/qemu/qemu_capabilities.h | 5 ++- .../caps_7.0.0.x86_64.xml | 1 + .../audio-alsa-best.x86_64-latest.args| 4 +- .../audio-alsa-full.x86_64-latest.args| 4 +- .../audio-alsa-minimal.x86_64-latest.args | 4 +- .../audio-coreaudio-best.x86_64-latest.args | 4 +- .../audio-coreaudio-full.x86_64-latest.args | 4 +- ...audio-coreaudio-minimal.x86_64-latest.args | 4 +- ...udio-default-nographics.x86_64-latest.args | 4 +- .../audio-default-sdl.x86_64-latest.args | 6 +-- .../audio-default-spice.x86_64-latest.args| 6 +-- .../audio-default-vnc.x86_64-latest.args | 6 +-- .../audio-file-best.x86_64-latest.args| 4 +- .../audio-file-full.x86_64-latest.args| 4 +- .../audio-file-minimal.x86_64-latest.args | 4 +- .../audio-jack-full.x86_64-latest.args| 4 +- .../audio-jack-minimal.x86_64-latest.args | 4 +- .../audio-many-backends.x86_64-latest.args| 14 +++ .../audio-none-best.x86_64-latest.args| 4 +- .../audio-none-full.x86_64-latest.args| 4 +- .../audio-none-minimal.x86_64-latest.args | 4 +- .../audio-oss-best.x86_64-latest.args | 4 +- .../audio-oss-full.x86_64-latest.args | 4 +- .../audio-oss-minimal.x86_64-latest.args | 4 +- .../audio-pulseaudio-best.x86_64-latest.args | 4 +- .../audio-pulseaudio-full.x86_64-latest.args | 4 +- ...udio-pulseaudio-minimal.x86_64-latest.args | 4 +- .../audio-sdl-best.x86_64-latest.args | 4 +- .../audio-sdl-full.x86_64-latest.args | 4 +- .../audio-sdl-minimal.x86_64-latest.args | 4 +- .../audio-spice-best.x86_64-latest.args | 4 +- .../audio-spice-full.x86_64-latest.args | 4 +- .../audio-spice-minimal.x86_64-latest.args| 4 +- .../blkdeviotune-group-num.x86_64-latest.args | 8 ++-- ...blkdeviotune-max-length.x86_64-latest.args | 8 ++-- .../blkdeviotune-max.x86_64-latest.args | 8 ++-- .../blkdeviotune.x86_64-latest.args | 8 ++-- .../channel-unix-guestfwd.x86_64-latest.args | 4 +- .../console-compat-auto.x86_64-latest.args| 8 ++-- .../console-compat-chardev.x86_64-latest.args | 8 ++-- .../console-compat.x86_64-latest.args | 6 +-- .../console-virtio-unix.x86_64-latest.args| 10 ++--- .../controller-usb-order.x86_64-latest.args | 8 ++-- .../controller-virtio-scsi.x86_64-latest.args | 24 +-- ...-Icelake-Server-pconfig.x86_64-latest.args | 4 +- .../cpu-host-model.x86_64-latest.args | 10 ++--- .../cpu-translation.x86_64-latest.args| 4 +- .../cputune-cpuset-big-id.x86_64-latest.args | 6 +-- .../devices-acpi-index.x86_64-latest.args | 16 +++ .../disk-aio-io_uring.x86_64-latest.args | 6 +-- .../disk-aio.x86_64-latest.args | 8 ++-- ...-backing-chains-noindex.x86_64-latest.args | 16 +++ .../disk-blockio.x86_64-latest.args | 8 ++-- .../disk-boot-cdrom.x86_64-latest.args| 6 +-- .../disk-boot-disk.x86_64-latest.args | 6 +-- .../disk-cache.x86_64-latest.args | 14 +++ .../disk-cdrom-bus-other.x86_64-latest.args | 6 +-- ...m-empty-network-invalid.x86_64-latest.args | 4 +- .../disk-cdrom-network.x86_64-latest.args | 10 ++--- .../disk-cdrom-tray.x86_64-latest.args| 10 ++--- .../disk-cdrom.x86_64-latest.args | 10 ++--- .../disk-copy_on_read.x86_64-latest.args | 12 +++--- .../disk-detect-zeroes.x86_64-latest.args | 8 ++-- .../disk-discard.x86_64-latest.args | 8 ++-- .../disk-error-policy.x86_64-latest.args | 8 ++-- .../disk-floppy-q35.x86_64-latest.args| 12 +++--- .../disk-floppy-tray.x86_64-latest.args | 10 ++--- .../disk-floppy.x86_64-latest.args| 8 ++-- .../disk-fmt-qcow.x86_64-latest.args | 6 +-- .../disk-geometry.x86_64-latest.args | 6 +-- .../disk-ide-split.x86_64-latest.args | 8 ++-- .../disk-ide-wwn.x86_64-latest.args | 6 +-- .../disk-ioeventfd.x86_64-latest.args | 12 +++--- .../disk-metadata-cache.x86_64-latest.args| 8 ++-- .../disk-network-gluster.x86_64-latest.args | 8 ++-- .../disk-network-http.x86_64-latest.args | 10 ++--- .../disk-network-iscsi.x86_64-latest.args | 18 .../disk-network-nbd.x86_64-latest.args
[PATCH 1/3] qemuxml2(argv|xml)data: x86-kvm-32-on-64: Add machine type
The machine type doesn't change the test result and prevents tests being
changed every time we are about to update real capabilities to a new
qemu.
Signed-off-by: Peter Krempa
---
tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args | 2 +-
tests/qemuxml2argvdata/x86-kvm-32-on-64.xml | 2 +-
tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args
b/tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args
index fe326b6943..cfe1abab60 100644
--- a/tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/x86-kvm-32-on-64.x86_64-latest.args
@@ -10,7 +10,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-kvm/.config \
-name guest=kvm,debug-threads=on \
-S \
-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-kvm/master-key.aes"}'
\
--machine pc-i440fx-6.2,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram \
-accel kvm \
-cpu qemu64 \
-m 4096 \
diff --git a/tests/qemuxml2argvdata/x86-kvm-32-on-64.xml
b/tests/qemuxml2argvdata/x86-kvm-32-on-64.xml
index 37f53bf2af..35ddbd0630 100644
--- a/tests/qemuxml2argvdata/x86-kvm-32-on-64.xml
+++ b/tests/qemuxml2argvdata/x86-kvm-32-on-64.xml
@@ -3,7 +3,7 @@
d091ea82-29e6-2e34-3005-f02617b36e87
4194304
-hvm
+hvm
/usr/bin/qemu-system-x86_64
diff --git a/tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml
b/tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml
index b51fb18788..c5e9c4fe22 100644
--- a/tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/x86-kvm-32-on-64.x86_64-latest.xml
@@ -5,7 +5,7 @@
4194304
1
-hvm
+hvm
--
2.34.1
[PATCH 0/3] qemu: Add caps for qemu-7.0 dev cycle and re-enable JSON for -device
Note that I've truncated the auto-generated stuff out of patches 2/3 and 3/3. For full version: git fetch https://gitlab.com/pipo.sk/libvirt.git device-json-reenable Also note that I've based this on the 'staging' branch of qemu.git currently, which means that there's a small possibility that the pull request will be rejected. I'll make sure to verify that before pushing. Peter Krempa (3): qemuxml2(argv|xml)data: x86-kvm-32-on-64: Add machine type tests: qemucapabilities: Add test data for the qemu-7.0 development cycle qemu: capabilities: Re-enable JSON syntax for -device src/qemu/qemu_capabilities.c | 6 +- src/qemu/qemu_capabilities.h | 5 +- .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 230 + .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 236 + tests/domaincapsdata/qemu_7.0.0.x86_64.xml| 230 + .../caps_7.0.0.x86_64.replies | 37261 .../caps_7.0.0.x86_64.xml | 3717 ++ .../audio-alsa-best.x86_64-latest.args| 4 +- .../audio-alsa-full.x86_64-latest.args| 4 +- .../audio-alsa-minimal.x86_64-latest.args | 4 +- .../audio-coreaudio-best.x86_64-latest.args | 4 +- .../audio-coreaudio-full.x86_64-latest.args | 4 +- ...audio-coreaudio-minimal.x86_64-latest.args | 4 +- ...udio-default-nographics.x86_64-latest.args | 4 +- .../audio-default-sdl.x86_64-latest.args | 6 +- .../audio-default-spice.x86_64-latest.args| 6 +- .../audio-default-vnc.x86_64-latest.args | 6 +- .../audio-file-best.x86_64-latest.args| 4 +- .../audio-file-full.x86_64-latest.args| 4 +- .../audio-file-minimal.x86_64-latest.args | 4 +- .../audio-jack-full.x86_64-latest.args| 4 +- .../audio-jack-minimal.x86_64-latest.args | 4 +- .../audio-many-backends.x86_64-latest.args|14 +- .../audio-none-best.x86_64-latest.args| 4 +- .../audio-none-full.x86_64-latest.args| 4 +- .../audio-none-minimal.x86_64-latest.args | 4 +- .../audio-oss-best.x86_64-latest.args | 4 +- .../audio-oss-full.x86_64-latest.args | 4 +- .../audio-oss-minimal.x86_64-latest.args | 4 +- .../audio-pulseaudio-best.x86_64-latest.args | 4 +- .../audio-pulseaudio-full.x86_64-latest.args | 4 +- ...udio-pulseaudio-minimal.x86_64-latest.args | 4 +- .../audio-sdl-best.x86_64-latest.args | 4 +- .../audio-sdl-full.x86_64-latest.args | 4 +- .../audio-sdl-minimal.x86_64-latest.args | 4 +- .../audio-spice-best.x86_64-latest.args | 4 +- .../audio-spice-full.x86_64-latest.args | 4 +- .../audio-spice-minimal.x86_64-latest.args| 4 +- .../blkdeviotune-group-num.x86_64-latest.args | 8 +- ...blkdeviotune-max-length.x86_64-latest.args | 8 +- .../blkdeviotune-max.x86_64-latest.args | 8 +- .../blkdeviotune.x86_64-latest.args | 8 +- .../channel-unix-guestfwd.x86_64-latest.args | 4 +- .../console-compat-auto.x86_64-latest.args| 8 +- .../console-compat-chardev.x86_64-latest.args | 8 +- .../console-compat.x86_64-latest.args | 6 +- .../console-virtio-unix.x86_64-latest.args|10 +- .../controller-usb-order.x86_64-latest.args | 8 +- .../controller-virtio-scsi.x86_64-latest.args |24 +- ...-Icelake-Server-pconfig.x86_64-latest.args | 4 +- .../cpu-host-model.x86_64-latest.args |10 +- .../cpu-translation.x86_64-latest.args| 4 +- .../cputune-cpuset-big-id.x86_64-latest.args | 6 +- .../devices-acpi-index.x86_64-latest.args |16 +- .../disk-aio-io_uring.x86_64-latest.args | 6 +- .../disk-aio.x86_64-latest.args | 8 +- ...-backing-chains-noindex.x86_64-latest.args |16 +- .../disk-blockio.x86_64-latest.args | 8 +- .../disk-boot-cdrom.x86_64-latest.args| 6 +- .../disk-boot-disk.x86_64-latest.args | 6 +- .../disk-cache.x86_64-latest.args |14 +- .../disk-cdrom-bus-other.x86_64-latest.args | 6 +- ...m-empty-network-invalid.x86_64-latest.args | 4 +- .../disk-cdrom-network.x86_64-latest.args |10 +- .../disk-cdrom-tray.x86_64-latest.args|10 +- .../disk-cdrom.x86_64-latest.args |10 +- .../disk-copy_on_read.x86_64-latest.args |12 +- .../disk-detect-zeroes.x86_64-latest.args | 8 +- .../disk-discard.x86_64-latest.args | 8 +- .../disk-error-policy.x86_64-latest.args | 8 +- .../disk-floppy-q35.x86_64-latest.args|12 +- .../disk-floppy-tray.x86_64-latest.args |10 +- .../disk-floppy.x86_64-latest.args| 8 +- .../disk-fmt-qcow.x86_64-latest.args | 6 +- .../disk-geometry.x86_64-latest.args | 6 +- .../disk-ide-split.x86_64-latest.args
Re: [PATCH 00/11] Misc cleanups
On a Friday in 2022, Michal Privoznik wrote: Couple of cleanups I've done whilst looking around our code base. Michal Prívozník (11): storage_file: Declare virStorageSourceParseRBDColonString only in one header virconf: Report an error in when virConfSetValue() fails xen_xl: Check for virConfSetValue() retval src: Declare and use g_autoptr(virConfValue) virconf: Make virConfSetValue() clear consumed pointer libxl: Don't use a static buffer in xenParseXLVnuma() libxl: Allocate @libxldisk in xenParseXLDisk() on stack xen_xl.c: Use g_autofree more xen_xl.c: Use g_autoptr() for virCPUDef libxl: Remove needless labels virsh: Remove needless labels src/libxl/xen_common.c| 62 ++--- src/libxl/xen_xl.c| 259 ++ src/libxl/xen_xm.c| 18 +- src/storage_file/storage_source.h | 5 - .../storage_source_backingstore.h | 3 +- src/util/virconf.c| 19 +- src/util/virconf.h| 3 +- tools/virsh-host.c| 20 +- tools/vsh.c | 11 +- 9 files changed, 135 insertions(+), 265 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 08/11] xen_xl.c: Use g_autofree more
On a Friday in 2022, Michal Privoznik wrote:
There are few places inside src/libxl/xen_xl.c that can benefit
from g_autofree. Let them use automatic memory freeing.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_xl.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
[...]
@@ -952,13 +951,13 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
{
virConfValue *list = virConfGetValue(conf, "channel");
virDomainChrDef *channel = NULL;
-char *name = NULL;
-char *path = NULL;
if (list && list->type == VIR_CONF_LIST) {
list = list->list;
while (list) {
g_autofree char *type = NULL;
+g_autofree char *name = NULL;
+g_autofree char *path = NULL;
char *key;
if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
@@ -1003,7 +1002,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
There's a mix of g_auto with manual freeing here:
} else if (STRPREFIX(key, "name=")) {
int len = nextkey ? (nextkey - data) : strlen(data);
VIR_FREE(name);
name = g_strndup(data, len);
} else if (STRPREFIX(key, "path=")) {
int len = nextkey ? (nextkey - data) : strlen(data);
VIR_FREE(path);
path = g_strndup(data, len);
}
I would prefer either altering the code to take the first key into
account instead of the last one by checking:
else if (!name && STRPREFIX...
similarly how we do in node-based XML parsers, or leaving these two
variables unconverted and leave the refactor for later.
Jano
channel->source->data.nix.path = g_steal_pointer();
} else if (STRPREFIX(type, "pty")) {
signature.asc
Description: PGP signature
Re: [PATCH 01/11] storage_file: Declare virStorageSourceParseRBDColonString only in one header
On a Friday in 2022, Michal Privoznik wrote: The virStorageSourceParseRBDColonString() function is declared in src/storage_file/storage_source.h and src/storage_file/storage_source_backingstore.h but implemented only in the .c that corresponds to the latter header file. Therefore, drop declaration from storage_source.h as the function is not implemented in its corresponding .c file. Signed-off-by: Michal Privoznik --- src/libxl/xen_xl.c | 1 + src/storage_file/storage_source.h | 5 - src/storage_file/storage_source_backingstore.h | 3 ++- 3 files changed, 3 insertions(+), 6 deletions(-) Leftover from: 2d29a3a9d86b5f95a859888283e6caa98593b1d2 Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On a Friday in 2022, Tim Wiederhake wrote: This was not mentioned before. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: +int x; +int y; + + BAD: +int x, y; + Please use longer variable names and initialize some too, to illustrate it better, e.g.: int count = 0, nnodes; Personally I don't mind: size_t i, j; that much - even though removing one does cause churn, they are simple to read. Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 3/4] docs: coding-style: Remove "no_memory" as acceptable goto target
On a Friday in 2022, Tim Wiederhake wrote: There are no instances of that label left. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 1/4] docs: coding-style: Clarify on virXXXPtr types
On a Friday in 2022, Tim Wiederhake wrote: Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..dca9de1915 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + :: typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... }; + Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types. + This weakly suggests that they should be introduced for new public types. I have no preference, but I think we should mention it here if we're mentioning internal types. Jano Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word -- 2.31.1 signature.asc Description: PGP signature
Re: [libvirt PATCH 2/4] docs: coding-style: Rewrite section on shortening comparisons
On a Friday in 2022, Tim Wiederhake wrote: The code style showed `bool hasFoos; if (hasFoos == true)` as a good example in one place, only to warn against comparisons with `true` a couple of paragraphs further down. Merge this advice on comparing with `true` into the "Conditional expressions" section and split the example up for readability. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 60 +++ 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index dca9de1915..3dedb032f4 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -427,25 +427,47 @@ Conditional expressions --- For readability reasons new code should avoid shortening -comparisons to 0 for numeric types. Boolean and pointer -comparisons may be shortened. All long forms are okay: +comparisons to 0 for numeric types: :: - virFoo *foos = NULL; size nfoos = 0; - bool hasFoos = false; GOOD: -if (!foos) -if (!hasFoos) +if (nfoos != 0) if (nfoos == 0) -if (foos == NULL) -if (hasFoos == true) BAD: -if (!nfoos) if (nfoos) +if (!nfoos) + +Prefer the shortened version for boolean values. Boolean values +should never be compared against the literal ``true``, as a +logical non-false value need not be ``1``. + +:: + + bool hasFoos = false; + + GOOD: +if (hasFoos) +if (!hasFoos) + + BAD: +if (hasFoos == true) +if (hasFoos != false) +if (hasFoos == false) +if (hasFoos != true) I think people would get it even if all four options weren't listed :) + +Pointer comparisons may be shortened. All long forms are okay. + Either way: Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v3 3/7] virdnsmasq: Lookup DNSMASQ in PATH
On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
> With this code in place, the virFileIsExecutable() check can be
> removed from dnsmasqCapsRefreshInternal() because
> virFindFileInPath() already made sure the binary is executable.
>
> But introducing virFileIsExecutable() means we have to mock it in
I believe you meant virFindFileInPath() here.
> +++ b/src/util/virdnsmasq.c
> @@ -702,14 +692,20 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool
> force)
> static dnsmasqCaps *
> dnsmasqCapsNewEmpty(void)
> {
> -dnsmasqCaps *caps;
> +g_autoptr(dnsmasqCaps) caps = NULL;
>
> if (dnsmasqCapsInitialize() < 0)
> return NULL;
> if (!(caps = virObjectNew(dnsmasqCapsClass)))
> return NULL;
> -caps->binaryPath = g_strdup(DNSMASQ);
> -return caps;
> +
> +if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
> +virReportSystemError(ENOENT, "%s",
> + _("Unable to find 'dnsmasq' binary in $PATH"));
> +return NULL;
> +}
> +
> +return g_steal_pointer();
> }
Can you please squash the g_autoptr conversion into the previous
patch?
> +++ b/tests/networkmock.c
> @@ -0,0 +1,30 @@
> +/*
> + * 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
> + *
[libvirt PATCH 2/4] docs: coding-style: Rewrite section on shortening comparisons
The code style showed `bool hasFoos; if (hasFoos == true)` as a good example in one place, only to warn against comparisons with `true` a couple of paragraphs further down. Merge this advice on comparing with `true` into the "Conditional expressions" section and split the example up for readability. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 60 +++ 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index dca9de1915..3dedb032f4 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -427,25 +427,47 @@ Conditional expressions --- For readability reasons new code should avoid shortening -comparisons to 0 for numeric types. Boolean and pointer -comparisons may be shortened. All long forms are okay: +comparisons to 0 for numeric types: :: - virFoo *foos = NULL; size nfoos = 0; - bool hasFoos = false; GOOD: -if (!foos) -if (!hasFoos) +if (nfoos != 0) if (nfoos == 0) -if (foos == NULL) -if (hasFoos == true) BAD: -if (!nfoos) if (nfoos) +if (!nfoos) + +Prefer the shortened version for boolean values. Boolean values +should never be compared against the literal ``true``, as a +logical non-false value need not be ``1``. + +:: + + bool hasFoos = false; + + GOOD: +if (hasFoos) +if (!hasFoos) + + BAD: +if (hasFoos == true) +if (hasFoos != false) +if (hasFoos == false) +if (hasFoos != true) + +Pointer comparisons may be shortened. All long forms are okay. + +:: + + virFoo *foo = NULL; + + GOOD: +if (foo) # or: if (foo != NULL) +if (!foo)# or: if (foo == NULL) New code should avoid the ternary operator as much as possible. Specifically it must never span more than one line or nest: @@ -507,19 +529,13 @@ Scalars - In the unusual event that you require a specific width, use a standard type like ``int32_t``, ``uint32_t``, ``uint64_t``, etc. -- While using ``bool`` is good for readability, it comes with - minor caveats: - - - Don't use ``bool`` in places where the type size must be - constant across all systems, like public interfaces and - on-the-wire protocols. Note that it would be possible - (albeit wasteful) to use ``bool`` in libvirt's logical wire - protocol, since XDR maps that to its lower-level ``bool_t`` - type, which **is** fixed-size. - - Don't compare a bool variable against the literal, ``true``, - since a value with a logical non-false value need not be - ``1``. I.e., don't write ``if (seen == true) ...``. Rather, - write ``if (seen)...``. +- While using ``bool`` is good for readability, it comes with a + minor caveat: Don't use ``bool`` in places where the type size + must be constant across all systems, like public interfaces and + on-the-wire protocols. Note that it would be possible (albeit + wasteful) to use ``bool`` in libvirt's logical wire protocol, + since XDR maps that to its lower-level ``bool_t`` type, which + **is** fixed-size. Of course, take all of the above with a grain of salt. If you're about to use some system interface that requires a type like -- 2.31.1
[libvirt PATCH 0/4] Code style documentation
Some additions and clarifications to libvirt's code style documentation, based on points of feedback that are given regularly on the mailing list. Tim Wiederhake (4): docs: coding-style: Clarify on virXXXPtr types docs: coding-style: Rewrite section on shortening comparisons docs: coding-style: Remove "no_memory" as acceptable goto target docs: coding-style: One variable declaration per line docs/coding-style.rst | 79 ++- 1 file changed, 56 insertions(+), 23 deletions(-) -- 2.31.1
[libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
This was not mentioned before. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: +int x; +int y; + + BAD: +int x, y; + Attribute annotations - -- 2.31.1
[libvirt PATCH 1/4] docs: coding-style: Clarify on virXXXPtr types
Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..dca9de1915 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + :: typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... }; + Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types. + Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word -- 2.31.1
[libvirt PATCH 3/4] docs: coding-style: Remove "no_memory" as acceptable goto target
There are no instances of that label left. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 3dedb032f4..14c5136398 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -953,7 +953,6 @@ makes sense: error: A path only taken upon return with an error code cleanup: A path taken upon return with success code + optional error - no_memory: A path only taken upon return with an OOM error code retry: If needing to jump upwards (e.g., retry on EINTR) Top-level labels should be indented by one space (putting them on -- 2.31.1
Re: [PATCH 0/2 for 8.0] Update index after allocating port for isa-serial device
On 1/13/22 15:43, Michal Prívozník wrote: > On 1/13/22 08:33, Divya Garg wrote: >> Issue > >> >> Divya Garg (2): >> Add the port allocation logic for isa-serial devices. >> qemu: add index for isa-serial device using target.port > > >> 61 files changed, 156 insertions(+), 90 deletions(-) >> > > Hey, couple of points. Usually, when sending new version we do so in a > new thread. And we don't CC random people. Us, developers, are > subscribed to the list. > > Anyway, I'm raising only very basic nitpicks with your patches. I'll > squash in the fixes I'm suggesting before pushing. Speaking of which, we > are currently in freeze, preparing for tomorrow's release. so I'll push > these tomorrow. > > Reviewed-by: Michal Privoznik > This is now pushed. Congratulations on your first libvirt contribution! Michal
Re: [PATCH v3 2/7] lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref
On Wed, Jan 12, 2022 at 09:47:53AM +0100, Michal Privoznik wrote: > The dnsmasqCaps type has its own cleanup function defined and > ready to use via g_autoptr(). Use automatic cleanup instead of > an explicit one. > > Signed-off-by: Michal Privoznik > --- > src/util/virdnsmasq.c | 18 -- > tests/networkxml2conftest.c | 7 +++ > 2 files changed, 11 insertions(+), 14 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 11/11] virsh: Remove needless labels
There are few places where a cleanup label contains nothing but a
return statement. Drop such labels and return directly.
Signed-off-by: Michal Privoznik
---
tools/virsh-host.c | 20 ++--
tools/vsh.c| 11 ---
2 files changed, 10 insertions(+), 21 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 5ee3834de2..2e3cbc39b6 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -1195,7 +1195,6 @@ static bool
cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
{
const char *from = NULL;
-bool ret = false;
int result;
g_auto(GStrv) cpus = NULL;
unsigned int flags = 0;
@@ -1219,7 +1218,7 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
case VIR_CPU_COMPARE_INCOMPATIBLE:
vshPrint(ctl, _("CPU described in %s is incompatible with host CPU\n"),
from);
-goto cleanup;
+return false;
break;
case VIR_CPU_COMPARE_IDENTICAL:
@@ -1235,13 +1234,10 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
case VIR_CPU_COMPARE_ERROR:
default:
vshError(ctl, _("Failed to compare host CPU with %s"), from);
-goto cleanup;
+return false;
}
-ret = true;
-
- cleanup:
-return ret;
+return true;
}
/*
@@ -1615,7 +1611,6 @@ cmdHypervisorCPUCompare(vshControl *ctl,
const char *emulator = NULL;
const char *arch = NULL;
const char *machine = NULL;
-bool ret = false;
int result;
g_auto(GStrv) cpus = NULL;
unsigned int flags = 0;
@@ -1646,7 +1641,7 @@ cmdHypervisorCPUCompare(vshControl *ctl,
_("CPU described in %s is incompatible with the CPU provided "
"by hypervisor on the host\n"),
from);
-goto cleanup;
+return false;
break;
case VIR_CPU_COMPARE_IDENTICAL:
@@ -1666,13 +1661,10 @@ cmdHypervisorCPUCompare(vshControl *ctl,
case VIR_CPU_COMPARE_ERROR:
default:
vshError(ctl, _("Failed to compare hypervisor CPU with %s"), from);
-goto cleanup;
+return false;
}
-ret = true;
-
- cleanup:
-return ret;
+return true;
}
diff --git a/tools/vsh.c b/tools/vsh.c
index e3e27a0ba6..c0098054e0 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3360,7 +3360,6 @@ const vshCmdInfo info_complete[] = {
bool
cmdComplete(vshControl *ctl, const vshCmd *cmd)
{
-bool ret = false;
const vshClientHooks *hooks = ctl->hooks;
int stdin_fileno = STDIN_FILENO;
const char *arg = "";
@@ -3370,7 +3369,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
-goto cleanup;
+return false;
/* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
* need to prevent auth hooks reading any input. Therefore, we
@@ -3378,7 +3377,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
VIR_FORCE_CLOSE(stdin_fileno);
if (!(hooks && hooks->connHandler && hooks->connHandler(ctl)))
-goto cleanup;
+return false;
while ((opt = vshCommandOptArgv(ctl, cmd, opt))) {
if (virBufferUse() != 0)
@@ -3397,7 +3396,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
rl_point = strlen(rl_line_buffer);
if (!(matches = vshReadlineCompletion(arg, 0, 0)))
-goto cleanup;
+return false;
for (iter = matches; *iter; iter++) {
if (iter == matches && matches[1])
@@ -3405,9 +3404,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
printf("%s\n", *iter);
}
-ret = true;
- cleanup:
-return ret;
+return true;
}
--
2.34.1
[PATCH 06/11] libxl: Don't use a static buffer in xenParseXLVnuma()
The xenParseXLVnuma() function is responsible for parsing 'vnuma'
part of XL config and setting corresponding values in
virDomainDef. While doing so it uses a static buffer which is set
to data we are interested in and then parsing the buffer further
(e.g. string to integer conversion, bitmap parsing, and so on).
Well, the data we are interested in are already in a string
(@data) which can be used directly rendering this intermediary
buffer needless.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_xl.c | 43 ---
1 file changed, 4 insertions(+), 39 deletions(-)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 6e489f35ad..3a4e21ee0e 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -394,7 +394,6 @@ xenParseXLVnuma(virConf *conf,
virDomainDef *def)
{
int ret = -1;
-char *tmp = NULL;
size_t vcpus = 0;
size_t nr_nodes = 0;
size_t vnodeCnt = 0;
@@ -446,19 +445,10 @@ xenParseXLVnuma(virConf *conf,
data++;
if (*data) {
-char vtoken[64];
-
if (STRPREFIX(str, "pnode")) {
unsigned int cellid;
-if (virStrcpyStatic(vtoken, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("vnuma vnode %zu pnode '%s' too
long for destination"),
- vnodeCnt, data);
-goto cleanup;
-}
-
-if ((virStrToLong_ui(vtoken, NULL, 10, ) < 0) ||
+if ((virStrToLong_ui(data, NULL, 10, ) < 0) ||
(cellid >= nr_nodes)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vnuma vnode %zu contains invalid
pnode value '%s'"),
@@ -467,27 +457,13 @@ xenParseXLVnuma(virConf *conf,
}
pnode = cellid;
} else if (STRPREFIX(str, "size")) {
-if (virStrcpyStatic(vtoken, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("vnuma vnode %zu size '%s' too
long for destination"),
- vnodeCnt, data);
-goto cleanup;
-}
-
-if (virStrToLong_ull(vtoken, NULL, 10, ) < 0)
+if (virStrToLong_ull(data, NULL, 10, ) < 0)
goto cleanup;
virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize
* 1024));
} else if (STRPREFIX(str, "vcpus")) {
-if (virStrcpyStatic(vtoken, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("vnuma vnode %zu vcpus '%s' too
long for destination"),
- vnodeCnt, data);
-goto cleanup;
-}
-
-if (virBitmapParse(vtoken, ,
VIR_DOMAIN_CPUMASK_LEN) < 0)
+if (virBitmapParse(data, ,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask);
@@ -498,17 +474,7 @@ xenParseXLVnuma(virConf *conf,
size_t i, ndistances;
unsigned int value;
-if (virStrcpyStatic(vtoken, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("vnuma vnode %zu vdistances '%s'
too long for destination"),
- vnodeCnt, data);
-goto cleanup;
-}
-
-VIR_FREE(tmp);
-tmp = g_strdup(vtoken);
-
-if (!(token = g_strsplit(tmp, ",", 0)))
+if (!(token = g_strsplit(data, ",", 0)))
goto cleanup;
ndistances = g_strv_length(token);
@@ -571,7 +537,6 @@ xenParseXLVnuma(virConf *conf,
cleanup:
if (ret)
VIR_FREE(cpu);
-VIR_FREE(tmp);
return ret;
}
--
2.34.1
[PATCH 10/11] libxl: Remove needless labels
After previous cleanups some labels are needless: they contain
nothing but a return statement. Drop such labels and return
directly.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_common.c | 7 ++--
src/libxl/xen_xl.c | 80 ++
src/libxl/xen_xm.c | 7 ++--
3 files changed, 31 insertions(+), 63 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index 2d363ac2fb..c3fa98b71d 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -2313,17 +2313,14 @@ xenFormatVif(virConf *conf,
for (i = 0; i < def->nnets; i++) {
if (xenFormatNet(conn, netVal, def->nets[i],
hvm, vif_typename) < 0)
-goto cleanup;
+return -1;
}
if (netVal->list != NULL &&
virConfSetValue(conf, "vif", ) < 0)
-goto cleanup;
+return -1;
return 0;
-
- cleanup:
-return -1;
}
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 25bf1f7893..e3a87890af 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -392,7 +392,6 @@ static int
xenParseXLVnuma(virConf *conf,
virDomainDef *def)
{
-int ret = -1;
size_t vcpus = 0;
size_t nr_nodes = 0;
size_t vnodeCnt = 0;
@@ -416,7 +415,7 @@ xenParseXLVnuma(virConf *conf,
}
if (!virDomainNumaSetNodeCount(numa, nr_nodes))
-goto cleanup;
+return -1;
cpu = virCPUDefNew();
@@ -439,7 +438,7 @@ xenParseXLVnuma(virConf *conf,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("vnuma vnode invalid format '%s'"),
str);
-goto cleanup;
+return -1;
}
data++;
@@ -452,18 +451,18 @@ xenParseXLVnuma(virConf *conf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vnuma vnode %zu contains invalid
pnode value '%s'"),
vnodeCnt, data);
-goto cleanup;
+return -1;
}
pnode = cellid;
} else if (STRPREFIX(str, "size")) {
if (virStrToLong_ull(data, NULL, 10, ) < 0)
-goto cleanup;
+return -1;
virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize
* 1024));
} else if (STRPREFIX(str, "vcpus")) {
if (virBitmapParse(data, ,
VIR_DOMAIN_CPUMASK_LEN) < 0)
-goto cleanup;
+return -1;
virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask);
vcpus += virBitmapCountBits(cpumask);
@@ -474,7 +473,7 @@ xenParseXLVnuma(virConf *conf,
unsigned int value;
if (!(token = g_strsplit(data, ",", 0)))
-goto cleanup;
+return -1;
ndistances = g_strv_length(token);
@@ -482,23 +481,23 @@ xenParseXLVnuma(virConf *conf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vnuma pnode %d configured '%s'
(count %zu) doesn't fit the number of specified vnodes %zu"),
pnode, str, ndistances, nr_nodes);
-goto cleanup;
+return -1;
}
if (virDomainNumaSetNodeDistanceCount(numa, vnodeCnt,
ndistances) != ndistances)
-goto cleanup;
+return -1;
for (i = 0; i < ndistances; i++) {
if ((virStrToLong_ui(token[i], NULL, 10, ) <
0) ||
(virDomainNumaSetNodeDistance(numa, vnodeCnt,
i, value) != value))
-goto cleanup;
+return -1;
}
} else {
virReportError(VIR_ERR_CONF_SYNTAX,
_("Invalid vnuma configuration for
vnode %zu"),
vnodeCnt);
-goto cleanup;
+return -1;
}
}
vnode = vnode->next;
@@ -511,7 +510,7 @@ xenParseXLVnuma(virConf *conf,
virReportError(VIR_ERR_CONF_SYNTAX,
_("Incomplete vnuma configuration for vnode %zu"),
vnodeCnt);
-goto cleanup;
+return -1;
}
list = list->next;
@@ -525,16 +524,13 @@ xenParseXLVnuma(virConf *conf,
[PATCH 09/11] xen_xl.c: Use g_autoptr() for virCPUDef
In xenParseXLVnuma() the @cpu variable is freed explicitly. However, when switched to g_autoptr(virCPUDef) the explicit call can be removed. Signed-off-by: Michal Privoznik --- src/libxl/xen_xl.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 043f3c27db..25bf1f7893 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -396,7 +396,7 @@ xenParseXLVnuma(virConf *conf, size_t vcpus = 0; size_t nr_nodes = 0; size_t vnodeCnt = 0; -virCPUDef *cpu = NULL; +g_autoptr(virCPUDef) cpu = NULL; virConfValue *list; virConfValue *vnode; virDomainNuma *numa; @@ -529,14 +529,11 @@ xenParseXLVnuma(virConf *conf, } cpu->type = VIR_CPU_TYPE_GUEST; -def->cpu = cpu; +def->cpu = g_steal_pointer(); ret = 0; cleanup: -if (ret) -VIR_FREE(cpu); - return ret; } -- 2.34.1
[PATCH 08/11] xen_xl.c: Use g_autofree more
There are few places inside src/libxl/xen_xl.c that can benefit
from g_autofree. Let them use automatic memory freeing.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_xl.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index f32a6cd65a..043f3c27db 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -321,10 +321,11 @@ xenParseXLSpice(virConf *conf, virDomainDef *def)
{
virDomainGraphicsDef *graphics = NULL;
unsigned long port;
-char *listenAddr = NULL;
int val;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+g_autofree char *listenAddr = NULL;
+
if (xenConfigGetBool(conf, "spice", , 0) < 0)
return -1;
@@ -335,7 +336,6 @@ xenParseXLSpice(virConf *conf, virDomainDef *def)
goto cleanup;
if (virDomainGraphicsListenAppendAddress(graphics, listenAddr) < 0)
goto cleanup;
-VIR_FREE(listenAddr);
if (xenConfigGetULong(conf, "spicetls_port", , 0) < 0)
goto cleanup;
@@ -384,7 +384,6 @@ xenParseXLSpice(virConf *conf, virDomainDef *def)
return 0;
cleanup:
-VIR_FREE(listenAddr);
virDomainGraphicsDefFree(graphics);
return -1;
}
@@ -575,7 +574,6 @@ xenParseXLXenbusLimits(virConf *conf, virDomainDef *def)
static int
xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr)
{
-char *tmpstr = NULL;
int ret = -1;
/* A NULL source is valid, e.g. an empty CDROM */
@@ -583,6 +581,8 @@ xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr)
return 0;
if (STRPREFIX(srcstr, "rbd:")) {
+g_autofree char *tmpstr = NULL;
+
if (!(tmpstr = virStringReplace(srcstr, "", "\\")))
goto cleanup;
@@ -596,7 +596,6 @@ xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr)
}
cleanup:
-VIR_FREE(tmpstr);
return ret;
}
@@ -952,13 +951,13 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
{
virConfValue *list = virConfGetValue(conf, "channel");
virDomainChrDef *channel = NULL;
-char *name = NULL;
-char *path = NULL;
if (list && list->type == VIR_CONF_LIST) {
list = list->list;
while (list) {
g_autofree char *type = NULL;
+g_autofree char *name = NULL;
+g_autofree char *path = NULL;
char *key;
if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
@@ -1003,7 +1002,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
channel->source->data.nix.path = g_steal_pointer();
} else if (STRPREFIX(type, "pty")) {
channel->source->type = VIR_DOMAIN_CHR_TYPE_PTY;
-VIR_FREE(path);
} else {
goto cleanup;
}
@@ -1023,8 +1021,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
cleanup:
virDomainChrDefFree(channel);
-VIR_FREE(path);
-VIR_FREE(name);
return -1;
}
@@ -1382,7 +1378,6 @@ xenFormatXLVnuma(virConfValue *list,
list->list = numaVnode;
ret = 0;
-VIR_FREE(nodeVcpus);
return ret;
}
--
2.34.1
[PATCH 07/11] libxl: Allocate @libxldisk in xenParseXLDisk() on stack
In xenParseXLDisk() the @libxldisk variable (which is type of
libxl_device_disk) is allocated on heap. But this is not
necessary as nothing in the function needs that approach.
Allocate the variable on the stack and drop corresponding
VIR_FREE() call.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_xl.c | 35 ---
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 3a4e21ee0e..f32a6cd65a 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -641,11 +641,9 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
int ret = -1;
virConfValue *list = virConfGetValue(conf, "disk");
XLU_Config *xluconf;
-libxl_device_disk *libxldisk;
+libxl_device_disk libxldisk;
virDomainDiskDef *disk = NULL;
-libxldisk = g_new0(libxl_device_disk, 1);
-
if (!(xluconf = xlu_cfg_init(stderr, "command line")))
goto cleanup;
@@ -657,23 +655,23 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
if (list->type != VIR_CONF_STRING || list->str == NULL)
goto skipdisk;
-libxl_device_disk_init(libxldisk);
+libxl_device_disk_init();
-if (xlu_disk_parse(xluconf, 1, _spec, libxldisk))
+if (xlu_disk_parse(xluconf, 1, _spec, ))
goto fail;
if (!(disk = virDomainDiskDefNew(NULL)))
goto fail;
-if (xenParseXLDiskSrc(disk, libxldisk->pdev_path) < 0)
+if (xenParseXLDiskSrc(disk, libxldisk.pdev_path) < 0)
goto fail;
-disk->dst = g_strdup(libxldisk->vdev);
+disk->dst = g_strdup(libxldisk.vdev);
-disk->src->readonly = !libxldisk->readwrite;
-disk->removable = libxldisk->removable;
+disk->src->readonly = !libxldisk.readwrite;
+disk->removable = libxldisk.removable;
-if (libxldisk->is_cdrom) {
+if (libxldisk.is_cdrom) {
virDomainDiskSetDriver(disk, "qemu");
virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
@@ -683,7 +681,7 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
else
disk->src->format = VIR_STORAGE_FILE_RAW;
} else {
-switch (libxldisk->format) {
+switch (libxldisk.format) {
case LIBXL_DISK_FORMAT_QCOW:
disk->src->format = VIR_STORAGE_FILE_QCOW;
break;
@@ -711,11 +709,11 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk image format not supported: %s"),
-
libxl_disk_format_to_string(libxldisk->format));
+
libxl_disk_format_to_string(libxldisk.format));
goto fail;
}
-switch (libxldisk->backend) {
+switch (libxldisk.backend) {
case LIBXL_DISK_BACKEND_QDISK:
case LIBXL_DISK_BACKEND_UNKNOWN:
virDomainDiskSetDriver(disk, "qemu");
@@ -735,22 +733,22 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk backend not supported: %s"),
-
libxl_disk_backend_to_string(libxldisk->backend));
+
libxl_disk_backend_to_string(libxldisk.backend));
goto fail;
}
}
-if (STRPREFIX(libxldisk->vdev, "xvd") ||
+if (STRPREFIX(libxldisk.vdev, "xvd") ||
def->os.type != VIR_DOMAIN_OSTYPE_HVM)
disk->bus = VIR_DOMAIN_DISK_BUS_XEN;
-else if (STRPREFIX(libxldisk->vdev, "sd"))
+else if (STRPREFIX(libxldisk.vdev, "sd"))
disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
else
disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk);
-libxl_device_disk_dispose(libxldisk);
+libxl_device_disk_dispose();
skipdisk:
list = list->next;
@@ -761,11 +759,10 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
cleanup:
virDomainDiskDefFree(disk);
xlu_cfg_destroy(xluconf);
-VIR_FREE(libxldisk);
return ret;
fail:
-libxl_device_disk_dispose(libxldisk);
+libxl_device_disk_dispose();
goto cleanup;
}
--
2.34.1
[PATCH 01/11] storage_file: Declare virStorageSourceParseRBDColonString only in one header
The virStorageSourceParseRBDColonString() function is declared in src/storage_file/storage_source.h and src/storage_file/storage_source_backingstore.h but implemented only in the .c that corresponds to the latter header file. Therefore, drop declaration from storage_source.h as the function is not implemented in its corresponding .c file. Signed-off-by: Michal Privoznik --- src/libxl/xen_xl.c | 1 + src/storage_file/storage_source.h | 5 - src/storage_file/storage_source_backingstore.h | 3 ++- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 7604e3d534..94268fb76d 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -30,6 +30,7 @@ #include "viralloc.h" #include "virstring.h" #include "storage_source.h" +#include "storage_source_backingstore.h" #include "xen_xl.h" #include "libxl_capabilities.h" #include "libxl_conf.h" diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 5aaf4c356a..0ae06f4e7d 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -66,11 +66,6 @@ int virStorageSourceNewFromBacking(virStorageSource *parent, virStorageSource **backing); -int -virStorageSourceParseRBDColonString(const char *rbdstr, -virStorageSource *src) -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int virStorageSourceGetRelativeBackingPath(virStorageSource *top, virStorageSource *base, diff --git a/src/storage_file/storage_source_backingstore.h b/src/storage_file/storage_source_backingstore.h index f5ec8e9154..e7e2a58eb9 100644 --- a/src/storage_file/storage_source_backingstore.h +++ b/src/storage_file/storage_source_backingstore.h @@ -29,7 +29,8 @@ virStorageSourceParseBackingURI(virStorageSource *src, int virStorageSourceParseRBDColonString(const char *rbdstr, -virStorageSource *src); +virStorageSource *src) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageSourceParseBackingColon(virStorageSource *src, -- 2.34.1
[PATCH 05/11] virconf: Make virConfSetValue() clear consumed pointer
The way that virConfSetValue() works (and the way it is even
documented) is that the @value pointer is always consumed.
However, since the first order pointer is passed it leaves
callers in a pickle situation - they always have to set pointer
to NULL after calling virConfSetValue() to avoid touching it.
Let's switch @value to a double pointer and clear it inside the
function.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_common.c | 37 +-
src/libxl/xen_xl.c | 60 +-
src/libxl/xen_xm.c | 9 +++
src/util/virconf.c | 16 +++
src/util/virconf.h | 2 +-
5 files changed, 46 insertions(+), 78 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index f36e269aab..2d363ac2fb 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -259,7 +259,7 @@ xenConfigSetInt(virConf *conf, const char *setting, long
long l)
value->next = NULL;
value->l = l;
-return virConfSetValue(conf, setting, value);
+return virConfSetValue(conf, setting, );
}
@@ -274,7 +274,7 @@ xenConfigSetString(virConf *conf, const char *setting,
const char *str)
value->next = NULL;
value->str = g_strdup(str);
-return virConfSetValue(conf, setting, value);
+return virConfSetValue(conf, setting, );
}
@@ -1788,12 +1788,9 @@ xenFormatPCI(virConf *conf, virDomainDef *def)
}
}
-if (pciVal->list != NULL) {
-int ret = virConfSetValue(conf, "pci", pciVal);
-pciVal = NULL;
-if (ret < 0)
-return -1;
-}
+if (pciVal->list != NULL &&
+virConfSetValue(conf, "pci", ) < 0)
+return -1;
return 0;
}
@@ -2000,13 +1997,9 @@ xenFormatCharDev(virConf *conf, virDomainDef *def,
}
}
-if (serialVal->list != NULL) {
-int ret = virConfSetValue(conf, "serial", serialVal);
-
-serialVal = NULL;
-if (ret < 0)
-return -1;
-}
+if (serialVal->list != NULL &&
+virConfSetValue(conf, "serial", ) < 0)
+return -1;
}
} else {
if (xenConfigSetString(conf, "serial", "none") < 0)
@@ -2264,11 +2257,8 @@ xenFormatVfb(virConf *conf, virDomainDef *def)
vfb->type = VIR_CONF_LIST;
vfb->list = g_steal_pointer();
-if (virConfSetValue(conf, "vfb", vfb) < 0) {
-vfb = NULL;
+if (virConfSetValue(conf, "vfb", ) < 0)
return -1;
-}
-vfb = NULL;
}
}
@@ -2326,12 +2316,9 @@ xenFormatVif(virConf *conf,
goto cleanup;
}
-if (netVal->list != NULL) {
-int ret = virConfSetValue(conf, "vif", netVal);
-netVal = NULL;
-if (ret < 0)
-goto cleanup;
-}
+if (netVal->list != NULL &&
+virConfSetValue(conf, "vif", ) < 0)
+goto cleanup;
return 0;
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 815c1216f7..6e489f35ad 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -1447,12 +1447,9 @@ xenFormatXLDomainVnuma(virConf *conf,
goto cleanup;
}
-if (vnumaVal->list != NULL) {
-int ret = virConfSetValue(conf, "vnuma", vnumaVal);
-vnumaVal = NULL;
-if (ret < 0)
-return -1;
-}
+if (vnumaVal->list != NULL &&
+virConfSetValue(conf, "vnuma", ) < 0)
+return -1;
return 0;
@@ -1697,12 +1694,9 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def)
goto cleanup;
}
-if (diskVal->list != NULL) {
-int ret = virConfSetValue(conf, "disk", diskVal);
-diskVal = NULL;
-if (ret < 0)
-return -1;
-}
+if (diskVal->list != NULL &&
+virConfSetValue(conf, "disk", ) < 0)
+return -1;
return 0;
@@ -1848,11 +1842,8 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def)
if (xenConfigSetString(conf, "usbdevice",
usbdevices->list->str) < 0)
goto error;
} else {
-if (virConfSetValue(conf, "usbdevice", usbdevices) < 0) {
-usbdevices = NULL;
+if (virConfSetValue(conf, "usbdevice", ) < 0)
goto error;
-}
-usbdevices = NULL;
}
}
}
@@ -1923,12 +1914,9 @@ xenFormatXLUSBController(virConf *conf,
}
}
-if (usbctrlVal->list != NULL) {
-int ret = virConfSetValue(conf, "usbctrl", usbctrlVal);
-usbctrlVal = NULL;
-if (ret < 0)
-return -1;
-}
+if (usbctrlVal->list != NULL &&
+virConfSetValue(conf, "usbctrl", ) < 0)
+return -1;
return 0;
@@ -1985,12 +1973,9 @@
[PATCH 04/11] src: Declare and use g_autoptr(virConfValue)
This commit declares g_autoptr() function for virConfValue type.
At the same time, it switches variable declarations to use it.
Also, in a few places we might have freed a variable twice, for
instance in xenFormatXLDomainNamespaceData(). This is because
virConfSetValue() consumes passed pointer (@value) even in case
of failure and thus any code that uses virConfSetValue() must
refrain from touching @value and it must not call
virConfFreeValue().
This semantic is not obvious and will be addressed in one of
future commits.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_common.c | 28 +---
src/libxl/xen_xl.c | 36 +---
src/libxl/xen_xm.c | 4 +---
src/util/virconf.h | 1 +
4 files changed, 28 insertions(+), 41 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index e68ca1cde3..f36e269aab 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -1729,7 +1729,7 @@ xenFormatNet(virConnectPtr conn,
static int
xenFormatPCI(virConf *conf, virDomainDef *def)
{
-virConfValue *pciVal = NULL;
+g_autoptr(virConfValue) pciVal = NULL;
int hasPCI = 0;
size_t i;
@@ -1794,7 +1794,6 @@ xenFormatPCI(virConf *conf, virDomainDef *def)
if (ret < 0)
return -1;
}
-VIR_FREE(pciVal);
return 0;
}
@@ -1970,7 +1969,7 @@ xenFormatCharDev(virConf *conf, virDomainDef *def,
} else {
size_t j = 0;
int maxport = -1, port;
-virConfValue *serialVal = NULL;
+g_autoptr(virConfValue) serialVal = NULL;
if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1997,7 +1996,6 @@ xenFormatCharDev(virConf *conf, virDomainDef *def,
}
if (xenFormatSerial(serialVal, chr) < 0) {
-VIR_FREE(serialVal);
return -1;
}
}
@@ -2009,7 +2007,6 @@ xenFormatCharDev(virConf *conf, virDomainDef *def,
if (ret < 0)
return -1;
}
-VIR_FREE(serialVal);
}
} else {
if (xenConfigSetString(conf, "serial", "none") < 0)
@@ -2224,8 +2221,8 @@ xenFormatVfb(virConf *conf, virDomainDef *def)
return -1;
}
} else {
-virConfValue *vfb;
-virConfValue *disp;
+g_autoptr(virConfValue) vfb = NULL;
+g_autoptr(virConfValue) disp = NULL;
char *vfbstr = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
@@ -2259,16 +2256,19 @@ xenFormatVfb(virConf *conf, virDomainDef *def)
vfbstr = virBufferContentAndReset();
-vfb = g_new0(virConfValue, 1);
disp = g_new0(virConfValue, 1);
-
-vfb->type = VIR_CONF_LIST;
-vfb->list = disp;
disp->type = VIR_CONF_STRING;
disp->str = vfbstr;
-if (virConfSetValue(conf, "vfb", vfb) < 0)
+vfb = g_new0(virConfValue, 1);
+vfb->type = VIR_CONF_LIST;
+vfb->list = g_steal_pointer();
+
+if (virConfSetValue(conf, "vfb", vfb) < 0) {
+vfb = NULL;
return -1;
+}
+vfb = NULL;
}
}
@@ -2312,7 +2312,7 @@ xenFormatVif(virConf *conf,
virDomainDef *def,
const char *vif_typename)
{
-virConfValue *netVal = NULL;
+g_autoptr(virConfValue) netVal = NULL;
size_t i;
int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
@@ -2333,11 +2333,9 @@ xenFormatVif(virConf *conf,
goto cleanup;
}
-VIR_FREE(netVal);
return 0;
cleanup:
-virConfFreeValue(netVal);
return -1;
}
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index e3ddae8827..815c1216f7 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -1429,7 +1429,7 @@ xenFormatXLDomainVnuma(virConf *conf,
virDomainDef *def)
{
virDomainNuma *numa = def->numa;
-virConfValue *vnumaVal;
+g_autoptr(virConfValue) vnumaVal = NULL;
size_t i;
size_t nr_nodes;
@@ -1453,12 +1453,10 @@ xenFormatXLDomainVnuma(virConf *conf,
if (ret < 0)
return -1;
}
-VIR_FREE(vnumaVal);
return 0;
cleanup:
-virConfFreeValue(vnumaVal);
return -1;
}
@@ -1683,7 +1681,7 @@ xenFormatXLDisk(virConfValue *list, virDomainDiskDef
*disk)
static int
xenFormatXLDomainDisks(virConf *conf, virDomainDef *def)
{
-virConfValue *diskVal;
+g_autoptr(virConfValue) diskVal = NULL;
size_t i;
diskVal = g_new0(virConfValue, 1);
@@ -1705,12 +1703,10 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def)
if (ret < 0)
return -1;
}
-
[PATCH 00/11] Misc cleanups
Couple of cleanups I've done whilst looking around our code base. Michal Prívozník (11): storage_file: Declare virStorageSourceParseRBDColonString only in one header virconf: Report an error in when virConfSetValue() fails xen_xl: Check for virConfSetValue() retval src: Declare and use g_autoptr(virConfValue) virconf: Make virConfSetValue() clear consumed pointer libxl: Don't use a static buffer in xenParseXLVnuma() libxl: Allocate @libxldisk in xenParseXLDisk() on stack xen_xl.c: Use g_autofree more xen_xl.c: Use g_autoptr() for virCPUDef libxl: Remove needless labels virsh: Remove needless labels src/libxl/xen_common.c| 62 ++--- src/libxl/xen_xl.c| 259 ++ src/libxl/xen_xm.c| 18 +- src/storage_file/storage_source.h | 5 - .../storage_source_backingstore.h | 3 +- src/util/virconf.c| 19 +- src/util/virconf.h| 3 +- tools/virsh-host.c| 20 +- tools/vsh.c | 11 +- 9 files changed, 135 insertions(+), 265 deletions(-) -- 2.34.1
[PATCH 02/11] virconf: Report an error in when virConfSetValue() fails
Callers of virConfSetValue() don't report any error, they just
pass the error blindly. Therefore, report an error when
virConfSetValue() is about to fail.
Signed-off-by: Michal Privoznik
---
src/util/virconf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 07ecfc7b57..29b3622791 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -1329,6 +1329,9 @@ virConfSetValue(virConf *conf,
virConfEntry *prev = NULL;
if (value && value->type == VIR_CONF_STRING && value->str == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("expecting a value for value of type %s"),
+ virConfTypeToString(VIR_CONF_STRING));
virConfFreeValue(value);
return -1;
}
--
2.34.1
[PATCH 03/11] xen_xl: Check for virConfSetValue() retval
There's one case where the return value of virConfSetValue() is
not checked for and it's in xenFormatXLInputDevs() function.
Let's fix that.
Signed-off-by: Michal Privoznik
---
src/libxl/xen_xl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 94268fb76d..e3ddae8827 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -1853,7 +1853,11 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def)
goto error;
virConfFreeValue(usbdevices);
} else {
-virConfSetValue(conf, "usbdevice", usbdevices);
+if (virConfSetValue(conf, "usbdevice", usbdevices) < 0) {
+usbdevices = NULL;
+goto error;
+}
+usbdevices = NULL;
}
} else {
VIR_FREE(usbdevices);
--
2.34.1
Release of libvirt-8.0.0
The 8.0.0 release of both libvirt and libvirt-python is tagged and signed tarballs and source RPMs are available at https://libvirt.org/sources/ https://libvirt.org/sources/python/ Thanks everybody who helped with this release by sending patches, reviewing, testing, or providing feedback. Your work is greatly appreciated. * Security * libxl: Fix potential deadlock and crash (CVE-2021-4147) A rogue guest could continuously reboot itself and cause libvirtd on the host to deadlock or crash, resulting in a denial of service condition. * Removed features * qemu: Explicitly forbid live changing nodeset for strict numatune For ``strict`` mode of it can't be guaranteed that memory is moved completely onto new set of nodes (e.g. QEMU might have locked pieces of its memory) thus breaking the strict promise. If live migration of QEMU memory between NUMA nodes is desired, users are advised to use ``restrictive`` mode instead. * New features * qemu: Synchronous write mode for disk copy operations The ``blockdev-mirror`` block job supports a mode where writes from the VM are synchronously propagated to the destination of the copy. This ensures that the job will converge under heavy I/O. Implement the mode for the copy blockjob as ``VIR_DOMAIN_BLOCK_COPY_SYNCHRONOUS_WRITES`` flag exposed via ``virsh blockcopy --synchronous-writes`` and for non-shared storage migration as ``VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES`` exposed via ``virsh migrate --copy-storage-synchronous-writes``. * Introduce TCG domain features Libvirt is now able to set the size of translation block cache size (tb-size) for TCG domains. * qemu: Add new API to inject a launch secret in a domain New API ``virDomainSetLaunchSecurityState()`` and virsh command ``domsetlaunchsecstate`` are added to support injecting a launch secret in a domain's memory. * Improvements * libxl: Implement the virDomainGetMessages API * qemu: Preserve qcow2 sub-cluster allocation state after external snapshots and block-copy The new image which is installed as an overlay on top of the current chain when taking an external snapshot, or the target of a block copy operation now enables sub-cluster allocation (``extended_l2``) if the original image has the option enabled. * Bug fixes * qemu: Fix device hot-unplug with ``libvirt-7.9`` or ``libvirt-7.10`` used with ``qemu-6.2`` An internal change to the configuration format used by the above libvirt versions triggers a bug in ``qemu-6.2`` where qemu no longer emits the event notifying that the device was unplugged successfully and thus libvirt never removes the device from the definition. This impacts only devices which were present at startup of the VM, hotplugged devices behave correctly. This is fixed in ``libvirt-8.0`` by reverting to the old configuration approach until qemu is fixed. As a workaround for ``libvirt-7.9`` and ``libvirt-7.10`` the old configuration approach can be forced by: Option 1, global ``qemu.conf``:: capability_filters = [ "device.json" ] Option 2, per VM XML override:: [...] * Fix sparse streams with split daemon In split daemon scenario, a client connected to a hypervisor driver and using sparse streams (e.g. ``virsh vol-download --sparse``) would make the hypervisor daemon enter an infinite loop without any data transfer. This is now fixed. * Build no longer requires RPC library Code and its cross dependencies were fixed so that build without remote driver and thus an RPC library (like ``tirpc``) fails no more. * virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl When a was defined for a TAP device that's plugged into an OvS bridge values passed to the OvS were incorrectly recalculated resulting in slightly different limits being applied. Enjoy. Jirka
Re: [PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.
> > > index 144ba4dd12..a8f41dc8c2 100644
> > > --- a/src/conf/domain_conf.h
> > > +++ b/src/conf/domain_conf.h
> > > @@ -1187,6 +1187,12 @@ typedef enum {
> > > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST
> > > } virDomainChrConsoleTargetType;
> > >
> > > +/*
> > > + * The value of VIR_MAX_ISA_SERIAL_PORTS corresponds to
> > > MAX_ISA_SERIAL_PORTS
> > > + * set in qemu code base.
> >
> > We prefer upper case spelling of QEMU. And it's been a long time since
> > I've last booted up my machine with ISA, but IIRC it could only have 4
> > COM ports, so maybe the limit doesn't come from QEMU really but BIOS of
> > that time? What I'm trying to say is, if this is a limit shared across
> > other hypervisors then it can live under src/conf/ but if isn't shared
> > then it has to go under hypervisor specific dir (src/qemu/ in this case).
> >
> > I'm just going to assume the limit is shared and not QEMU specific.
Ok I spoke to Peter about the following commit on @qemu-devel. The
underlying limitation is indeed dictated by the hw it is emulating. Fair
enough! I still think though that we should leave the above comment as is
because this validation in libvirt is dependent on what Qemu allows today.
The reference is important IMHO.
>
> Maybe I read this wrong but I interpreted this commit message in QEMU
> repo to mean that the limitation is qemu specific :
>
> commit def337ffda34d331404bd7f1a42726b71500df22
> Author: Peter Maydell
> Date: Fri Apr 20 15:52:46 2018 +0100
>
> serial-isa: Use MAX_ISA_SERIAL_PORTS instead of MAX_SERIAL_PORTS
>
> The ISA serial port handling in serial-isa.c imposes a limit
> of 4 serial ports. This is because we only know of 4 IO port
> and IRQ settings for them, and is unrelated to the generic
> MAX_SERIAL_PORTS limit, though they happen to both be set at
> 4 currently.
>
> Use a new MAX_ISA_SERIAL_PORTS wherever that is the correct
> limit to be checking against.
>
> Signed-off-by: Peter Maydell
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Thomas Huth
> Tested-by: Philippe Mathieu-Daudé
> Message-id: 20180420145249.32435-11-peter.mayd...@linaro.org
>
>
