Re: [PATCH] remote: Add libvirtd dependency to virt-guest-shutdown.target
On 1/27/21 12:58 AM, Christian Ehrhardt wrote: On Wed, Nov 4, 2020 at 7:47 AM Neal Gompa wrote: On Tue, Nov 3, 2020 at 9:26 PM Jim Fehlig wrote: When restarting libvirt services and sockets *and* libvirt-guests.service is running, the latter will sometimes hang when trying to connect to libvirtd. Even though libvirt-guests has 'Wants=libvirtd.service' and 'After=libvirtd.service', we can see via journalctl that it is not shutdown before libvirtd when executing something like systemctl try-restart libvirtd.service libvirtd.socket \ libvirtd-ro.socket virtlockd.service virtlockd.socket \ virtlogd.service virtlogd.socket virt-guest-shutdown.target ... Adding 'Requires=libvirtd.service' to virt-guest-shutdown.target results in expected behavior Hi Jim (and all others), This change is accepted since 6.10 [1] - while testing 7.0 for Debian I've found that this change regresses us bringing back bug 955216 [2] for us. Based on how the require statement works I think it is bringing the same issue to everyone (not just Debian/Ubuntu). Nod. I verified the same behavior on SUSE :-(. That issue is - once you restart libvirtd.service (common on upgrades) it will now also always cycle libvirt-guests.service through a restart. But cycling libvirt-guests.service means shutting down and starting all guests which suddenly makes upgrading libvirt very disruptive. Agreed! For the time being I've suggested we revert this change in Debian/Ubuntu, but I wonder if there could be something that ensures the ordering Jim wanted to achieve without hard-linking (via ) a libvirtd.service restart to also restart libvirt-guests.service? Suggestions and/or comments very welcomed! I don't have other ideas ATM. In the meantime I've sent a patch to revert the commit https://www.redhat.com/archives/libvir-list/2021-January/msg01198.html Regards, Jim --- log showing that libvirtd.service now restarts libvirt-guests as well --- root@h-libvirt-netcheck-up:~# systemctl status libvirt-guests.service ● libvirt-guests.service - Suspend/Resume Running libvirt Guests Loaded: loaded (/lib/systemd/system/libvirt-guests.service; enabled; vendor preset: enabled) Active: active (exited) since Wed 2021-01-27 06:33:41 UTC; 1h 19min ago Docs: man:libvirtd(8) https://libvirt.org Main PID: 56753 (code=exited, status=0/SUCCESS) Tasks: 0 (limit: 38269) Memory: 0B CGroup: /system.slice/libvirt-guests.service Jan 27 06:33:41 h-libvirt-netcheck-up systemd[1]: Starting Suspend/Resume Running libvirt Guests... Jan 27 06:33:41 h-libvirt-netcheck-up systemd[1]: Finished Suspend/Resume Running libvirt Guests. root@h-libvirt-netcheck-up:~# systemctl restart libvirtd root@h-libvirt-netcheck-up:~# systemctl status libvirt-guests.service ● libvirt-guests.service - Suspend/Resume Running libvirt Guests Loaded: loaded (/lib/systemd/system/libvirt-guests.service; enabled; vendor preset: enabled) Active: active (exited) since Wed 2021-01-27 07:52:57 UTC; 4s ago Docs: man:libvirtd(8) https://libvirt.org Process: 57626 ExecStart=/usr/lib/libvirt/libvirt-guests.sh start (code=exited, status=0/SUCCESS) Main PID: 57626 (code=exited, status=0/SUCCESS) Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Starting Suspend/Resume Running libvirt Guests... Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Finished Suspend/Resume Running libvirt Guests. You see new PID, and entries that it stopped/started all guests. --- journal while the above restarts happened --- Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopping Suspend/Resume Running libvirt Guests... Jan 27 07:52:57 h-libvirt-netcheck-up libvirt-guests.sh[57593]: Running guests on default URI: Jan 27 07:52:57 h-libvirt-netcheck-up libvirt-guests.sh[57602]: no running guests. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: libvirt-guests.service: Succeeded. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopped Suspend/Resume Running libvirt Guests. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopped target Libvirt guests shutdown. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopping Libvirt guests shutdown. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopping Virtualization daemon... Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: libvirtd.service: Succeeded. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: libvirtd.service: Unit process 343 (dnsmasq) remains running after unit stopped. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: libvirtd.service: Unit process 344 (dnsmasq) remains running after unit stopped. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: Stopped Virtualization daemon. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: libvirtd.service: Found left-over process 343 (dnsmasq) in control group while starting unit. Ignoring. Jan 27 07:52:57 h-libvirt-netcheck-up systemd[1]: This usually indicates unclean termination of a previous run, or
[PATCH] Revert "remote: Add libvirtd dependency to virt-guest-shutdown.target"
Further testing revealed commit f035f53baa regresses Debian bug 955216 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=955216 Restarting libvirt-guests on libvirtd restart is worse than the original dependency issue, so revert the commit until a better solution is found. This reverts commit f035f53baa2e5dc00b8e866e594672a90b4cea78. Signed-off-by: Jim Fehlig --- src/remote/virt-guest-shutdown.target | 1 - 1 file changed, 1 deletion(-) diff --git a/src/remote/virt-guest-shutdown.target b/src/remote/virt-guest-shutdown.target index e2efa3e63a..25d4aaa267 100644 --- a/src/remote/virt-guest-shutdown.target +++ b/src/remote/virt-guest-shutdown.target @@ -1,4 +1,3 @@ [Unit] Description=Libvirt guests shutdown -Requires=libvirtd.service Documentation=https://libvirt.org -- 2.29.2
Re: [libvirt PATCH v3 00/21] Add support for persistent mediated devices
On Thu, 7 Jan 2021 17:43:54 +0100 Erik Skultety wrote: > > > > 4.Define a mdev device with the uuid specified, but the mdev device > > defined seems using another uuid. Maybe it make a little confusion > > about the use of uuid in the xml: > > #cat mdev.xml > > > > mdev_85531b6d_e5e4_41c1_aa2a_8844717f066a > > Yeah, the easy way out here is to document that the element is > read only, but that would be wrong, because we allow specifying it > for domains, networks, interfaces, etc. So, we should give the end > user the option to specify whatever name they want and generate one > if none is provided. > Unfortunately, this appears to be difficult to achieve for persistent devices. For mediated devices, we are using mdevctl as our backend persistent device definition storage. But mdevctl doesn't provide a way to attach additional metadata along with a mdev definition. So we would need to either modify mdevctl to allow us to store additional data with a device definition, or we would need to create (and keep synchronized) a separate persistent storage for libvirt-specific metadata about mediated devices. It's also worth noting that 'nodedev-create' previously allowed you to create vHBA devices in libvirt, but the 'name' element is explicitly ignored (see https://wiki.libvirt.org/page/NPIV_in_libvirt): "NOTE: If you specify "name" for the vHBA, then it will be ignored. The kernel will automatically pick the next SCSI host name in sequence not already used." That said, I do think that it will be useful or even necessary to be able to create mdevs with a specific UUID, but I think that's a separate issue than being able to specify the 'name' of the nodedev. It seems better to do that by specifying a 'uuid' element within the mdev caps, rather than trying to parse a UUID from an arbitrary name string. For example: 901891a6-2077-4476-9465-53d8995b81b4 pci__00_02_0 Thoughts? Jonathon
Re: [libvirt PATCH 0/7] Add boot order to virtiofs
On 1/28/21 4:15 PM, Ján Tomko wrote: Sadly, the replies changes for older QEMUs are synthetic. Separated for easier review. Also available on gitlab: git fetch https://gitlab.com/janotomko/libvirt/ virtiofs-bootindex https://gitlab.com/janotomko/libvirt/-/tree/virtiofs-bootindex And a broken pipeline: https://gitlab.com/janotomko/libvirt/-/pipelines/248162273 Ján Tomko (7): tests: switch vhost-user-fs-hugepages to use boot order conf: add boot order to filesystem qemu: add QEMU_CAPS_VHOST_USER_FS_BOOTINDEX fixup: vhost-user-fs-device properties fixup: renumber Add validation for virtiofs boot order setting qemu: format bootindex for vhost-user-fs docs/schemas/domaincommon.rng | 3 + src/conf/domain_conf.c| 5 +- src/conf/domain_validate.c| 17 ++- src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 3 + src/qemu/qemu_validate.c | 6 + .../caps_4.2.0.aarch64.replies| 131 .../caps_4.2.0.s390x.replies | 119 --- .../caps_4.2.0.x86_64.replies | 131 .../caps_5.0.0.aarch64.replies| 136 + .../caps_5.0.0.ppc64.replies | 124 +--- .../caps_5.0.0.riscv64.replies| 120 --- .../caps_5.0.0.x86_64.replies | 136 + .../caps_5.1.0.x86_64.replies | 136 + .../caps_5.2.0.aarch64.replies| 136 + .../caps_5.2.0.ppc64.replies | 124 +--- .../caps_5.2.0.riscv64.replies| 120 --- .../caps_5.2.0.s390x.replies | 124 +--- .../caps_5.2.0.x86_64.replies | 136 + .../caps_6.0.0.x86_64.replies | 140 ++ .../caps_6.0.0.x86_64.xml | 1 + ...vhost-user-fs-hugepages.x86_64-latest.args | 3 +- .../vhost-user-fs-hugepages.xml | 3 +- 24 files changed, 1534 insertions(+), 329 deletions(-) Hopefully, you'll squash 4/7 and 5/7 into 3/7 so that we're able to compile && test after each commit. Also, news entry would be nice (it's fine a follow up patch). Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 0/7] Add boot order to virtiofs
On 01/28/21 16:15, Ján Tomko wrote:
> Sadly, the replies changes for older QEMUs are synthetic.
> Separated for easier review.
>
> Also available on gitlab:
> git fetch https://gitlab.com/janotomko/libvirt/ virtiofs-bootindex
> https://gitlab.com/janotomko/libvirt/-/tree/virtiofs-bootindex
>
> And a broken pipeline:
> https://gitlab.com/janotomko/libvirt/-/pipelines/248162273
>
> Ján Tomko (7):
> tests: switch vhost-user-fs-hugepages to use boot order
> conf: add boot order to filesystem
> qemu: add QEMU_CAPS_VHOST_USER_FS_BOOTINDEX
> fixup: vhost-user-fs-device properties
> fixup: renumber
> Add validation for virtiofs boot order setting
> qemu: format bootindex for vhost-user-fs
>
> docs/schemas/domaincommon.rng | 3 +
> src/conf/domain_conf.c| 5 +-
> src/conf/domain_validate.c| 17 ++-
> src/qemu/qemu_capabilities.c | 8 +
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 3 +
> src/qemu/qemu_validate.c | 6 +
> .../caps_4.2.0.aarch64.replies| 131
> .../caps_4.2.0.s390x.replies | 119 ---
> .../caps_4.2.0.x86_64.replies | 131
> .../caps_5.0.0.aarch64.replies| 136 +
> .../caps_5.0.0.ppc64.replies | 124 +---
> .../caps_5.0.0.riscv64.replies| 120 ---
> .../caps_5.0.0.x86_64.replies | 136 +
> .../caps_5.1.0.x86_64.replies | 136 +
> .../caps_5.2.0.aarch64.replies| 136 +
> .../caps_5.2.0.ppc64.replies | 124 +---
> .../caps_5.2.0.riscv64.replies| 120 ---
> .../caps_5.2.0.s390x.replies | 124 +---
> .../caps_5.2.0.x86_64.replies | 136 +
> .../caps_6.0.0.x86_64.replies | 140 ++
> .../caps_6.0.0.x86_64.xml | 1 +
> ...vhost-user-fs-hugepages.x86_64-latest.args | 3 +-
> .../vhost-user-fs-hugepages.xml | 3 +-
> 24 files changed, 1534 insertions(+), 329 deletions(-)
>
I've applied this series locally on top of e59bb226b7d9 ("docs: link to
PCI docs from the kbase page", 2021-01-28), and tested it as follows:
- Added to the virtio-fs element I already had; virsh
edit completed fine
- Booted the OVMF guest once with N=1 and then separately with N=3,
while the SCSI system disk of the guest had in both
cases. Checked the firmware log to verify the behavior -- it was OK in
both cases.
So please add the following to whichever patch it applies to:
Tested-by: Laszlo Ersek
(I didn't explicitly run any test suite, nor did I attempt to verify
behavior with an older QEMU, so I figure my T-b does not apply to every
patch in the series.)
Thank you Jano for implementing this feature.
--*--
Some general notes/questions on testing:
I used the documentation at .
- I think the example pathname "/home/to/your/checkout/src/libvirtd"
should include the "build" directory name now, after the meson conversion.
- I had to stop / start "virtlogd" separately, too; noting that in the
docs could help.
- I had to set SELinux to Permissive temporarily, otherwise QEMU
wouldn't start. A note on SELinux could be helpful.
- I manually "power cycled" the virtual networks on my laptop as well,
because the dnsmasq command lines refer to the "lease helper" binaries,
and the latter are also specific to the libvirtd build. I'm not sure
this was really necessary, but better safe than sorry?...
- After completing the tests, I shut down the one-off virtlogd and
libvirtd processes (Ctrl-C), and started the system-wide binaries again,
with systemctl. Systemctl reports both of those as "running OK" now;
however, when I try to net-start the virtual networks again, with
"virsh", I get the following error:
error: failed to connect to the hypervisor
error: Failed to connect socket to '/var/run/libvirt/libvirt-sock': No
such file or directory
I don't know what that's about. I guess I'll just reboot my laptop now :)
Thanks!
Laszlo
[libvirt PATCH 2/7] conf: add boot order to filesystem
Signed-off-by: Ján Tomko --- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 5 +++-- tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml | 1 + 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a4bddcf132..09e7747787 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2812,6 +2812,9 @@ + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dab4f10326..b8cfadb70c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10345,7 +10345,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, def->src->path = g_steal_pointer(); def->dst = g_steal_pointer(); -if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0) +if (virDomainDeviceInfoParseXML(xmlopt, node, >info, +flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) goto error; return def; @@ -24896,7 +24897,7 @@ virDomainFSDefFormat(virBufferPtr buf, if (def->readonly) virBufferAddLit(buf, "\n"); -virDomainDeviceInfoFormat(buf, >info, flags); +virDomainDeviceInfoFormat(buf, >info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); if (def->space_hard_limit) virBufferAsprintf(buf, "" diff --git a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml index e9982150c7..3f130dd152 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml +++ b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml @@ -66,6 +66,7 @@ + -- 2.29.2
[libvirt PATCH 6/7] Add validation for virtiofs boot order setting
Signed-off-by: Ján Tomko
---
src/conf/domain_validate.c | 17 -
src/qemu/qemu_validate.c | 6 ++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 649fc335ac..404eee09a9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -1528,6 +1528,19 @@ virDomainShmemDefValidate(const virDomainShmemDef *shmem)
return 0;
}
+static int
+virDomainFSDefValidate(const virDomainFSDef *fs)
+{
+if (fs->info.bootIndex &&
+fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("boot order is only supported for virtiofs"));
+return -1;
+}
+
+return 0;
+}
+
static int
virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
@@ -1573,10 +1586,12 @@ virDomainDeviceDefValidateInternal(const
virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_SHMEM:
return virDomainShmemDefValidate(dev->data.shmem);
+case VIR_DOMAIN_DEVICE_FS:
+return virDomainFSDefValidate(dev->data.fs);
+
case VIR_DOMAIN_DEVICE_AUDIO:
/* TODO: validate? */
case VIR_DOMAIN_DEVICE_LEASE:
-case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_SOUND:
case VIR_DOMAIN_DEVICE_WATCHDOG:
case VIR_DOMAIN_DEVICE_GRAPHICS:
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index a060bd98ba..92319ab3a5 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4093,6 +4093,12 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
}
if (qemuValidateDomainDefVirtioFSSharedMemory(def, qemuCaps) < 0)
return -1;
+if (fs->info.bootIndex &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOST_USER_FS_BOOTINDEX)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("setting virtiofs boot order is not supported
with this QEMU binary"));
+return -1;
+}
break;
case VIR_DOMAIN_FS_DRIVER_TYPE_LAST:
--
2.29.2
[libvirt PATCH 4/7] fixup: vhost-user-fs-device properties
---
.../caps_4.2.0.aarch64.replies| 79 +
.../caps_4.2.0.s390x.replies | 79 +
.../caps_4.2.0.x86_64.replies | 79 +
.../caps_5.0.0.aarch64.replies| 84 ++
.../caps_5.0.0.ppc64.replies | 84 ++
.../caps_5.0.0.riscv64.replies| 84 ++
.../caps_5.0.0.x86_64.replies | 84 ++
.../caps_5.1.0.x86_64.replies | 84 ++
.../caps_5.2.0.aarch64.replies| 84 ++
.../caps_5.2.0.ppc64.replies | 84 ++
.../caps_5.2.0.riscv64.replies| 84 ++
.../caps_5.2.0.s390x.replies | 84 ++
.../caps_5.2.0.x86_64.replies | 84 ++
.../caps_6.0.0.x86_64.replies | 88 +++
14 files changed, 1165 insertions(+)
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
index 68f67d6dd8..29981e96a0 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
@@ -6302,6 +6302,85 @@
"id": "libvirt-31"
}
+{
+ "execute": "device-list-properties",
+ "arguments": {
+"typename": "vhost-user-fs-device"
+ },
+ "id": "libvirt-33"
+}
+
+{
+ "return": [
+{
+ "default-value": false,
+ "name": "packed",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": false,
+ "name": "iommu_platform",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "event_idx",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": false,
+ "name": "x-disable-legacy-check",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "notify_on_empty",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "any_layout",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "use-started",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "indirect_desc",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "name": "chardev",
+ "description": "ID of a chardev to use as a backend",
+ "type": "str"
+},
+{
+ "default-value": 128,
+ "name": "queue-size",
+ "type": "uint16"
+},
+{
+ "name": "tag",
+ "type": "str"
+},
+{
+ "default-value": 1,
+ "name": "num-request-queues",
+ "type": "uint16"
+}
+ ],
+ "id": "libvirt-33"
+}
+
{
"execute": "qom-list-properties",
"arguments": {
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
index f87a7aa812..000426f4c2 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
@@ -4421,6 +4421,85 @@
"id": "libvirt-30"
}
+{
+ "execute": "device-list-properties",
+ "arguments": {
+"typename": "vhost-user-fs-device"
+ },
+ "id": "libvirt-33"
+}
+
+{
+ "return": [
+{
+ "default-value": false,
+ "name": "packed",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": false,
+ "name": "iommu_platform",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "event_idx",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": false,
+ "name": "x-disable-legacy-check",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "notify_on_empty",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "any_layout",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "use-started",
+ "type": "bool"
+},
+{
+ "default-value": true,
+ "name": "indirect_desc",
+ "description": "on/off",
+ "type": "bool"
+},
+{
+ "name": "chardev",
+ "description": "ID of a chardev to use as a backend",
+ "type": "str"
+},
+{
+ "default-value": 128,
+ "name": "queue-size",
+ "type": "uint16"
+},
+{
+ "name": "tag",
+ "type": "str"
+},
+{
+ "default-value": 1,
+ "name": "num-request-queues",
+ "type": "uint16"
+}
+ ],
+ "id": "libvirt-33"
+}
+
{
"execute": "qom-list-properties",
"arguments": {
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies
[libvirt PATCH 1/7] tests: switch vhost-user-fs-hugepages to use boot order
Signed-off-by: Ján Tomko --- tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml index 96b9774704..e9982150c7 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml +++ b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml @@ -12,7 +12,6 @@ 2 hvm - @@ -34,6 +33,7 @@ + -- 2.29.2
[libvirt PATCH 0/7] Add boot order to virtiofs
Sadly, the replies changes for older QEMUs are synthetic. Separated for easier review. Also available on gitlab: git fetch https://gitlab.com/janotomko/libvirt/ virtiofs-bootindex https://gitlab.com/janotomko/libvirt/-/tree/virtiofs-bootindex And a broken pipeline: https://gitlab.com/janotomko/libvirt/-/pipelines/248162273 Ján Tomko (7): tests: switch vhost-user-fs-hugepages to use boot order conf: add boot order to filesystem qemu: add QEMU_CAPS_VHOST_USER_FS_BOOTINDEX fixup: vhost-user-fs-device properties fixup: renumber Add validation for virtiofs boot order setting qemu: format bootindex for vhost-user-fs docs/schemas/domaincommon.rng | 3 + src/conf/domain_conf.c| 5 +- src/conf/domain_validate.c| 17 ++- src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 3 + src/qemu/qemu_validate.c | 6 + .../caps_4.2.0.aarch64.replies| 131 .../caps_4.2.0.s390x.replies | 119 --- .../caps_4.2.0.x86_64.replies | 131 .../caps_5.0.0.aarch64.replies| 136 + .../caps_5.0.0.ppc64.replies | 124 +--- .../caps_5.0.0.riscv64.replies| 120 --- .../caps_5.0.0.x86_64.replies | 136 + .../caps_5.1.0.x86_64.replies | 136 + .../caps_5.2.0.aarch64.replies| 136 + .../caps_5.2.0.ppc64.replies | 124 +--- .../caps_5.2.0.riscv64.replies| 120 --- .../caps_5.2.0.s390x.replies | 124 +--- .../caps_5.2.0.x86_64.replies | 136 + .../caps_6.0.0.x86_64.replies | 140 ++ .../caps_6.0.0.x86_64.xml | 1 + ...vhost-user-fs-hugepages.x86_64-latest.args | 3 +- .../vhost-user-fs-hugepages.xml | 3 +- 24 files changed, 1534 insertions(+), 329 deletions(-) -- 2.29.2
[libvirt PATCH 7/7] qemu: format bootindex for vhost-user-fs
Wire up the QEMU command line for this option. Signed-off-by: Ján Tomko --- src/qemu/qemu_command.c| 3 +++ .../vhost-user-fs-hugepages.x86_64-latest.args | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ec302d4ac..ac19902025 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2194,6 +2194,9 @@ qemuBuildVHostUserFsCommandLine(virCommandPtr cmd, virQEMUBuildBufferEscapeComma(, fs->dst); qemuBuildVirtioOptionsStr(, fs->virtio); +if (fs->info.bootIndex) +virBufferAsprintf(, ",bootindex=%u", fs->info.bootIndex); + if (qemuBuildDeviceAddressStr(, def, >info, priv->qemuCaps) < 0) return -1; diff --git a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.x86_64-latest.args b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.x86_64-latest.args index e4f5db7a63..24cafed400 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.x86_64-latest.args +++ b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.x86_64-latest.args @@ -42,7 +42,8 @@ addr=0x1 \ -device virtio-blk-pci,bus=pci.4,addr=0x0,drive=libvirt-1-format,\ id=virtio-disk0,bootindex=1 \ -chardev socket,id=chr-vu-fs0,path=/tmp/lib/domain--1-guest/fs0.vhost-fs.sock \ --device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=mount_tag,bus=pci.1,addr=0x0 \ +-device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=mount_tag,bootindex=2,\ +bus=pci.1,addr=0x0 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on -- 2.29.2
[libvirt PATCH 5/7] fixup: renumber
---
.../caps_4.2.0.aarch64.replies| 56 +--
.../caps_4.2.0.s390x.replies | 44 +++
.../caps_4.2.0.x86_64.replies | 56 +--
.../caps_5.0.0.aarch64.replies| 56 +--
.../caps_5.0.0.ppc64.replies | 44 +++
.../caps_5.0.0.riscv64.replies| 36 ++--
.../caps_5.0.0.x86_64.replies | 56 +--
.../caps_5.1.0.x86_64.replies | 56 +--
.../caps_5.2.0.aarch64.replies| 52 -
.../caps_5.2.0.ppc64.replies | 44 +++
.../caps_5.2.0.riscv64.replies| 36 ++--
.../caps_5.2.0.s390x.replies | 44 +++
.../caps_5.2.0.x86_64.replies | 56 +--
.../caps_6.0.0.x86_64.replies | 56 +--
14 files changed, 346 insertions(+), 346 deletions(-)
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
index 29981e96a0..e488270c0c 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
@@ -6307,7 +6307,7 @@
"arguments": {
"typename": "vhost-user-fs-device"
},
- "id": "libvirt-33"
+ "id": "libvirt-32"
}
{
@@ -6378,7 +6378,7 @@
"type": "uint16"
}
],
- "id": "libvirt-33"
+ "id": "libvirt-32"
}
{
@@ -6386,7 +6386,7 @@
"arguments": {
"typename": "memory-backend-file"
},
- "id": "libvirt-32"
+ "id": "libvirt-33"
}
{
@@ -6451,7 +6451,7 @@
"type": "bool"
}
],
- "id": "libvirt-32"
+ "id": "libvirt-33"
}
{
@@ -6459,7 +6459,7 @@
"arguments": {
"typename": "memory-backend-memfd"
},
- "id": "libvirt-33"
+ "id": "libvirt-34"
}
{
@@ -6523,7 +6523,7 @@
"type": "bool"
}
],
- "id": "libvirt-33"
+ "id": "libvirt-34"
}
{
@@ -6531,7 +6531,7 @@
"arguments": {
"typename": "max-arm-cpu"
},
- "id": "libvirt-34"
+ "id": "libvirt-35"
}
{
@@ -6718,12 +6718,12 @@
"type": "bool"
}
],
- "id": "libvirt-34"
+ "id": "libvirt-35"
}
{
"execute": "query-machines",
- "id": "libvirt-35"
+ "id": "libvirt-36"
}
{
@@ -7281,7 +7281,7 @@
"deprecated": false
}
],
- "id": "libvirt-35"
+ "id": "libvirt-36"
}
{
@@ -7289,7 +7289,7 @@
"arguments": {
"typename": "virt-4.2-machine"
},
- "id": "libvirt-36"
+ "id": "libvirt-37"
}
{
@@ -7442,12 +7442,12 @@
"type": "child"
}
],
- "id": "libvirt-36"
+ "id": "libvirt-37"
}
{
"execute": "query-cpu-definitions",
- "id": "libvirt-37"
+ "id": "libvirt-38"
}
{
@@ -7638,34 +7638,34 @@
"static": false
}
],
- "id": "libvirt-37"
+ "id": "libvirt-38"
}
{
"execute": "query-tpm-models",
- "id": "libvirt-38"
+ "id": "libvirt-39"
}
{
"return": [
],
- "id": "libvirt-38"
+ "id": "libvirt-39"
}
{
"execute": "query-tpm-types",
- "id": "libvirt-39"
+ "id": "libvirt-40"
}
{
"return": [
],
- "id": "libvirt-39"
+ "id": "libvirt-40"
}
{
"execute": "query-command-line-options",
- "id": "libvirt-40"
+ "id": "libvirt-41"
}
{
@@ -8833,12 +8833,12 @@
"option": "drive"
}
],
- "id": "libvirt-40"
+ "id": "libvirt-41"
}
{
"execute": "query-migrate-capabilities",
- "id": "libvirt-41"
+ "id": "libvirt-42"
}
{
@@ -8916,12 +8916,12 @@
"capability": "validate-uuid"
}
],
- "id": "libvirt-41"
+ "id": "libvirt-42"
}
{
"execute": "query-qmp-schema",
- "id": "libvirt-42"
+ "id": "libvirt-43"
}
{
@@ -21492,12 +21492,12 @@
]
}
],
- "id": "libvirt-42"
+ "id": "libvirt-43"
}
{
"execute": "query-gic-capabilities",
- "id": "libvirt-43"
+ "id": "libvirt-44"
}
{
@@ -21513,7 +21513,7 @@
"kernel": false
}
],
- "id": "libvirt-43"
+ "id": "libvirt-44"
}
{
@@ -21524,7 +21524,7 @@
"name": "host"
}
},
- "id": "libvirt-44"
+ "id": "libvirt-45"
}
{
@@ -21554,7 +21554,7 @@
}
}
},
- "id": "libvirt-44"
+ "id": "libvirt-45"
}
{
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
index 000426f4c2..94d2659f9d 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.replies
@@ -4426,7 +4426,7 @@
"arguments": {
"typename": "vhost-user-fs-device"
},
- "id": "libvirt-33"
+ "id": "libvirt-31"
}
{
@@ -4497,7 +4497,7 @@
"type": "uint16"
}
],
- "id": "libvirt-33"
+ "id": "libvirt-31"
}
{
@@ -4505,7 +4505,7 @@
"arguments": {
"typename": "memory-backend-file"
},
- "id": "libvirt-31"
+ "id": "libvirt-32"
}
{
@@ -4570,7 +4570,7 @@
"type": "bool"
[libvirt PATCH 3/7] qemu: add QEMU_CAPS_VHOST_USER_FS_BOOTINDEX
Introduced by QEMU commit:
commit 6da32fe5efdd71c9d254a436ce972194ff631285
Author: Laszlo Ersek
AuthorDate: 2021-01-12 14:16:03 +0100
Commit: Michael S. Tsirkin
CommitDate: 2021-01-13 09:06:37 -0500
vhost-user-fs: add the "bootindex" property
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 8
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
3 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d656732c3e..4a1bf9b6fe 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -610,6 +610,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"dc390",
"am53c974",
"virtio-pmem-pci",
+ "vhost-user-fs.bootindex",
);
@@ -1510,6 +1511,10 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsNVDIMM[] = {
{ "unarmed", QEMU_CAPS_DEVICE_NVDIMM_UNARMED, NULL },
};
+static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVhostUserFS[]
= {
+{ "bootindex", QEMU_CAPS_VHOST_USER_FS_BOOTINDEX, NULL },
+};
+
/* see documentation for virQEMUQAPISchemaPathGet for the query format */
static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
{ "blockdev-add/arg-type/options/+gluster/debug-level",
QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
@@ -1661,6 +1666,9 @@ static virQEMUCapsDeviceTypeProps
virQEMUCapsDeviceProps[] = {
{ "usb-host", virQEMUCapsDevicePropsUSBHost,
G_N_ELEMENTS(virQEMUCapsDevicePropsUSBHost),
-1 },
+{ "vhost-user-fs-device", virQEMUCapsDevicePropsVhostUserFS,
+ G_N_ELEMENTS(virQEMUCapsDevicePropsVhostUserFS),
+ QEMU_CAPS_DEVICE_VHOST_USER_FS },
};
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[]
= {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index a14a78f959..cd95103652 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -590,6 +590,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_SCSI_DC390, /* -device dc-390 */
QEMU_CAPS_SCSI_AM53C974, /* -device am53c974 */
QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI, /* -device virtio-pmem-pci */
+QEMU_CAPS_VHOST_USER_FS_BOOTINDEX, /* vhost-user-fs.bootindex */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index bd5021860e..f988357c44 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -251,6 +251,7 @@
+
5002050
0
43100242
--
2.29.2
Re: [libvirt PATCH 10/11] vircommand: Remove NULL check in virCommandAddArg
On Thu, 2021-01-28 at 11:54 +0100, Peter Krempa wrote:
> On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote:
> > `val` is declared `ATTRIBUTE_NONNULL`.
>
> Please see:
>
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127
>
> ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done
> by
> gcc.
>
> > Found by clang-tidy's "clang-diagnostic-tautological-pointer-
> > compare"
> > check.
> >
> > Signed-off-by: Tim Wiederhake
> > ---
> > src/util/vircommand.c | 5 -
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> > index c3a98bbeac..223010c6aa 100644
> > --- a/src/util/vircommand.c
> > +++ b/src/util/vircommand.c
> > @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const
> > char *val)
> > if (!cmd || cmd->has_error)
> > return;
> >
> > -if (val == NULL) {
> > -cmd->has_error = EINVAL;
> > -return;
> > -}
>
> So this might have actual value.
>
Hm, I see. I will remove this patch from the series for now. A bit
frustrating though, if you compare with, as a random example,
"virCommandGetGID", which is declared in [1] as "ATTRIBUTE_NONNULL" for
argument "cmd", which is subsequently dereferenced without additional
NULL check in [2].
Cheers,
Tim
[1]
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/vircommand.h#L71
[2]
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/vircommand.c#L1116
Re: [libvirt PATCH 00/11] Random bits found by clang-tidy
On Thu, 2021-01-28 at 10:35 +, Daniel P. Berrangé wrote:
> On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
> > clang-tidy is a static code analysis tool under the llvm umbrella.
> > It is
> > primarily meant to be used on C++ code bases, but some of the
> > checks it
> > provides also apply to C.
> >
> > The findings vary in severity and contain pseudo-false-positives,
> > i.e.
> > clang-tidy is flagging potential execution flows that could happen
> > in
> > theory but are virtually impossible in real life: In function
> > `virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be
> > read
> > unintialized if `stat()` failed and set `errno` to a negative
> > value, to name
> > just one example.
> >
> > The main source of false positive findings is the lack of support
> > for
> > `__attribute__((cleanup))` in clang-tidy, which is heavily used in
> > libvirt
> > through glib's `g_autofree` and `g_auto()` macros:
> >
> > #include
> >
> > void freeptr(int** p) {
> > if (*p)
> > free(*p);
> > }
> >
> > int main() {
> > __attribute__((cleanup(freeptr))) int *ptr = NULL;
> > ptr = calloc(sizeof(int), 1);
> > return 0; /* flagged as memory leak of `ptr` */
> > }
> >
> > This sadly renders clang-tidy's analysis of dynamic memory useless,
> > hiding all
> > real issues that it could otherwise find.
> >
> > Meson provides excellent integration for clang-tidy (a "clang-tidy"
> > target is
> > automatically generated if a ".clang-tidy" configuration file is
> > present
> > in the project's root directory). The amount of false-positives and
> > the slow
> > analysis, triggering time-outs in the CI, make this tool unfit for
> > inclusion
> > in libvirt's GitLab CI though.
>
> Is it possible to make it viable for CI by disabling *all* checks by
> default and then selectively re-enabling just the handful that are
> useful and don't have false positives ?
>
> Regards,
> Daniel
That is what I tried at first. Unfortunately, it does not work for
several reasons:
* clang-tidy does not like memory constraint environments -- at least
that appears to be the bottom of it. The result is random segfaults.
* There are false positive findings in the group of "clang-analyze-*"
and "clang-diagnostic-*", which cannot be disabled or rather, are
implicitly re-enabled no matter your configuration. E.g: Flagging
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virfdstream.c#L1240
as dead store, see above explation of "__attribute__((cleanup))" for
background.
* There are true positive findings in the same group of checks that I,
frankly, just disagree with. E.g: Flagging
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virobject.c#L228
as tautological comparison (in the first iteration of the `while`
loop), as `klass` is declared ATTRIBUTE_NONNULL. "Unrolling" the first
iteration does not make the code better in my eyes. "no-lint" markers
for clang-tidy do exist, but I would rather not litter the code with
them to make some software happy.
* clang-tidy requires all files to be present, or the run will
terminate with a non-zero return code and analysis errors. This
includes generated files. My Meson-fu is not particularly great, and I
have not found a "run generators only" target. Building libvirt
completely before running clang-tidy cuts badly in gitlab's one-hour
time budget, and running the monster below, for which I would like to
profoundly apologize, is also not a future-proof option:
[ -d "${bld_dir}" ] || CC=clang meson "${bld_dir}" "${src_dir}"
ninja -C "${bld_dir}" \
src/access/viraccessapicheck.c \
src/access/viraccessapicheck.h \
src/access/viraccessapichecklxc.c \
src/access/viraccessapichecklxc.h \
src/access/viraccessapicheckqemu.c \
src/access/viraccessapicheckqemu.h \
src/admin/admin_client.h \
src/admin/admin_protocol.c \
src/admin/admin_protocol.h \
src/admin/admin_server_dispatch_stubs.h \
src/esx/esx_vi.generated.c \
src/esx/esx_vi.generated.h \
src/esx/esx_vi_methods.generated.c \
src/esx/esx_vi_methods.generated.h \
src/esx/esx_vi_methods.generated.macro \
src/esx/esx_vi_types.generated.c \
src/esx/esx_vi_types.generated.h \
src/esx/esx_vi_types.generated.typedef \
src/esx/esx_vi_types.generated.typeenum \
src/esx/esx_vi_types.generated.typefromstring \
src/esx/esx_vi_types.generated.typetostring \
src/hyperv/hyperv_wmi_classes.generated.c \
src/hyperv/hyperv_wmi_classes.generated.h \
src/hyperv/hyperv_wmi_classes.generated.typedef \
src/libvirt_functions.stp \
src/libvirt_probes.h \
src/libvirt_probes.stp \
src/locking/lock_daemon_dispatch_stubs.h \
src/locking/lock_protocol.c \
src/locking/lock_protocol.h \
src/logging/log_daemon_dispatch_stubs.h \
src/logging/log_protocol.c \
src/logging/log_protocol.h \
src/lxc/lxc_controller_dispatch.h \
[PATCH 3/3] conf: Improve virDomainVirtioOptionsCheckABIStability()
The virDomainVirtioOptionsCheckABIStability() function is called
from various ABI stability check functions. Every caller checks
if both old and new definitions have virtio options set and only
after that they call the function. This is suboptimal because:
a) this check can be done in the function itself (making all
callers shorter),
b) is inherently wrong, because it doesn't catch case where one
definition has virtio options set and the other doesn't.
Do proper checks at the beginning of the function and simplify
its calls.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6e90b8e180..905f8f0691 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21533,6 +21533,15 @@ static bool
virDomainVirtioOptionsCheckABIStability(virDomainVirtioOptionsPtr src,
virDomainVirtioOptionsPtr dst)
{
+if (!src && !dst)
+return true;
+
+if (!src || !dst) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Target device virtio options don't match the
source"));
+return false;
+}
+
if (src->iommu != dst->iommu) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target device iommu option '%s' does not "
@@ -21617,8 +21626,7 @@ virDomainDiskDefCheckABIStability(virDomainDiskDefPtr
src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -21677,8 +21685,7 @@
virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src,
}
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -21711,8 +21718,7 @@ virDomainFsDefCheckABIStability(virDomainFSDefPtr src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -21760,8 +21766,7 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -21799,8 +21804,7 @@ virDomainInputDefCheckABIStability(virDomainInputDefPtr
src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -21899,8 +21903,7 @@ virDomainVideoDefCheckABIStability(virDomainVideoDefPtr
src,
}
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -22113,8 +22116,7 @@
virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
@@ -22136,8 +22138,7 @@ virDomainRNGDefCheckABIStability(virDomainRNGDefPtr src,
return false;
}
-if (src->virtio && dst->virtio &&
-!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
+if (!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
return false;
if (!virDomainDeviceInfoCheckABIStability(>info, >info))
--
2.26.2
[PATCH 2/3] conf: Drop empty virDomainNetDefPostParse()
The previous commit rendered this function empty and needless.
Remove it.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 12 +---
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69ebd5d66d..6e90b8e180 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5276,13 +5276,6 @@
virDomainControllerDefPostParse(virDomainControllerDefPtr cdev)
}
-static int
-virDomainNetDefPostParse(virDomainNetDefPtr net G_GNUC_UNUSED)
-{
-return 0;
-}
-
-
static void
virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
{
@@ -5367,10 +5360,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr
dev,
ret = virDomainControllerDefPostParse(dev->data.controller);
break;
-case VIR_DOMAIN_DEVICE_NET:
-ret = virDomainNetDefPostParse(dev->data.net);
-break;
-
case VIR_DOMAIN_DEVICE_VSOCK:
virDomainVsockDefPostParse(dev->data.vsock);
ret = 0;
@@ -5382,6 +5371,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr
dev,
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
+case VIR_DOMAIN_DEVICE_NET:
case VIR_DOMAIN_DEVICE_INPUT:
case VIR_DOMAIN_DEVICE_SOUND:
case VIR_DOMAIN_DEVICE_WATCHDOG:
--
2.26.2
[PATCH 1/3] conf: Move virDomainCheckVirtioOptions() into domain_validate.c
The aim of virDomainCheckVirtioOptions() function is to check
whether no virtio options are set, i.e. no @iommu no @ats and no
@packed attributes were present in given device's XML (yeah, the
function has very misleading name). Nevertheless, this kind of
check belongs to validation phase, but now is done in post parse
phase. Move the function and its calls to domain_validate.c so
that future code is not tempted to repeat this mistake.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 40 +--
src/conf/domain_validate.c | 55 +++---
2 files changed, 47 insertions(+), 48 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dab4f10326..69ebd5d66d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5132,34 +5132,6 @@ virDomainHostdevDefPostParse(virDomainHostdevDefPtr dev,
}
-static int
-virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
-{
-if (!virtio)
-return 0;
-
-if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("iommu driver option is only supported "
- "for virtio devices"));
-return -1;
-}
-if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("ats driver option is only supported "
- "for virtio devices"));
-return -1;
-}
-if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("packed driver option is only supported "
- "for virtio devices"));
-return -1;
-}
-return 0;
-}
-
-
static void
virDomainChrDefPostParse(virDomainChrDefPtr chr,
const virDomainDef *def)
@@ -5256,11 +5228,6 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
virDomainPostParseCheckISCSIPath(>src->path);
}
-if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
-virDomainCheckVirtioOptions(disk->virtio) < 0) {
-return -1;
-}
-
if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
@@ -5310,13 +5277,8 @@
virDomainControllerDefPostParse(virDomainControllerDefPtr cdev)
static int
-virDomainNetDefPostParse(virDomainNetDefPtr net)
+virDomainNetDefPostParse(virDomainNetDefPtr net G_GNUC_UNUSED)
{
-if (!virDomainNetIsVirtioModel(net) &&
-virDomainCheckVirtioOptions(net->virtio) < 0) {
-return -1;
-}
-
return 0;
}
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 649fc335ac..37c09ff648 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -226,6 +226,34 @@
virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels,
}
+static int
+virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr virtio)
+{
+if (!virtio)
+return 0;
+
+if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("iommu driver option is only supported "
+ "for virtio devices"));
+return -1;
+}
+if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("ats driver option is only supported "
+ "for virtio devices"));
+return -1;
+}
+if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("packed driver option is only supported "
+ "for virtio devices"));
+return -1;
+}
+return 0;
+}
+
+
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -277,15 +305,19 @@ virDomainDiskDefValidate(const virDomainDef *def,
return -1;
}
-if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
-(disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO ||
- disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL ||
- disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk model '%s' not supported for bus '%s'"),
- virDomainDiskModelTypeToString(disk->model),
- virDomainDiskBusTypeToString(disk->bus));
-return -1;
+if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
+if (disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO ||
+disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL ||
+disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk model '%s'
[PATCH 0/3] conf: Couple of virtio options related improvements
I've noticed these while reviewing Boris' patch: https://www.redhat.com/archives/libvir-list/2021-January/msg01149.html Michal Prívozník (3): conf: Move virDomainCheckVirtioOptions() into domain_validate.c conf: Drop empty virDomainNetDefPostParse() conf: Improve virDomainVirtioOptionsCheckABIStability() src/conf/domain_conf.c | 83 +- src/conf/domain_validate.c | 55 - 2 files changed, 64 insertions(+), 74 deletions(-) -- 2.26.2
Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On Thu, Jan 28, 2021 at 02:03:25PM +0100, Michal Privoznik wrote:
> On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
> > On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
> > > On 1/28/21 11:44 AM, Peter Krempa wrote:
> > > > On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> > > > > If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
> > > > > would return "0", indicating success, without writing to "value".
> > > > >
> > > > > This was found by clang-tidy's
> > > > > "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> > > > > function "udevProcessCCW", flagging a read on the potentially
> > > > > uninitialized variable "online".
> > > > >
> > > > > Signed-off-by: Tim Wiederhake
> > > > > ---
> > > > >src/node_device/node_device_udev.c | 5 -
> > > > >1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/node_device/node_device_udev.c
> > > > > b/src/node_device/node_device_udev.c
> > > > > index 55a2731681..d5a12bab0e 100644
> > > > > --- a/src/node_device/node_device_udev.c
> > > > > +++ b/src/node_device/node_device_udev.c
> > > > > @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device
> > > > > *udev_device,
> > > > >str = udevGetDeviceSysfsAttr(udev_device, attr_name);
> > > > > -if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> > > > > +if (!str)
> > > > > +return -1;
> > > >
> > > > In this case an error wouldn't be reported any more.
> > >
> > > I think it's quite the opposite actually. Previously, if str == NULL then
> > > a
> > > zero was returned (without any error) from this function. Now you get -1.
> > >
> > > I think we want to keep return 0 in case of !str. Callers use the
> > > following
> > > pattern:
> > >
> > > var = -1; /* default */
> > > udevGetIntSysfsAttr(device, "attribute", , 10);
> > >
> > > If "attribute" exists, @var is updated; if it doesn't it's left untouched
> > > with the default value (-1 in this case).
> >
> > There should not be any code with this pattern because that leads us to
> > ignore genuine errors.
>
> I was a bit too harsh in my reply. Of course we check for
> udevGetIntSysfsAttr() retval. The pattern is like this:
>
> var = -1;
> if (udevGetIntSysfsAttr(device, "attribute", , 10) < 0)
> goto error;
>
> And I think this okay.
>
> >
> > If we want to degrade when an attribute isn't present, we must be explict
> > about that and test existance of the sysfs file
>
> Well, the above example would then look like this:
>
> var = -1;
> str = udev_device_get_sysattr_value(device, "attribute");
> if (str && virStrToLong_i(str, NULL, , 10) < 0) {
> /* error */
> }
>
> Which is exactly what udevGetIntSysfsAttr() does, except it's open coded.
Ok, so the real bug here is idevProcessCCW not initializing the
"online" variable before calling the method.
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 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
would return "0", indicating success, without writing to "value".
This was found by clang-tidy's
"clang-analyzer-core.UndefinedBinaryOperatorResult" check in
function "udevProcessCCW", flagging a read on the potentially
uninitialized variable "online".
Signed-off-by: Tim Wiederhake
---
src/node_device/node_device_udev.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 55a2731681..d5a12bab0e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
-if (str && virStrToLong_i(str, NULL, base, value) < 0) {
+if (!str)
+return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL then a
zero was returned (without any error) from this function. Now you get -1.
I think we want to keep return 0 in case of !str. Callers use the following
pattern:
var = -1; /* default */
udevGetIntSysfsAttr(device, "attribute", , 10);
If "attribute" exists, @var is updated; if it doesn't it's left untouched
with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to
ignore genuine errors.
I was a bit too harsh in my reply. Of course we check for
udevGetIntSysfsAttr() retval. The pattern is like this:
var = -1;
if (udevGetIntSysfsAttr(device, "attribute", , 10) < 0)
goto error;
And I think this okay.
If we want to degrade when an attribute isn't present, we must be explict
about that and test existance of the sysfs file
Well, the above example would then look like this:
var = -1;
str = udev_device_get_sysattr_value(device, "attribute");
if (str && virStrToLong_i(str, NULL, , 10) < 0) {
/* error */
}
Which is exactly what udevGetIntSysfsAttr() does, except it's open coded.
Michal
Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
> On 1/28/21 11:44 AM, Peter Krempa wrote:
> > On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> > > If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
> > > would return "0", indicating success, without writing to "value".
> > >
> > > This was found by clang-tidy's
> > > "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> > > function "udevProcessCCW", flagging a read on the potentially
> > > uninitialized variable "online".
> > >
> > > Signed-off-by: Tim Wiederhake
> > > ---
> > > src/node_device/node_device_udev.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/node_device/node_device_udev.c
> > > b/src/node_device/node_device_udev.c
> > > index 55a2731681..d5a12bab0e 100644
> > > --- a/src/node_device/node_device_udev.c
> > > +++ b/src/node_device/node_device_udev.c
> > > @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
> > > str = udevGetDeviceSysfsAttr(udev_device, attr_name);
> > > -if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> > > +if (!str)
> > > +return -1;
> >
> > In this case an error wouldn't be reported any more.
>
> I think it's quite the opposite actually. Previously, if str == NULL then a
> zero was returned (without any error) from this function. Now you get -1.
>
> I think we want to keep return 0 in case of !str. Callers use the following
> pattern:
>
> var = -1; /* default */
> udevGetIntSysfsAttr(device, "attribute", , 10);
>
> If "attribute" exists, @var is updated; if it doesn't it's left untouched
> with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to
ignore genuine errors.
If we want to degrade when an attribute isn't present, we must be explict
about that and test existance of the sysfs file
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qemu: Add virtio related options to vsock
On 1/27/21 7:46 PM, Boris Fiuczynski wrote:
Add virtio related options iommu, ats and packed as driver element attributes
to vsock devices. Ex:
Signed-off-by: Boris Fiuczynski
---
docs/formatdomain.rst | 2 +
docs/schemas/domaincommon.rng | 5 +++
src/conf/domain_conf.c| 34 +--
src/conf/domain_conf.h| 1 +
src/qemu/qemu_command.c | 3 ++
src/qemu/qemu_validate.c | 3 ++
.../vhost-vsock-ccw-iommu.s390x-latest.args | 42 +++
.../vhost-vsock-ccw-iommu.xml | 33 +++
tests/qemuxml2argvtest.c | 1 +
.../vhost-vsock-ccw-iommu.s390x-latest.xml| 37
tests/qemuxml2xmltest.c | 2 +
11 files changed, 160 insertions(+), 3 deletions(-)
create mode 100644
tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.s390x-latest.args
create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-iommu.xml
create mode 100644
tests/qemuxml2xmloutdata/vhost-vsock-ccw-iommu.s390x-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index c738078b90..a09868bed5 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -7433,6 +7433,8 @@ devices <#elementsVirtioTransitional>`__ for more
details. The optional
attribute ``address`` of the ``cid`` element specifies the CID assigned to the
guest. If the attribute ``auto`` is set to ``yes``, libvirt will assign a free
CID automatically on domain startup. :since:`Since 4.4.0`
+The optional ``driver`` element allows to specify virtio options, see
+`Virtio-specific options <#elementsVirtio>`__ for more details. :since:`Since
7.1.0`
::
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index a4bddcf132..232587e690 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4883,6 +4883,11 @@
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dab4f10326..b94204cb4f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2457,6 +2457,7 @@ virDomainVsockDefFree(virDomainVsockDefPtr vsock)
virObjectUnref(vsock->privateData);
virDomainDeviceInfoClear(>info);
+VIR_FREE(vsock->virtio);
VIR_FREE(vsock);
}
@@ -5321,7 +5322,16 @@ virDomainNetDefPostParse(virDomainNetDefPtr net)
}
-static void
+static bool
+virDomainVsockIsVirtioModel(const virDomainVsockDef *vsock)
+{
+return (vsock->model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO ||
+vsock->model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO_TRANSITIONAL ||
+vsock->model == VIR_DOMAIN_VSOCK_MODEL_VIRTIO_NON_TRANSITIONAL);
+}
+
+
+static int
virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
{
if (vsock->auto_cid == VIR_TRISTATE_BOOL_ABSENT) {
@@ -5330,6 +5340,12 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
else
vsock->auto_cid = VIR_TRISTATE_BOOL_YES;
}
+
+if (!virDomainVsockIsVirtioModel(vsock) &&
+virDomainCheckVirtioOptions(vsock->virtio) < 0)
+return -1;
This check should go into validator (virDomainVsockDefValidate()); The
idea is that in post parse callbacks we fill in missing info (e.g.
generate MAC for an ), and then in validate callbacks we
check whether parsed (and at that point autofilled) configuration makes
sense (e.g. if virtio options are set only if model is virtio).
But this is pre-existing and I'll post a patch that cleans that up.
+
+return 0;
}
@@ -5410,8 +5426,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_VSOCK:
-virDomainVsockDefPostParse(dev->data.vsock);
-ret = 0;
+ret = virDomainVsockDefPostParse(dev->data.vsock);
break;
case VIR_DOMAIN_DEVICE_MEMORY:
@@ -15711,6 +15726,11 @@ virDomainVsockDefParseXML(virDomainXMLOptionPtr xmlopt,
if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
return NULL;
+if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt),
+ >virtio) < 0)
+return NULL;
+
+
return g_steal_pointer();
}
@@ -22897,6 +22917,10 @@ virDomainVsockDefCheckABIStability(virDomainVsockDefPtr src,
return false;
}
+if (src->virtio && dst->virtio &&
+!virDomainVirtioOptionsCheckABIStability(src->virtio, dst->virtio))
Again, pre-existing, but what if only one from the pair of definitions
has ->virtio set? Another item on my cleanup list - move these checks
into virDomainVirtioOptionsCheckABIStability() so that it can be called
with either (or even both) args == NULL.
+return false;
+
if
Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On Thu, 2021-01-28 at 11:44 +0100, Peter Krempa wrote:
> On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> > If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
> > would return "0", indicating success, without writing to "value".
> >
> > This was found by clang-tidy's
> > "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> > function "udevProcessCCW", flagging a read on the potentially
> > uninitialized variable "online".
> >
> > Signed-off-by: Tim Wiederhake
> > ---
> > src/node_device/node_device_udev.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/node_device/node_device_udev.c
> > b/src/node_device/node_device_udev.c
> > index 55a2731681..d5a12bab0e 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device
> > *udev_device,
> >
> > str = udevGetDeviceSysfsAttr(udev_device, attr_name);
> >
> > -if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> > +if (!str)
> > +return -1;
>
> In this case an error wouldn't be reported any more.
>
> > +
> > +if (virStrToLong_i(str, NULL, base, value) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Failed to convert '%s' to int"), str);
>
> while here it was
I believe this is not correct. Here is how the code looked before:
(1) str = udevGetDeviceSysfsAttr(udev_device, attr_name);
(2) if (str && virStrToLong_i(str, NULL, base, value) < 0) {
(3) virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to convert '%s' to int"), str);
return -1;
}
return 0;
If "udevGetDeviceSysfsAttr" returns NULL in line (1), the (non-
existant) "false" branch of line (2) is taken, i.e. the virReportError
in line (3) is not executed.
In the new version of the code:
(4) str = udevGetDeviceSysfsAttr(udev_device, attr_name);
(5) if (!str)
return -1;
if (virStrToLong_i(str, NULL, base, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to convert '%s' to int"), str);
return -1;
}
return 0;
If "udevGetDeviceSysfsAttr" returns NULL in line (4), this is caught in
line (5), still not reporting an error message, but returning with an
appropriate return value. Reporting the error is performed higher up in
the call chain, e.g.:
* udevAddOneDevice ← reports the issue in "cleanup" block
* udevGetDeviceDetails
* udevProcessCCW
* udevGetIntSysfsAttr ← now returns -1 for missing attributes
Cheers,
Tim
Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
would return "0", indicating success, without writing to "value".
This was found by clang-tidy's
"clang-analyzer-core.UndefinedBinaryOperatorResult" check in
function "udevProcessCCW", flagging a read on the potentially
uninitialized variable "online".
Signed-off-by: Tim Wiederhake
---
src/node_device/node_device_udev.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 55a2731681..d5a12bab0e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
-if (str && virStrToLong_i(str, NULL, base, value) < 0) {
+if (!str)
+return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL
then a zero was returned (without any error) from this function. Now you
get -1.
I think we want to keep return 0 in case of !str. Callers use the
following pattern:
var = -1; /* default */
udevGetIntSysfsAttr(device, "attribute", , 10);
If "attribute" exists, @var is updated; if it doesn't it's left
untouched with the default value (-1 in this case).
Michal
Re: failed to start vm after add vsock device
On 1/28/21 12:08 PM, longguang.yue wrote: Michal, thanks. i have another question which is related to kata-container. when there is only one virtiofs-device , how does it do that in guest there are 4 times of virtiofs-mounts that have same src and different targets. # in guest [root@kvm kata-containers]# docker exec efda32ca6a93 mount | grep kataShared kataShared on / type virtiofs (rw,relatime) kataShared on /etc/resolv.conf type virtiofs (rw,relatime) kataShared on /etc/hostname type virtiofs (rw,relatime) kataShared on /etc/hosts type virtiofs (rw,relatime) I'm not sure how this is related to libvirt, but I'll try to answer anyway. I believe these mount points are set up by the initrd in the guest. And this confusion you are seeing is not related to virtiofs at all. It's non-intuitive way of how 'mount' shows bind mounts. One can bind mount a file. For instance: # touch /tmp/a /tmp/b # mount --bind /tmp/a /tmp/b # mount | grep /tmp/b tmpfs on /tmp/b type tmpfs (rw,nosuid,nodev,seclabel,nr_inodes=409600,inode64) As you can see, mount doesn't show /tmp/a as the source of the mount point but the FS associated. And I believe this is what's happening. kataShared is mount as root, but then those three files from /etc are bind mounted and thus mount shows kataShared as their source. I agree it's misleading (and I remember running into this same problem when developing private namespaces for QEMU VMs - but that's another story). Michal
Re: [libvirt PATCH 05/11] Replace bzero() with memset()
On Thu, Jan 28, 2021 at 10:59:41 +, Daniel Berrange wrote: > On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote: > > On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote: > > > This was found by clang-tidy's > > > "clang-analyzer-security.insecureAPI.bzero" check. > > > > Any reasoning behind why bzero is bad? > > Yeah, it is wierd to call this an insecure API. If anything memset is > more dangerous because people invert the 2nd and 3rd args, resulting > in not setting any bytes at all. According to the manpage it can allegedly be optimized out: The explicit_bzero() function performs the same task as bzero(). It differs from bzero() in that it guarantees that compiler optimizations will not remove the erase operation if the compiler deduces that the operation is "unnecessary". > > None the less bzero is deprecated, so it makes sense to use the > memset funtion in general. Yes it does, but the reason should be mentioned in the commit message.
Re: [libvirt PATCH 00/11] Random bits found by clang-tidy
On Thu, Jan 28, 2021 at 11:24:30 +0100, Tim Wiederhake wrote: [...] > Tim Wiederhake (11): > virfile: Remove redundant #ifndef > xen: Fix indentation in xenParseXLSpice > qemu_tpm: Fix indentation in qemuTPMEmulatorBuildCommand > virsh-domain: Fix error handling of pthread_sigmask > Replace bzero() with memset() > udevGetIntSysfsAttr: Return -1 for missing attributes > virhostuptime: Fix rounding in uptime calculation > tests: Prevent malloc with size 0 > vircryptotest: Directly assign string to avoid memcpy > vircommand: Remove NULL check in virCommandAddArg > vircommand: Simplify virCommandAddArg On patches with no explicit reply: Reviewed-by: Peter Krempa
Re: [libvirt PATCH 05/11] Replace bzero() with memset()
On Thu, Jan 28, 2021 at 12:03:36PM +0100, Peter Krempa wrote: > On Thu, Jan 28, 2021 at 10:59:41 +, Daniel Berrange wrote: > > On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote: > > > On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote: > > > > This was found by clang-tidy's > > > > "clang-analyzer-security.insecureAPI.bzero" check. > > > > > > Any reasoning behind why bzero is bad? > > > > Yeah, it is wierd to call this an insecure API. If anything memset is > > more dangerous because people invert the 2nd and 3rd args, resulting > > in not setting any bytes at all. > > According to the manpage it can allegedly be optimized out: > >The explicit_bzero() function performs the same task as bzero(). It >differs from bzero() in that it guarantees that compiler optimizations >will not remove the erase operation if the compiler deduces that the >operation is "unnecessary". A compiler smart enough eliminate bzero can do also likely eliminate memset. > > None the less bzero is deprecated, so it makes sense to use the > > memset funtion in general. > > Yes it does, but the reason should be mentioned in the commit message. 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:Re: failed to start vm after add vsock device
Michal, thanks. i have another question which is related to kata-container. when there is only one virtiofs-device , how does it do that in guest there are 4 times of virtiofs-mounts that have same src and different targets. # in guest [root@kvm kata-containers]# docker exec efda32ca6a93 mount | grep kataShared kataShared on / type virtiofs (rw,relatime) kataShared on /etc/resolv.conf type virtiofs (rw,relatime) kataShared on /etc/hostname type virtiofs (rw,relatime) kataShared on /etc/hosts type virtiofs (rw,relatime) # qemu-kvm -chardev socket,id=char-c91f3c6a619cec75,path=/run/vc/vm/efda32ca6a93491ac173dc2ad8a38ac095abab3bd8147a1101851f2a0a8d9012/vhost-fs.sock -device vhost-user-fs-pci,chardev=char-c91f3c6a619cec75,tag=kataShared,romfile= At 2021-01-27 21:31:49, "Michal Privoznik" wrote: >On 1/26/21 2:13 PM, longguang.yue wrote: >> Hi, all: >> >> there is no error when launch qemu-kvm from cli directly, but vm fails >> to start via libvirtd. >> i have tried to chmod 0666 /dev/vhost-vsock. >> >> >> error: internal error: qemu unexpectedly closed the monitor: >> 2021-01-26T13:06:06.403097Z qemu-kvm: -device >> vhost-vsock-pci,id=vhost-vsock-pci0,guest-cid=4: vhost-vsock: failed to open >> vhost device: Unknown error -13 > >Errno 13 is EACCES (Permission denied) which means that libvirt didn't >set seclabel on something ... > >> >> >> >> >> >> >> > >.. and this is explains why. Anything that's added via qemu commandline >passthru is opaque to libvirt. Libvirt does not examine it, nor it sets >any labels, nothing. If you use it, you're on your own. However, vsock >was added to libvirt (almost 3 years ago) and instead of passing through >a command line you can define vsock device: > >https://libvirt.org/formatdomain.html#vsock > >For instance like this: > > > > > >Michal
Re: [libvirt PATCH 05/11] Replace bzero() with memset()
On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote: > On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote: > > This was found by clang-tidy's > > "clang-analyzer-security.insecureAPI.bzero" check. > > Any reasoning behind why bzero is bad? Yeah, it is wierd to call this an insecure API. If anything memset is more dangerous because people invert the 2nd and 3rd args, resulting in not setting any bytes at all. None the less bzero is deprecated, so it makes sense to use the memset funtion in general. > > > > > > Signed-off-by: Tim Wiederhake > > --- > > src/util/virarptable.c | 2 +- > > tests/virpcimock.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > 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 10/11] vircommand: Remove NULL check in virCommandAddArg
On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote:
> `val` is declared `ATTRIBUTE_NONNULL`.
Please see:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127
ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done by
gcc.
> Found by clang-tidy's "clang-diagnostic-tautological-pointer-compare"
> check.
>
> Signed-off-by: Tim Wiederhake
> ---
> src/util/vircommand.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index c3a98bbeac..223010c6aa 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val)
> if (!cmd || cmd->has_error)
> return;
>
> -if (val == NULL) {
> -cmd->has_error = EINVAL;
> -return;
> -}
So this might have actual value.
Re: [libvirt PATCH 05/11] Replace bzero() with memset()
On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote: > This was found by clang-tidy's > "clang-analyzer-security.insecureAPI.bzero" check. Any reasoning behind why bzero is bad? > > Signed-off-by: Tim Wiederhake > --- > src/util/virarptable.c | 2 +- > tests/virpcimock.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-)
Re: [libvirt PATCH 08/11] tests: Prevent malloc with size 0
On Thu, Jan 28, 2021 at 11:24:38 +0100, Tim Wiederhake wrote:
> Found by clang-tidy's "clang-analyzer-optin.portability.UnixAPI" check.
>
> Signed-off-by: Tim Wiederhake
> ---
> tests/commandhelper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> index ba5681b715..9394a42726 100644
> --- a/tests/commandhelper.c
> +++ b/tests/commandhelper.c
> @@ -99,7 +99,7 @@ int main(int argc, char **argv) {
> origenv++;
> }
>
> -if (!(newenv = malloc(sizeof(*newenv) * n)))
> +if ((n == 0) || !(newenv = malloc(sizeof(*newenv) * n)))
> abort();
This doesn't seem to be an abort-worthy case. It's unlikely but the
environment can contain no variables. Also it makes stuff hard to debug.
If n==0 is indeed a problem an error should be reported.
>
> origenv = environ;
> --
> 2.26.2
>
Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
> would return "0", indicating success, without writing to "value".
>
> This was found by clang-tidy's
> "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> function "udevProcessCCW", flagging a read on the potentially
> uninitialized variable "online".
>
> Signed-off-by: Tim Wiederhake
> ---
> src/node_device/node_device_udev.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index 55a2731681..d5a12bab0e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
>
> str = udevGetDeviceSysfsAttr(udev_device, attr_name);
>
> -if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> +if (!str)
> +return -1;
In this case an error wouldn't be reported any more.
> +
> +if (virStrToLong_i(str, NULL, base, value) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to convert '%s' to int"), str);
while here it was
> return -1;
> --
> 2.26.2
>
Re: [libvirt PATCH 00/11] Random bits found by clang-tidy
On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
> clang-tidy is a static code analysis tool under the llvm umbrella. It is
> primarily meant to be used on C++ code bases, but some of the checks it
> provides also apply to C.
>
> The findings vary in severity and contain pseudo-false-positives, i.e.
> clang-tidy is flagging potential execution flows that could happen in
> theory but are virtually impossible in real life: In function
> `virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be read
> unintialized if `stat()` failed and set `errno` to a negative value, to name
> just one example.
>
> The main source of false positive findings is the lack of support for
> `__attribute__((cleanup))` in clang-tidy, which is heavily used in libvirt
> through glib's `g_autofree` and `g_auto()` macros:
>
> #include
>
> void freeptr(int** p) {
> if (*p)
> free(*p);
> }
>
> int main() {
> __attribute__((cleanup(freeptr))) int *ptr = NULL;
> ptr = calloc(sizeof(int), 1);
> return 0; /* flagged as memory leak of `ptr` */
> }
>
> This sadly renders clang-tidy's analysis of dynamic memory useless, hiding all
> real issues that it could otherwise find.
>
> Meson provides excellent integration for clang-tidy (a "clang-tidy" target is
> automatically generated if a ".clang-tidy" configuration file is present
> in the project's root directory). The amount of false-positives and the slow
> analysis, triggering time-outs in the CI, make this tool unfit for inclusion
> in libvirt's GitLab CI though.
Is it possible to make it viable for CI by disabling *all* checks by
default and then selectively re-enabling just the handful that are
useful and don't have false positives ?
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 :|
[libvirt PATCH 10/11] vircommand: Remove NULL check in virCommandAddArg
`val` is declared `ATTRIBUTE_NONNULL`.
Found by clang-tidy's "clang-diagnostic-tautological-pointer-compare"
check.
Signed-off-by: Tim Wiederhake
---
src/util/vircommand.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index c3a98bbeac..223010c6aa 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val)
if (!cmd || cmd->has_error)
return;
-if (val == NULL) {
-cmd->has_error = EINVAL;
-return;
-}
-
arg = g_strdup(val);
/* Arg plus trailing NULL. */
--
2.26.2
[libvirt PATCH 11/11] vircommand: Simplify virCommandAddArg
Signed-off-by: Tim Wiederhake
---
src/util/vircommand.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 223010c6aa..f83d49ffac 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1520,21 +1520,16 @@ virCommandAddEnvXDG(virCommandPtr cmd, const char
*baseDir)
void
virCommandAddArg(virCommandPtr cmd, const char *val)
{
-char *arg;
-
if (!cmd || cmd->has_error)
return;
-arg = g_strdup(val);
-
/* Arg plus trailing NULL. */
if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) {
-VIR_FREE(arg);
cmd->has_error = ENOMEM;
return;
}
-cmd->args[cmd->nargs++] = arg;
+cmd->args[cmd->nargs++] = g_strdup(val);
}
--
2.26.2
[libvirt PATCH 08/11] tests: Prevent malloc with size 0
Found by clang-tidy's "clang-analyzer-optin.portability.UnixAPI" check.
Signed-off-by: Tim Wiederhake
---
tests/commandhelper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index ba5681b715..9394a42726 100644
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -99,7 +99,7 @@ int main(int argc, char **argv) {
origenv++;
}
-if (!(newenv = malloc(sizeof(*newenv) * n)))
+if ((n == 0) || !(newenv = malloc(sizeof(*newenv) * n)))
abort();
origenv = environ;
--
2.26.2
[libvirt PATCH 05/11] Replace bzero() with memset()
This was found by clang-tidy's
"clang-analyzer-security.insecureAPI.bzero" check.
Signed-off-by: Tim Wiederhake
---
src/util/virarptable.c | 2 +-
tests/virpcimock.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index d62de5e3dd..dac3486470 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -120,7 +120,7 @@ virArpTableGet(void)
table->n = num + 1;
addr = RTA_DATA(tb[NDA_DST]);
-bzero(, sizeof(virAddr));
+memset(, 0, sizeof(virAddr));
virAddr.len = sizeof(virAddr.data.inet4);
virAddr.data.inet4.sin_family = AF_INET;
virAddr.data.inet4.sin_addr = *(struct in_addr *)addr;
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 4aa96cae08..f6280fc8b5 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -233,7 +233,7 @@ pci_read_file(const char *path,
if ((fd = real_open(newpath, O_RDWR)) < 0)
goto cleanup;
-bzero(buf, buf_size);
+memset(buf, 0, buf_size);
if (saferead(fd, buf, buf_size - 1) < 0) {
STDERR("Unable to read from %s", newpath);
goto cleanup;
--
2.26.2
[libvirt PATCH 03/11] qemu_tpm: Fix indentation in qemuTPMEmulatorBuildCommand
This was found by clang-tidy's "readability-misleading-indentation"
check.
Signed-off-by: Tim Wiederhake
---
src/qemu/qemu_tpm.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 532e0912bd..f94cad8230 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -600,9 +600,11 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
}
pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid,
cmd);
-if (pwdfile_fd)
-migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid,
- cmd);
+if (pwdfile_fd) {
+migpwdfile_fd =
qemuTPMSetupEncryption(tpm->data.emulator.secretuuid,
+ cmd);
+}
+
if (pwdfile_fd < 0 || migpwdfile_fd < 0)
goto error;
--
2.26.2
[libvirt PATCH 07/11] virhostuptime: Fix rounding in uptime calculation
"f + 0.5" does not round correctly for values very close to
".5" for every integer multiple, e.g. "0.49975".
Found by clang-tidy's "bugprone-incorrect-roundings" check.
Signed-off-by: Tim Wiederhake
---
src/util/virhostuptime.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c
index 696a38fbb5..7508a5a9b6 100644
--- a/src/util/virhostuptime.c
+++ b/src/util/virhostuptime.c
@@ -32,6 +32,8 @@
#include "virtime.h"
#include "virthread.h"
+#include
+
#define VIR_FROM_THIS VIR_FROM_NONE
VIR_LOG_INIT("util.virhostuptime");
@@ -76,7 +78,7 @@ virHostGetBootTimeProcfs(unsigned long long *btime)
return -EINVAL;
}
-*btime = now / 1000 - up + 0.5;
+*btime = llround(now / 1000 - up);
return 0;
}
--
2.26.2
[libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr"
would return "0", indicating success, without writing to "value".
This was found by clang-tidy's
"clang-analyzer-core.UndefinedBinaryOperatorResult" check in
function "udevProcessCCW", flagging a read on the potentially
uninitialized variable "online".
Signed-off-by: Tim Wiederhake
---
src/node_device/node_device_udev.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 55a2731681..d5a12bab0e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
-if (str && virStrToLong_i(str, NULL, base, value) < 0) {
+if (!str)
+return -1;
+
+if (virStrToLong_i(str, NULL, base, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to convert '%s' to int"), str);
return -1;
--
2.26.2
[libvirt PATCH 04/11] virsh-domain: Fix error handling of pthread_sigmask
pthread_sigmask() returns 0 on success and "a non-zero value on failure", but not neccessarily a negative one. Found by clang-tidy's "bugprone-posix-return" check. Signed-off-by: Tim Wiederhake --- tools/virsh-domain.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2bb136333f..298c3a6587 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4226,7 +4226,7 @@ doSave(void *opaque) sigemptyset(); sigaddset(, SIGINT); -if (pthread_sigmask(SIG_BLOCK, , ) < 0) +if (pthread_sigmask(SIG_BLOCK, , ) != 0) goto out_sig; #endif /* !WIN32 */ @@ -4756,7 +4756,7 @@ doManagedsave(void *opaque) sigemptyset(); sigaddset(, SIGINT); -if (pthread_sigmask(SIG_BLOCK, , ) < 0) +if (pthread_sigmask(SIG_BLOCK, , ) != 0) goto out_sig; #endif /* !WIN32 */ @@ -5438,7 +5438,7 @@ doDump(void *opaque) sigemptyset(); sigaddset(, SIGINT); -if (pthread_sigmask(SIG_BLOCK, , ) < 0) +if (pthread_sigmask(SIG_BLOCK, , ) != 0) goto out_sig; #endif /* !WIN32 */ @@ -10732,7 +10732,7 @@ doMigrate(void *opaque) sigemptyset(); sigaddset(, SIGINT); -if (pthread_sigmask(SIG_BLOCK, , ) < 0) +if (pthread_sigmask(SIG_BLOCK, , ) != 0) goto out_sig; #endif /* !WIN32 */ -- 2.26.2
[libvirt PATCH 09/11] vircryptotest: Directly assign string to avoid memcpy
Found by clang-tidy's "bugprone-not-null-terminated-result" check.
clang-tidy's finding is a false positive in this case, as the
memset call guarantees null termination. The assignment can be
simplified though, and this happens to silence the warning.
Signed-off-by: Tim Wiederhake
---
tests/vircryptotest.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c
index 90140077cf..2c3d4bfe75 100644
--- a/tests/vircryptotest.c
+++ b/tests/vircryptotest.c
@@ -122,7 +122,7 @@ static int
mymain(void)
{
int ret = 0;
-uint8_t secretdata[8];
+uint8_t secretdata[8] = "letmein";
uint8_t expected_ciphertext[16] = {0x48, 0x8e, 0x9, 0xb9,
0x6a, 0xa6, 0x24, 0x5f,
0x1b, 0x8c, 0x3f, 0x48,
@@ -166,9 +166,6 @@ mymain(void)
ret = -1; \
} while (0)
-memset(, 0, 8);
-memcpy(, "letmein", 7);
-
VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc",
secretdata, 7, expected_ciphertext, 16);
--
2.26.2
Re: [RFC] Change default ipv6 network from fec0/10 (site local) to fe80/10 (link local)
Hello, Philippe Mathieu-Daudé, le mer. 27 janv. 2021 22:46:13 +0100, a ecrit: > On 1/27/21 8:13 PM, Doug Evans wrote: > > I happened to notice QEMU's default for the ipv6 network is fec0::/10 > > which is deprecated (RFC3879). > > I think(!) an obvious replacement is fe80::/10, link local. fe80::/10 is really a different thing, I don't think we want to use it. We can use some prefix in fc00::/7, such as fd00::/8. It "just" needs checking with various guest OS, to check that it doesn't break the default behavior. Samuel
[libvirt PATCH 02/11] xen: Fix indentation in xenParseXLSpice
This was found by clang-tidy's "readability-misleading-indentation" check. Signed-off-by: Tim Wiederhake --- src/libxl/xen_xl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 621ee63a99..6dcba43fe0 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -359,9 +359,8 @@ xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) graphics->data.spice.port = (int)port; -if (!graphics->data.spice.tlsPort && -!graphics->data.spice.port) -graphics->data.spice.autoport = 1; +if (!graphics->data.spice.tlsPort && !graphics->data.spice.port) +graphics->data.spice.autoport = 1; if (xenConfigGetBool(conf, "spicedisable_ticketing", , 0) < 0) goto cleanup; -- 2.26.2
[libvirt PATCH 01/11] virfile: Remove redundant #ifndef
This section is guarded by "#ifndef WIN32" in line 2109--2808.
Found by clang-tidy's "readability-redundant-preprocessor" check.
Signed-off-by: Tim Wiederhake
---
src/util/virfile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 609cafdd34..5201b7fb07 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2632,7 +2632,7 @@ virDirCreateNoFork(const char *path,
virReportSystemError(errno, _("stat of '%s' failed"), path);
goto error;
}
-# ifndef WIN32
+
if (((uid != (uid_t) -1 && st.st_uid != uid) ||
(gid != (gid_t) -1 && st.st_gid != gid))
&& (chown(path, uid, gid) < 0)) {
@@ -2641,7 +2641,7 @@ virDirCreateNoFork(const char *path,
path, (unsigned int) uid, (unsigned int) gid);
goto error;
}
-# endif /* !WIN32 */
+
if (mode != (mode_t) -1 && chmod(path, mode) < 0) {
ret = -errno;
virReportSystemError(errno,
--
2.26.2
[libvirt PATCH 00/11] Random bits found by clang-tidy
clang-tidy is a static code analysis tool under the llvm umbrella. It is
primarily meant to be used on C++ code bases, but some of the checks it
provides also apply to C.
The findings vary in severity and contain pseudo-false-positives, i.e.
clang-tidy is flagging potential execution flows that could happen in
theory but are virtually impossible in real life: In function
`virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be read
unintialized if `stat()` failed and set `errno` to a negative value, to name
just one example.
The main source of false positive findings is the lack of support for
`__attribute__((cleanup))` in clang-tidy, which is heavily used in libvirt
through glib's `g_autofree` and `g_auto()` macros:
#include
void freeptr(int** p) {
if (*p)
free(*p);
}
int main() {
__attribute__((cleanup(freeptr))) int *ptr = NULL;
ptr = calloc(sizeof(int), 1);
return 0; /* flagged as memory leak of `ptr` */
}
This sadly renders clang-tidy's analysis of dynamic memory useless, hiding all
real issues that it could otherwise find.
Meson provides excellent integration for clang-tidy (a "clang-tidy" target is
automatically generated if a ".clang-tidy" configuration file is present
in the project's root directory). The amount of false-positives and the slow
analysis, triggering time-outs in the CI, make this tool unfit for inclusion
in libvirt's GitLab CI though.
The patches in this series are the result of fixing some of the issues
reported by running
CC=clang meson build
ninja -C build # generate sources and header files
ninja -C build clang-tidy
with the following `.clang-tidy` configuration file:
---
Checks: >
*,
-abseil-*,
-android-*,
-boost-*,
-cppcoreguidelines-*,
-fuchsia-*,
-google-*,
-hicpp-*,
-llvm-*,
-modernize-*,
-mpi-,
-objc-,
-openmp-,
-zircon-*,
-readability-braces-around-statements,
-readability-magic-numbers
WarningsAsErrors: '*'
HeaderFilterRegex: ''
FormatStyle: none
...
Regards,
Tim
Tim Wiederhake (11):
virfile: Remove redundant #ifndef
xen: Fix indentation in xenParseXLSpice
qemu_tpm: Fix indentation in qemuTPMEmulatorBuildCommand
virsh-domain: Fix error handling of pthread_sigmask
Replace bzero() with memset()
udevGetIntSysfsAttr: Return -1 for missing attributes
virhostuptime: Fix rounding in uptime calculation
tests: Prevent malloc with size 0
vircryptotest: Directly assign string to avoid memcpy
vircommand: Remove NULL check in virCommandAddArg
vircommand: Simplify virCommandAddArg
src/libxl/xen_xl.c | 5 ++---
src/node_device/node_device_udev.c | 5 -
src/qemu/qemu_tpm.c| 8 +---
src/util/virarptable.c | 2 +-
src/util/vircommand.c | 12 +---
src/util/virfile.c | 4 ++--
src/util/virhostuptime.c | 4 +++-
tests/commandhelper.c | 2 +-
tests/vircryptotest.c | 5 +
tests/virpcimock.c | 2 +-
tools/virsh-domain.c | 8
11 files changed, 25 insertions(+), 32 deletions(-)
--
2.26.2
