[libvirt PATCH 2/2] util: fix success return for virProcessKillPainfullyDelay()
virProcessKillPainfullyDelay() currently almost always returns 1 or -1,
even though the documentation indicates that it should return 0 if the
process was terminated gracefully. The only case that it currently
returns 0 is when it is called with the pid of a process that does not
exist.
This function initially sends a SIGTERM signal to the process. If
the signal was sent successfully (i.e. virProcessKill() returns 0) we
immediately sleep for 200ms before iterating the loop. On the second
iteration of the loop, signum will be set to 0 and we call
virProcessKill() again to check whether the process still exists. If the
process does not exist (virProcessKill() returns -1 and errno is ESRCH),
we consider the process killed successfully. But in order to determine
what value to return from the function, we compare the value of signum
to SIGTERM. But signum has already been set to 0; it will only ever be
SIGTERM in the very first loop iteration. So it always indicates a
forcible killing even when it is killed gracefully.
Signed-off-by: Jonathon Jongsma
---
src/util/virprocess.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 6ce5ef99a9..42b3b76eaa 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -377,6 +377,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
unsigned int extradelay, boo
/* This is in 1/5th seconds since polling is on a 0.2s interval */
unsigned int polldelay = (force ? 200 : 75) + (extradelay*5);
const char *signame = "TERM";
+int forced = 0;
VIR_DEBUG("vpid=%lld force=%d extradelay=%u group=%d",
(long long)pid, force, extradelay, group);
@@ -399,6 +400,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
unsigned int extradelay, boo
if (i == 0) {
signum = SIGTERM; /* kindly suggest it should exit */
} else if (i == 50 && force) {
+forced = 1;
VIR_DEBUG("Timed out waiting after SIGTERM to process %lld, "
"sending SIGKILL", (long long)pid);
/* No SIGKILL kill on Win32 ! Use SIGABRT instead which our
@@ -426,7 +428,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
unsigned int extradelay, boo
(long long)pid, signame);
return -1;
}
-return signum == SIGTERM ? 0 : 1;
+return forced;
}
g_usleep(200 * 1000);
--
2.41.0
[libvirt PATCH 1/2] util: Fix error return for virProcessKillPainfullyDelay()
Commit 93af79fb removed a cleanup label in favor of returning error
values directly in certain cases. But the final return value was changed
from -1 to 0. If we get to the end of the function, that means that
we've waited for the process to exit but it still exists. So we should
return -1. The error message was still being set correctly, but we were
returning a success status (0).
Signed-off-by: Jonathon Jongsma
---
src/util/virprocess.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 31b833e5c8..6ce5ef99a9 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -436,7 +436,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
unsigned int extradelay, boo
_("Failed to terminate process %1$lld with SIG%2$s"),
(long long)pid, signame);
-return 0;
+return -1;
}
--
2.41.0
[PATCH] libxl: Fix connection to modular network daemon
In a modular daemon configuration, virtxend does not support the
virNetwork* APIs. It should open a connection to virtnetworkd when
using those APIs, but currently always opens a connection to
"xen:///system". Switch to using virGetConnectNetwork to obtain a
valid connection instead of using the hardcoded URI.
Signed-off-by: Jim Fehlig
---
src/libxl/libxl_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4582126d19..62e1be6672 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1372,7 +1372,7 @@ libxlMakeNic(virDomainDef *def,
break;
case VIR_DOMAIN_NET_TYPE_NETWORK:
{
-if (!(conn = virConnectOpen("xen:///system")))
+if (!(conn = virGetConnectNetwork()))
goto cleanup;
if (!(network =
--
2.42.0
Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
On 9/21/23 1:10 PM, Peter Krempa wrote:
On Thu, Sep 21, 2023 at 12:50:52 -0500, Jonathon Jongsma wrote:
On 9/20/23 7:24 AM, Pavel Hrdina wrote:
On Thu, Aug 31, 2023 at 04:39:50PM -0500, Jonathon Jongsma wrote:
Add some helper functions to build a virCommand object and run the
nbdkit process for a given virStorageSource.
Signed-off-by: Jonathon Jongsma
Reviewed-by: Peter Krempa
---
src/qemu/qemu_nbdkit.c | 250 +
src/qemu/qemu_nbdkit.h | 10 ++
2 files changed, 260 insertions(+)
[...]
+VIR_DEBUG("Stopping nbdkit process %i", proc->pid);
+virProcessKill(proc->pid, SIGTERM);
Coverity complains here that the return value of virProcessKill() is not
checked which leads me to a question if we should use
virProcessKillPainfully() instead.
With the code that is pushed the function qemuNbdkitProcessStop() is
called only within the qemu_nbdkit.c for these cases:
- in qemuNbdkitProcessRestart() before starting the process again
where we do not check if the original process was killed correctly
or not,
- in qemuNbdkitStopStorageSource() where we check return value of
qemuNbdkitProcessStop() but it will always be 0,
- in qemuNbdkitProcessStart() as error path where we don't check any
return value.
To me it seems that the return value qemuNbdkitProcessStop can be changed
to void as we always return 0 and use virProcessKillPainfully() or
properly pass return value of virProcessKill() and check it for every
use of qemuNbdkitProcessStop().
Pavel
Good question. In one of my earlier series I had actually used
virProcessKillPainfully(), but changed it based on a suggestion from Peter
that it would be bad to kill it painfully if nbdkit was ever used in
read-write mode. But apparently I forgot to handle a shutdown failure. An
alternative would be to simply refuse to use nbdkit if the user requests
read-write mode.
We can use the same algorithm as with the qemu process where we first
issue SIGTERM, thus if the process is responsive it can execute the
shutdown actions. Otherwise it'll get SIGKILL right after.
That sounds almost identical to what virProcessKillPainfully() does.
Jonathon
[PATCH 3/3] virDomainMemoryDefValidate: Check for overlapping memory devices
As of v9.4.0-rc2~5 it is possible to specify guest address where
a virtio-mem/virtio-pmem memory device is mapped to. What that
commit forgot to introduce was a check for overlaps.
And yes, this is technically an O(n^2) algorithm, as
virDomainMemoryDefValidate() is called over each memory device
and after this, virDomainMemoryDefValidate() also iterates over
each memory device. But given there's usually only a handful of
such devices, and this runs only when parsing domain XML I guess
code readability wins over some less obvious solution.
Resolves: https://issues.redhat.com/browse/RHEL-4452
Signed-off-by: Michal Privoznik
---
src/conf/domain_validate.c| 47 +
...rtio-mem-overlap-address.x86_64-latest.err | 1 +
...ory-hotplug-virtio-mem-overlap-address.xml | 50 +++
tests/qemuxml2argvtest.c | 1 +
4 files changed, 99 insertions(+)
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index d14559cd73..6962fe76bf 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2223,6 +2223,9 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
{
const long pagesize = virGetSystemPageSize();
unsigned long long thpSize;
+unsigned long long thisStart = 0;
+unsigned long long thisEnd = 0;
+size_t i;
/* Guest NUMA nodes are continuous and indexed from zero. */
if (mem->targetNode != -1) {
@@ -2304,6 +2307,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
pagesize);
return -1;
}
+thisStart = mem->target.virtio_pmem.address;
break;
case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
@@ -2347,6 +2351,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
_("memory device address must be aligned to
blocksize"));
return -1;
}
+thisStart = mem->target.virtio_mem.address;
break;
case VIR_DOMAIN_MEMORY_MODEL_DIMM:
@@ -2368,6 +2373,48 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
return -1;
}
+if (thisStart == 0) {
+return 0;
+}
+
+/* thisStart and thisEnd are in bytes, mem->size in kibibytes */
+thisEnd = thisStart + mem->size * 1024;
+
+for (i = 0; i < def->nmems; i++) {
+const virDomainMemoryDef *other = def->mems[i];
+unsigned long long otherStart = 0;
+
+if (other == mem)
+continue;
+
+switch (other->model) {
+case VIR_DOMAIN_MEMORY_MODEL_NONE:
+case VIR_DOMAIN_MEMORY_MODEL_DIMM:
+case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
+case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
+case VIR_DOMAIN_MEMORY_MODEL_LAST:
+continue;
+break;
+
+case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
+otherStart = other->target.virtio_pmem.address;
+break;
+case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
+otherStart = other->target.virtio_mem.address;
+break;
+}
+
+if (otherStart == 0)
+continue;
+
+if (thisStart <= otherStart && thisEnd > otherStart) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("memory device address [0x%1$llx:0x%2$llx]
overlaps with other memory device (0x%3$llx)"),
+ thisStart, thisEnd, otherStart);
+return -1;
+}
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err
b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err
new file mode 100644
index 00..36d5b8a6e6
--- /dev/null
+++
b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: memory device address [0x14000:0x18000]
overlaps with other memory device (0x17000)
diff --git
a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml
b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml
new file mode 100644
index 00..65999ccd99
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml
@@ -0,0 +1,50 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 1099511627776
+ 8388608
+ 8388608
+ 2
+
+hvm
+
+
+
+qemu64
+
+
+
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+1048576
+0
+2048
+524288
+
+
+
+
+
+
+1-3
+2048
+
+
+2097152
+0
+2048
+1048576
+
+
+
+
+
+
[PATCH 1/3] domain_validate: Validate VIRTIO_PMEM address alignment
QEMU mandates the VIRTIO_PMEM address is aligned to a pagesize.
This is a very reasonable requirement. So much so, that it
deserves to be in hypervisor agnostic validation code
(virDomainMemoryDefValidate()). Not that any other hypervisor
would support VIRTIO_PMEM yet. But even if they did, this would
surely be still valid.
Signed-off-by: Michal Privoznik
---
src/conf/domain_validate.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index ac32fa1477..e423383e22 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2221,6 +2221,7 @@ static int
virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
const virDomainDef *def)
{
+const long pagesize = virGetSystemPageSize();
unsigned long long thpSize;
/* Guest NUMA nodes are continuous and indexed from zero. */
@@ -2295,6 +2296,14 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
_("virtio-pmem does not support NUMA nodes"));
return -1;
}
+
+if (pagesize > 0 &&
+mem->target.virtio_pmem.address % pagesize != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("memory address must be aligned to %1$ld bytes"),
+ pagesize);
+return -1;
+}
break;
case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
--
2.41.0
[PATCH 0/3] A couple of virtio-mem/virtio-pmem address improvements
*** BLURB HERE *** Michal Prívozník (3): domain_validate: Validate VIRTIO_PMEM address alignment virDomainMemoryDefValidate: Fix VIRTIO_MEM alignment check virDomainMemoryDefValidate: Check for overlapping memory devices src/conf/domain_validate.c| 59 ++- ...rtio-mem-overlap-address.x86_64-latest.err | 1 + ...ory-hotplug-virtio-mem-overlap-address.xml | 50 tests/qemuxml2argvtest.c | 1 + 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml -- 2.41.0
[PATCH 2/3] virDomainMemoryDefValidate: Fix VIRTIO_MEM alignment check
Inside of virDomainMemoryDefValidate() there's a check that
address where a virtio-mem memory device is mapped to is a
multiple of its block size. But this check is off by a couple of
bits, because the memory address is in bytes while the block size
is in kibibytes. Therefore, when checking whether address is a
multiple of the block size, the latter has to be multiplied by a
factor of 1024.
Signed-off-by: Michal Privoznik
---
src/conf/domain_validate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index e423383e22..d14559cd73 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2341,7 +2341,8 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
return -1;
}
-if (mem->target.virtio_mem.address % mem->target.virtio_mem.blocksize
!= 0) {
+/* blocksize is stored in KiB while address is in bytes */
+if (mem->target.virtio_mem.address % (mem->target.virtio_mem.blocksize
* 1024) != 0) {
virReportError(VIR_ERR_XML_DETAIL, "%s",
_("memory device address must be aligned to
blocksize"));
return -1;
--
2.41.0
Re: [libvirt PATCH] qemu: Improve error message for failed firmware autoselection
On 9/22/23 15:26, Andrea Bolognani wrote: > The current message can be misleading, because it seems to suggest > that no firmware of the requested type is available on the system. > > What actually happens most of the time, however, is that despite > having multiple firmwares of the right type to choose from, none > of them is suitable because of lacking some specific feature or > being incompatible with some setting that the user has explicitly > enabled. > > Providing an error message that describes exactly the problem is > not feasible, since we would have to list each candidate along > with the reason why we rejected it, which would get out of hand > quickly. > > As a small but hopefully helpful improvement over the current > situation, reword the error message to make it clearer that the > culprit is not necessarily the firmware type, but rather the > overall domain configuration. > > Suggested-by: Michael Kjörling <7d1340278...@ewoof.net> > Signed-off-by: Andrea Bolognani > --- > src/qemu/qemu_firmware.c| 2 +- > .../firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err | 2 +- > ...rmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err | 2 +- > .../firmware-auto-efi-rw-abi-update.x86_64-latest.err | 2 +- > tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err | 2 +- > 5 files changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Michal Privoznik And maybe we can document what us, libvirt developers, would do to debug this? I mean, we can put somewhere in a kbase article that we would enable debug logs and then check log messages where individual reasons for rejecting each fw are logged. Michal
Re: [libvirt PATCH] qemu: Improve error message for failed firmware autoselection
On Fri, Sep 22, 2023 at 03:44:55PM +0200, Michal Prívozník wrote: > On 9/22/23 15:26, Andrea Bolognani wrote: > > src/qemu/qemu_firmware.c| 2 +- > > .../firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err | 2 +- > > ...rmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err | 2 +- > > .../firmware-auto-efi-rw-abi-update.x86_64-latest.err | 2 +- > > tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err | 2 +- > > 5 files changed, 5 insertions(+), 5 deletions(-) > > Reviewed-by: Michal Privoznik Pushed, thanks. > And maybe we can document what us, libvirt developers, would do to debug > this? I mean, we can put somewhere in a kbase article that we would > enable debug logs and then check log messages where individual reasons > for rejecting each fw are logged. Can you think of an existing page where it would fit? Honestly it would probably make sense to have an entire kbase dedicated to firmware selection, with recommendations, best practices, gotchas and so on. Maybe we could then include the debugging steps as part of it? Note that the current output, even with debugging enabled, is not very easy to follow, so before suggesting that people go look at it I would probably want to improve upon it. Damn, I think you might just have nerd-sniped me into working on this! Let me add it to my to-do list :D -- Andrea Bolognani / Red Hat / Virtualization
[libvirt PATCH] qemu: Improve error message for failed firmware autoselection
The current message can be misleading, because it seems to suggest
that no firmware of the requested type is available on the system.
What actually happens most of the time, however, is that despite
having multiple firmwares of the right type to choose from, none
of them is suitable because of lacking some specific feature or
being incompatible with some setting that the user has explicitly
enabled.
Providing an error message that describes exactly the problem is
not feasible, since we would have to list each candidate along
with the reason why we rejected it, which would get out of hand
quickly.
As a small but hopefully helpful improvement over the current
situation, reword the error message to make it clearer that the
culprit is not necessarily the firmware type, but rather the
overall domain configuration.
Suggested-by: Michael Kjörling <7d1340278...@ewoof.net>
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_firmware.c| 2 +-
.../firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err | 2 +-
...rmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err | 2 +-
.../firmware-auto-efi-rw-abi-update.x86_64-latest.err | 2 +-
tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index 3dcd139a47..d39e61d071 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1854,7 +1854,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
}
} else {
virReportError(VIR_ERR_OPERATION_FAILED,
- _("Unable to find any firmware to satisfy '%1$s'"),
+ _("Unable to find '%1$s' firmware that is
compatible with the current configuration"),
virDomainOsDefFirmwareTypeToString(def->os.firmware));
return -1;
}
diff --git
a/tests/qemuxml2argvdata/firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err
b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err
index 4cfde1bd2e..3edb2b3451 100644
---
a/tests/qemuxml2argvdata/firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err
+++
b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path-nonstandard.x86_64-latest.err
@@ -1 +1 @@
-operation failed: Unable to find any firmware to satisfy 'efi'
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git
a/tests/qemuxml2argvdata/firmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err
b/tests/qemuxml2argvdata/firmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err
index 4cfde1bd2e..3edb2b3451 100644
---
a/tests/qemuxml2argvdata/firmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err
+++
b/tests/qemuxml2argvdata/firmware-auto-efi-nvram-template-nonstandard.x86_64-latest.err
@@ -1 +1 @@
-operation failed: Unable to find any firmware to satisfy 'efi'
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git
a/tests/qemuxml2argvdata/firmware-auto-efi-rw-abi-update.x86_64-latest.err
b/tests/qemuxml2argvdata/firmware-auto-efi-rw-abi-update.x86_64-latest.err
index 4cfde1bd2e..3edb2b3451 100644
--- a/tests/qemuxml2argvdata/firmware-auto-efi-rw-abi-update.x86_64-latest.err
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-rw-abi-update.x86_64-latest.err
@@ -1 +1 @@
-operation failed: Unable to find any firmware to satisfy 'efi'
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err
b/tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err
index 4cfde1bd2e..3edb2b3451 100644
--- a/tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-rw.x86_64-latest.err
@@ -1 +1 @@
-operation failed: Unable to find any firmware to satisfy 'efi'
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
--
2.41.0
Re: [PATCH 0/3] A couple of virtio-mem/virtio-pmem address improvements
On a Friday in 2023, Michal Privoznik wrote: *** BLURB HERE *** Michal Prívozník (3): domain_validate: Validate VIRTIO_PMEM address alignment virDomainMemoryDefValidate: Fix VIRTIO_MEM alignment check virDomainMemoryDefValidate: Check for overlapping memory devices src/conf/domain_validate.c| 59 ++- ...rtio-mem-overlap-address.x86_64-latest.err | 1 + ...ory-hotplug-virtio-mem-overlap-address.xml | 50 tests/qemuxml2argvtest.c | 1 + 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem-overlap-address.xml Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
