Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Wed, Mar 23, 2022 at 07:04:20PM +0100, Andrea Bolognani wrote:
The _ABSENT value of each enumeration has 'default' as string
representation, and when that's been formatted to XML we should
parse it back successfully, so we can't just treat encountering
it as an error.
Callers of virXMLPropTristate*() can of course still pass
VIR_XML_PROP_NONZERO explicitly to the helpers if the current
behavior is the one they want.
After this change, libvirtd no longer logs
error : virXMLPropEnumInternal:516 : XML error: Invalid value
for attribute 'value' in element 'allowReboot': 'default'.
when it gets restarted while there are running guests.
I had the same issue, but did not get around sending a patch for it.
What I assume is that allowReboot is one of the few, if not the only
exception where we format the default zero value.
Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
Signed-off-by: Andrea Bolognani
---
src/util/virxml.c | 4
1 file changed, 4 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 8ff59e7cda..db5212de20 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
virXMLPropFlags flags,
virTristateBool *result)
{
-flags |= VIR_XML_PROP_NONZERO;
-
I would rather change this flag to something like
VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this
flag making the callers be able to opt in for this behaviour rather then
all the others having to opt out.
Honestly I do not understand the flag as it is currently because it does
completely nothing when these functions just add the flag in
unconditionally.
return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString,
flags, result, VIR_TRISTATE_BOOL_ABSENT);
}
@@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
virXMLPropFlags flags,
virTristateSwitch *result)
{
-flags |= VIR_XML_PROP_NONZERO;
-
return virXMLPropEnumInternal(node, name, virTristateSwitchTypeFromString,
flags, result, VIR_TRISTATE_SWITCH_ABSENT);
}
--
2.35.1
signature.asc
Description: PGP signature
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
> The _ABSENT value of each enumeration has 'default' as string
> representation, and when that's been formatted to XML we should
> parse it back successfully, so we can't just treat encountering
> it as an error.
>
> Callers of virXMLPropTristate*() can of course still pass
> VIR_XML_PROP_NONZERO explicitly to the helpers if the current
> behavior is the one they want.
>
> After this change, libvirtd no longer logs
>
> error : virXMLPropEnumInternal:516 : XML error: Invalid value
> for attribute 'value' in element 'allowReboot': 'default'.
>
> when it gets restarted while there are running guests.
>
> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
> Signed-off-by: Andrea Bolognani
> ---
> src/util/virxml.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 8ff59e7cda..db5212de20 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> virXMLPropFlags flags,
> virTristateBool *result)
> {
> -flags |= VIR_XML_PROP_NONZERO;
> -
> return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString,
>flags, result, VIR_TRISTATE_BOOL_ABSENT);
> }
> @@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
> virXMLPropFlags flags,
> virTristateSwitch *result)
> {
> -flags |= VIR_XML_PROP_NONZERO;
> -
> return virXMLPropEnumInternal(node, name,
> virTristateSwitchTypeFromString,
>flags, result, VIR_TRISTATE_SWITCH_ABSENT);
> }
You can't do this without further consideration:
At least the usage in 'virDomainDiskSourceNetworkParse' where 'tls'
attribute is parsed declared in the schema as 'virYesNo' which allows
only 'yes' and 'no'.
If you want to do this you must fix all callers to pass
VIR_XML_PROP_NONZERO explicitly and then remove it from those you care
about.
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On 3/24/22 10:00, Peter Krempa wrote:
> On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
>> The _ABSENT value of each enumeration has 'default' as string
>> representation, and when that's been formatted to XML we should
>> parse it back successfully, so we can't just treat encountering
>> it as an error.
>>
>> Callers of virXMLPropTristate*() can of course still pass
>> VIR_XML_PROP_NONZERO explicitly to the helpers if the current
>> behavior is the one they want.
>>
>> After this change, libvirtd no longer logs
>>
>> error : virXMLPropEnumInternal:516 : XML error: Invalid value
>> for attribute 'value' in element 'allowReboot': 'default'.
>>
>> when it gets restarted while there are running guests.
>>
>> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
>> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
I wouldn't say Fixes, because this is basically an RFE, not a bugfix.
>> Signed-off-by: Andrea Bolognani
>> ---
>> src/util/virxml.c | 4
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/src/util/virxml.c b/src/util/virxml.c
>> index 8ff59e7cda..db5212de20 100644
>> --- a/src/util/virxml.c
>> +++ b/src/util/virxml.c
>> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
>> virXMLPropFlags flags,
>> virTristateBool *result)
>> {
>> -flags |= VIR_XML_PROP_NONZERO;
>> -
>> return virXMLPropEnumInternal(node, name, virTristateBoolTypeFromString,
>>flags, result, VIR_TRISTATE_BOOL_ABSENT);
>> }
>> @@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
>> virXMLPropFlags flags,
>> virTristateSwitch *result)
>> {
>> -flags |= VIR_XML_PROP_NONZERO;
>> -
>> return virXMLPropEnumInternal(node, name,
>> virTristateSwitchTypeFromString,
>>flags, result,
>> VIR_TRISTATE_SWITCH_ABSENT);
>> }
>
> You can't do this without further consideration:
>
> At least the usage in 'virDomainDiskSourceNetworkParse' where 'tls'
> attribute is parsed declared in the schema as 'virYesNo' which allows
> only 'yes' and 'no'.
>
> If you want to do this you must fix all callers to pass
> VIR_XML_PROP_NONZERO explicitly and then remove it from those you care
> about.
>
Don't forget about RNG change. I don't see much value in doing this
change without corresponding RNG update.
Michal
Re: [PATCH 1/4] conf: Introduce memory allocation threads
On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote:
Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of
Anything wrong with the old "commit ffac16fab33b" ?
specifying number of threads used to allocate memory. While it
defaults to the number of vCPUs, users might want to use a
different value (especially for humongous guests with gigantic
pages).
In general, on QEMU cmd line level it is possible to use
different number of threads per each memory-backend-* object, in
practical terms it's not useful. Therefore, use
to set guest wide value and let all memory devices 'inherit' it,
silently. IOW, don't introduce per device knob because that would
only complicate things for a little or no benefit.
Signed-off-by: Michal Privoznik
---
docs/formatdomain.rst| 6 --
docs/schemas/domaincommon.rng| 19 +--
src/conf/domain_conf.c | 15 ++-
src/conf/domain_conf.h | 1 +
tests/qemuxml2argvdata/memfd-memory-numa.xml | 2 +-
5 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9b1b69bb4d..8e25474db0 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1004,7 +1004,7 @@ Memory Backing
-
+
...
@@ -1054,7 +1054,9 @@ influence how virtual memory pages are backed by host
pages.
"private". This can be overridden per numa node by ``memAccess``.
``allocation``
Using the ``mode`` attribute, specify when to allocate the memory by
- supplying either "immediate" or "ondemand".
+ supplying either "immediate" or "ondemand". :since:`Since 8.2.0` this
+ attribute is optional among with ``threads`` attribute, that sets the number
+ of threads that hypervisor uses to allocate memory.
This is weird to read. Just say "Using the optional mode attribute" and
refer to threads as optional too. If anyone wants to use just the
allocation threads and leave out the mode they have to be on 8.2.0
anyway, so no need to complicate things.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e0dfc9e45f..2414a806d0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18914,6 +18914,13 @@ virDomainDefParseMemory(virDomainDef *def,
VIR_FREE(tmp);
}
+if (virXPathUInt("string(./memoryBacking/allocation/@threads)",
+ ctxt, &def->mem.allocation_threads) == -2) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Failed to parse memory allocation threads"));
+return -1;
+}
+
if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
/* hugepages will be used */
if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes))
< 0) {
@@ -27464,6 +27471,7 @@ virDomainMemorybackingFormat(virBuffer *buf,
const virDomainMemtune *mem)
{
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+g_auto(virBuffer) allocAttrBuf = VIR_BUFFER_INITIALIZER;
if (mem->nhugepages)
virDomainHugepagesFormat(&childBuf, mem->hugepages, mem->nhugepages);
@@ -27478,8 +27486,13 @@ virDomainMemorybackingFormat(virBuffer *buf,
virBufferAsprintf(&childBuf, "\n",
virDomainMemoryAccessTypeToString(mem->access));
if (mem->allocation)
-virBufferAsprintf(&childBuf, "\n",
+virBufferAsprintf(&allocAttrBuf, " mode='%s'",
virDomainMemoryAllocationTypeToString(mem->allocation));
+if (mem->allocation_threads)
Here you check if (mem->allocation_threads), but in 3/4 you check if
(allocation_threads > 0), which is a bit inconsistent. I prefer the
former although I know we had some disputes with this, so pick whatever
one and make it consistent.
signature.asc
Description: PGP signature
Re: [PATCH 2/4] qemu_capabilities: Detect memory-backend-*.prealloc-threads property
On Tue, Mar 22, 2022 at 04:05:15PM +0100, Michal Privoznik wrote:
The prealloc-threads is property of memory-backend class which is
parent to the other three classes memory-backend-{ram,file,memfd}.
Therefore the property is present for all, or none if QEMU is
older than v5.0.0-rc0~75^2~1^2~3 which introduced the property.
Anyway, the .reserve property is the same story, and we chose
memory-backend-file to detect it, so stick with our earlier
decision and use the same backend to detect this new property.
Signed-off-by: Michal Privoznik
Reviewed-by: Martin Kletzander
signature.asc
Description: PGP signature
Re: [PATCH 3/4] qemu_validate: Validate prealloc threads against qemuCpas
On Tue, Mar 22, 2022 at 04:05:16PM +0100, Michal Privoznik wrote: Only fairly new QEMUs are capable of user provided number of preallocation threads. Validate this assumption. Signed-off-by: Michal Privoznik Reviewed-by: Martin Kletzander signature.asc Description: PGP signature
Re: [PATCH 4/4] qemu_command: Generate prealloc-threads property
On Tue, Mar 22, 2022 at 04:05:17PM +0100, Michal Privoznik wrote: Let's generate prealloc-threads property onto the cmd line if domain configuration requests so. Signed-off-by: Michal Privoznik Reviewed-by: Martin Kletzander signature.asc Description: PGP signature
[PATCH] tests: add dependencies to meson declaration
Make sure that all tests are run after the helpers and mocks are
(re)built. This enables for example using "meson test" as the
command line passed to "git bisect run".
Signed-off-by: Paolo Bonzini
---
tests/meson.build | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/meson.build b/tests/meson.build
index 8f92f2033f..fd78d8a2fd 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -191,8 +191,9 @@ test_file_wrapper_lib = static_library(
dependencies: [ tests_dep ],
)
+tests_deps = []
foreach mock : mock_libs
- shared_library(
+ tests_deps += shared_library(
mock['name'],
mock.get('sources', [ '@0@.c'.format(mock['name']) ]),
override_options: [
@@ -215,7 +216,7 @@ endforeach
# Must not link to any libvirt modules - libc only otherwise external
# libraries might unexpectedly leak file descriptors into commandhelper
# invalidating the test logic assumptions.
-executable(
+tests_deps += executable(
'commandhelper',
[ 'commandhelper.c' ],
dependencies: [
@@ -227,7 +228,7 @@ executable(
)
# This is a fake SSH we use from virnetsockettest
-executable(
+tests_deps += executable(
'ssh',
[ 'ssh.c' ],
dependencies: [
@@ -592,7 +593,7 @@ foreach data : tests
# default meson timeout
timeout = 30
endif
- test(data['name'], test_bin, env: tests_env, timeout: timeout)
+ test(data['name'], test_bin, env: tests_env, timeout: timeout, depends:
tests_deps)
endforeach
--
2.35.1
[PATCH 0/2] domain, qemu: add support for clearing TSC on reset
Some versions of Windows hang on reboot if their TSC value is greater
than 2^54. The workaround is to reset the TSC to a small value, and
is implemented by QEMU and ESXi. This series adds a domain attribute
for this, and implements it in QEMU.
Paolo
Paolo Bonzini (2):
domain: add tsc.on_reboot element
qemu: add support for tsc.on_reboot element
docs/formatdomain.rst | 4 +++
src/conf/domain_conf.c| 22
src/conf/domain_conf.h| 10 ++
src/conf/schemas/domaincommon.rng | 9 +
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 4 +++
src/qemu/qemu_validate.c | 36 +--
.../caps_7.0.0.x86_64.replies | 4 +++
.../caps_7.0.0.x86_64.xml | 1 +
...uency.args => cpu-tsc-clear-on-reset.args} | 2 +-
... cpu-tsc-clear-on-reset.x86_64-7.0.0.args} | 6 ++--
...equency.xml => cpu-tsc-clear-on-reset.xml} | 2 +-
tests/qemuxml2argvtest.c | 2 ++
14 files changed, 89 insertions(+), 16 deletions(-)
copy tests/qemuxml2argvdata/{cpu-tsc-frequency.args =>
cpu-tsc-clear-on-reset.args} (97%)
copy tests/qemuxml2argvdata/{virtio-rng-builtin.x86_64-latest.args =>
cpu-tsc-clear-on-reset.x86_64-7.0.0.args} (77%)
copy tests/qemuxml2argvdata/{cpu-tsc-frequency.xml =>
cpu-tsc-clear-on-reset.xml} (95%)
--
2.35.1
[PATCH 1/2] domain: add tsc.on_reboot element
Some versions of Windows hang on reboot if their TSC value is greater
than 2^54. The workaround is to reset the TSC to a small value. Add
to the domain configuration an attribute for this. It can be used
by QEMU and in principle also by ESXi, which has a property called
monitor_control.enable_softResetClearTSC as well.
Signed-off-by: Paolo Bonzini
---
docs/formatdomain.rst | 4
src/conf/domain_conf.c| 22 ++
src/conf/domain_conf.h| 10 ++
src/conf/schemas/domaincommon.rng | 9 +
4 files changed, 45 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index d188de4858..c9319e42ac 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2241,6 +2241,10 @@ Windows, however, expects it to be in so called
'localtime'.
``frequency``
The ``frequency`` attribute is an unsigned integer specifying the
frequency at which ``name="tsc"`` runs.
+ ``on_reboot``
+ The ``on_reboot`` attribute controls how the ``name="tsc"`` timer behaves
+ when the VM is reset, and can be "default", "clear" or "keep". The reset
+ behavior of other timers is hardcoded, and depends on the type of timer.
``mode``
The ``mode`` attribute controls how the ``name="tsc"`` timer is managed,
and can be "auto", "native", "emulate", "paravirt", or "smpsafe". Other
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 153954a0b0..1893b8bbca 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1217,6 +1217,13 @@ VIR_ENUM_IMPL(virDomainTimerMode,
"smpsafe",
);
+VIR_ENUM_IMPL(virDomainTimerRebootMode,
+ VIR_DOMAIN_TIMER_REBOOT_MODE_LAST,
+ "default",
+ "keep",
+ "clear",
+);
+
VIR_ENUM_IMPL(virDomainStartupPolicy,
VIR_DOMAIN_STARTUP_POLICY_LAST,
"default",
@@ -12022,6 +12029,7 @@ virDomainTimerDefParseXML(xmlNodePtr node,
g_autofree char *tickpolicy = NULL;
g_autofree char *track = NULL;
g_autofree char *mode = NULL;
+g_autofree char *reboot = NULL;
def = g_new0(virDomainTimerDef, 1);
@@ -12080,6 +12088,15 @@ virDomainTimerDefParseXML(xmlNodePtr node,
}
}
+reboot = virXMLPropString(node, "on_reboot");
+if (reboot != NULL) {
+if ((def->reboot = virDomainTimerRebootModeTypeFromString(reboot)) <=
0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown timer reboot mode '%s'"), reboot);
+goto error;
+}
+}
+
catchup = virXPathNode("./catchup", ctxt);
if (catchup != NULL) {
ret = virXPathULong("string(./catchup/@threshold)", ctxt,
@@ -26151,6 +26168,11 @@ virDomainTimerDefFormat(virBuffer *buf,
virBufferAsprintf(&timerAttr, " mode='%s'",
virDomainTimerModeTypeToString(def->mode));
}
+
+if (def->reboot) {
+virBufferAsprintf(&timerAttr, " on_reboot='%s'",
+ virDomainTimerRebootModeTypeToString(def->mode));
+}
}
if (def->catchup.threshold > 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b69abfa270..3618410f6c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2408,6 +2408,14 @@ typedef enum {
VIR_DOMAIN_TIMER_MODE_LAST
} virDomainTimerModeType;
+typedef enum {
+VIR_DOMAIN_TIMER_REBOOT_MODE_DEFAULT = 0,
+VIR_DOMAIN_TIMER_REBOOT_MODE_KEEP,
+VIR_DOMAIN_TIMER_REBOOT_MODE_CLEAR,
+
+VIR_DOMAIN_TIMER_REBOOT_MODE_LAST
+} virDomainTimerRebootModeType;
+
typedef enum {
VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC = 0,
VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO,
@@ -2439,6 +2447,7 @@ struct _virDomainTimerDef {
/* frequency & mode are only valid for name='tsc' */
unsigned long long frequency; /* in Hz, unspecified = 0 */
int mode; /* enum virDomainTimerModeType */
+int reboot; /* enum virDomainTimerRebootModeType */
};
typedef enum {
@@ -4032,6 +4041,7 @@ VIR_ENUM_DECL(virDomainClockBasis);
VIR_ENUM_DECL(virDomainTimerName);
VIR_ENUM_DECL(virDomainTimerTrack);
VIR_ENUM_DECL(virDomainTimerTickpolicy);
+VIR_ENUM_DECL(virDomainTimerRebootMode);
VIR_ENUM_DECL(virDomainTimerMode);
VIR_ENUM_DECL(virDomainCpuPlacementMode);
diff --git a/src/conf/schemas/domaincommon.rng
b/src/conf/schemas/domaincommon.rng
index 9c1b64a644..10d0404889 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -1285,6 +1285,15 @@
+
+
+
+default
+clear
+keep
+
+
+
--
2.35.1
[PATCH 2/2] qemu: add support for tsc.on_reboot element
QEMU 7.0.0 adds a new property tsc-clear-on-reset to x86 CPU, corresponding
to Libvirt's element. Plumb it in the validation,
command line handling and tests.
Signed-off-by: Paolo Bonzini
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 4 +++
src/qemu/qemu_validate.c | 36 +--
.../caps_7.0.0.x86_64.replies | 4 +++
.../caps_7.0.0.x86_64.xml | 1 +
...uency.args => cpu-tsc-clear-on-reset.args} | 2 +-
... cpu-tsc-clear-on-reset.x86_64-7.0.0.args} | 6 ++--
...equency.xml => cpu-tsc-clear-on-reset.xml} | 2 +-
tests/qemuxml2argvtest.c | 2 ++
10 files changed, 44 insertions(+), 16 deletions(-)
copy tests/qemuxml2argvdata/{cpu-tsc-frequency.args =>
cpu-tsc-clear-on-reset.args} (97%)
copy tests/qemuxml2argvdata/{virtio-rng-builtin.x86_64-latest.args =>
cpu-tsc-clear-on-reset.x86_64-7.0.0.args} (77%)
copy tests/qemuxml2argvdata/{cpu-tsc-frequency.xml =>
cpu-tsc-clear-on-reset.xml} (95%)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 32980e7330..d22bbee70e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -668,6 +668,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 425 */
"blockdev.nbd.tls-hostname", /*
QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME */
+ "x86-cpu.tsc-clear-on-reset", /*
QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET */
);
@@ -1775,6 +1776,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsObjectPropsMemoryBackendMemfd[]
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMaxCPU[] = {
{ "unavailable-features", QEMU_CAPS_CPU_UNAVAILABLE_FEATURES },
{ "kvm-no-adjvtime", QEMU_CAPS_CPU_KVM_NO_ADJVTIME },
+{ "tsc-clear-on-reset", QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET },
{ "migratable", QEMU_CAPS_CPU_MIGRATABLE },
};
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 0a215a11d5..7aee73a725 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -643,6 +643,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 425 */
QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME, /* tls hostname can be overriden for
NBD clients */
+QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET, /* x86-cpu.tsc-clear-on-reset */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c836799888..8ecede0ade 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6613,6 +6613,10 @@ qemuBuildCpuCommandLine(virCommand *cmd,
case VIR_DOMAIN_TIMER_NAME_TSC:
if (timer->frequency > 0)
virBufferAsprintf(&buf, ",tsc-frequency=%llu",
timer->frequency);
+if (timer->reboot == VIR_DOMAIN_TIMER_REBOOT_MODE_CLEAR)
+virBufferAddLit(&buf, ",tsc-clear-on-reset=on");
+else if (timer->reboot == VIR_DOMAIN_TIMER_REBOOT_MODE_KEEP)
+virBufferAddLit(&buf, ",tsc-clear-on-reset=off");
break;
case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
switch (timer->tickpolicy) {
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index f27e480696..c4ab2976dc 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -396,10 +396,11 @@ static int
qemuValidateDomainDefClockTimers(const virDomainDef *def,
virQEMUCaps *qemuCaps)
{
+virDomainTimerDef *timer;
size_t i;
for (i = 0; i < def->clock.ntimers; i++) {
-virDomainTimerDef *timer = def->clock.timers[i];
+timer = def->clock.timers[i];
switch ((virDomainTimerNameType)timer->name) {
case VIR_DOMAIN_TIMER_NAME_PLATFORM:
@@ -409,20 +410,25 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def,
return -1;
case VIR_DOMAIN_TIMER_NAME_TSC:
-case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
-case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
-if (!ARCH_IS_X86(def->os.arch) && timer->present ==
VIR_TRISTATE_BOOL_YES) {
+if (!ARCH_IS_X86(def->os.arch) && timer->present ==
VIR_TRISTATE_BOOL_YES)
+goto no_support;
+
+if (timer->reboot != VIR_DOMAIN_TIMER_REBOOT_MODE_DEFAULT &&
+!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_X86_CPU_TSC_CLEAR_ON_RESET)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Configuring the '%s' timer is not supported "
- "for virtType=%s arch=%s machine=%s guests"),
- virDomainTimerNameTypeToString(timer->name),
- virDomainVirtTypeToString(def->virtType),
- virArchToString(def->os.arch),
-
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 10:14:18AM +0100, Michal Prívozník wrote:
> On 3/24/22 10:00, Peter Krempa wrote:
> > On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
> >> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
> >> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
>
> I wouldn't say Fixes, because this is basically an RFE, not a bugfix.
But it *is* a bugfix: we have stopped accepting values that we
accepted in the past, and libvirtd has started logging spurious error
messages as a consequence of that.
I agree that the Fixes tags shown above are not accurate: they should
instead point to commits like 593140dabd66, which are the ones that
replaced an open-coded implementation that would accept 'default' as
a valid value with a call to an helper that doesn't. I need to spend
some time digging through the git history to come up with the actual
list though :)
> >> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> >> virXMLPropFlags flags,
> >> virTristateBool *result)
> >> {
> >> -flags |= VIR_XML_PROP_NONZERO;
> >> -
> >> return virXMLPropEnumInternal(node, name,
> >> virTristateBoolTypeFromString,
> >>flags, result,
> >> VIR_TRISTATE_BOOL_ABSENT);
> >> }
> >> @@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
> >> virXMLPropFlags flags,
> >> virTristateSwitch *result)
> >> {
> >> -flags |= VIR_XML_PROP_NONZERO;
> >> -
> >> return virXMLPropEnumInternal(node, name,
> >> virTristateSwitchTypeFromString,
> >>flags, result,
> >> VIR_TRISTATE_SWITCH_ABSENT);
> >> }
> >
> > You can't do this without further consideration:
> >
> > At least the usage in 'virDomainDiskSourceNetworkParse' where 'tls'
> > attribute is parsed declared in the schema as 'virYesNo' which allows
> > only 'yes' and 'no'.
> >
> > If you want to do this you must fix all callers to pass
> > VIR_XML_PROP_NONZERO explicitly and then remove it from those you care
> > about.
Have you perhaps missed the cover letter? I posted this as RFC
specifically because I'm aware of the fact that I need to go through
all the commits that introduced a call to virXMLPropTristate*() and
check whether that changed the behavior compared to the existing
code. I just didn't feel like doing all that archeology work without
validating the approach first was a good idea :)
> Don't forget about RNG change. I don't see much value in doing this
> change without corresponding RNG update.
I don't think RNG changes are needed. I'm just trying to restore the
original behavior, which the RNG should already account for. I am
specifically not interested in adding more scenarios where 'default'
is formatted to XML: not formatting the attribute at all is clearly
the better choice, but there are some cases (such as allowReboot)
where we have historically formatted that value and so we also need
to be able to parse it back.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
> What I assume is that allowReboot is one of the few, if not the only
> exception where we format the default zero value.
My guess is that you're right, but I haven't actually verified this
yet :)
> > @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> >virXMLPropFlags flags,
> >virTristateBool *result)
> > {
> > -flags |= VIR_XML_PROP_NONZERO;
> > -
>
> I would rather change this flag to something like
> VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this
> flag making the callers be able to opt in for this behaviour rather then
> all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it
would require adding a check to make sure that only one of
VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed.
I'll see how clunky that looks.
> Honestly I do not understand the flag as it is currently because it does
> completely nothing when these functions just add the flag in
> unconditionally.
There are other helpers, such as virXMLPropUInt(), that don't
unconditionally set the flag. For all those, the behavior is up to
the caller, and having the flag makes sense.
--
Andrea Bolognani / Red Hat / Virtualization
[PATCH] meson: do not look for librbd/librados if not requested
rbd_dep is not used if -Dstorage_rbd=disabled. Do not bother looking for
the libraries that compose it if the rbd storage backend was not requested.
Signed-off-by: Paolo Bonzini
---
meson.build | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 2c1d3bc64d..eb13c7efa4 100644
--- a/meson.build
+++ b/meson.build
@@ -1127,8 +1127,8 @@ parallels_sdk_dep = dependency('parallels-sdk', version:
'>=' + parallels_sdk_ve
pciaccess_version = '0.10.0'
pciaccess_dep = dependency('pciaccess', version: '>=' + pciaccess_version,
required: get_option('pciaccess'))
-rbd_dep = cc.find_library('rbd', required: false)
-rados_dep = cc.find_library('rados', required: false)
+rbd_dep = cc.find_library('rbd', required: get_option('storage_rbd'))
+rados_dep = cc.find_library('rados', required: get_option('storage_rbd'))
if rbd_dep.found() and not cc.has_function('rbd_get_features', dependencies:
rbd_dep)
rbd_dep = dependency('', required: false)
endif
--
2.35.1
[PATCH] meson: do not look for libdevmapper/libparted if not requested
devmapper_dep and libparted_dep are not used if -Dstorage_disk=disabled. Do
not bother looking for these libraries if the disk storage backend was not
requested.
Signed-off-by: Paolo Bonzini
---
meson.build | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index eb13c7efa4..9d1f90fc80 100644
--- a/meson.build
+++ b/meson.build
@@ -933,7 +933,7 @@ if curl_dep.found()
endif
devmapper_version = '1.0.0'
-devmapper_dep = dependency('devmapper', version: '>=' + devmapper_version,
required: false)
+devmapper_dep = dependency('devmapper', version: '>=' + devmapper_version,
required: get_option('storage_disk'))
if devmapper_dep.found()
conf.set('WITH_DEVMAPPER', 1)
endif
@@ -1013,7 +1013,7 @@ else
endif
libparted_version = '1.8.0'
-libparted_dep = dependency('libparted', version: '>=' + libparted_version,
required: false)
+libparted_dep = dependency('libparted', version: '>=' + libparted_version,
required: get_option('storage_disk'))
libpcap_version = '1.5.0'
if not get_option('libpcap').disabled()
--
2.35.1
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 10:32:51AM +, Andrea Bolognani wrote:
On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
What I assume is that allowReboot is one of the few, if not the only
exception where we format the default zero value.
My guess is that you're right, but I haven't actually verified this
yet :)
> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
>virXMLPropFlags flags,
>virTristateBool *result)
> {
> -flags |= VIR_XML_PROP_NONZERO;
> -
I would rather change this flag to something like
VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this
flag making the callers be able to opt in for this behaviour rather then
all the others having to opt out.
Yeah, this sounds better from the caller's point of view, but it
would require adding a check to make sure that only one of
VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed.
I'll see how clunky that looks.
I meant only keeping the new one, although I must admit I completely
missed the fact that it is used properly somewhere else. If we went
that way then the flag might itself be a tristate enum "default,
allow_zero, non_zero" where default is different for numbers and enums,
but that seems even clunkier and I don't like that myself.
Honestly I do not understand the flag as it is currently because it does
completely nothing when these functions just add the flag in
unconditionally.
There are other helpers, such as virXMLPropUInt(), that don't
unconditionally set the flag. For all those, the behavior is up to
the caller, and having the flag makes sense.
It looks like the issue stems from the fact that we are trying to have a
very "universal" approach to parsing the properties while at the same
time having very different usage of enum and number values.
virXMLPropUInt() and virXMLPropEnum() don't even evaluate the flags in
the same function they would both bubble to. Maybe we could use the
fact that some integer properties default to -1 and some to 0 and make
the default explicit... I don't know, it all seems like
overcomplicating to me.
In any case I feel like we want not to parse default enum values (not
only booleans and switches) and going through all the callers seems very
tedious. And the opposite seems to be the case for numbers. But I just
did a quick check in the code, nothing thorough.
--
Andrea Bolognani / Red Hat / Virtualization
signature.asc
Description: PGP signature
Re: [PATCH 1/4] conf: Introduce memory allocation threads
On 3/24/22 10:26, Martin Kletzander wrote:
> On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote:
>> Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of
>
> Anything wrong with the old "commit ffac16fab33b" ?
Not specifically no. But I do it this ways so that it's visible at the
first sight what QEMU version was this feature introduced in. And it
works with 'git show' too (for those who are interested in the
particular commit). IOW:
"Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable .."
contains more information than:
"Since its commit ffac16fab33b QEMU is capable .."
But I don't care that much. I agree that in this particular case it's a
bit messy, because QEMU doesn't have linear history. It's way nicer with
libvirt commits.
>
>> specifying number of threads used to allocate memory. While it
>> defaults to the number of vCPUs, users might want to use a
>> different value (especially for humongous guests with gigantic
>> pages).
>>
>> In general, on QEMU cmd line level it is possible to use
>> different number of threads per each memory-backend-* object, in
>> practical terms it's not useful. Therefore, use
>> to set guest wide value and let all memory devices 'inherit' it,
>> silently. IOW, don't introduce per device knob because that would
>> only complicate things for a little or no benefit.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> docs/formatdomain.rst | 6 --
>> docs/schemas/domaincommon.rng | 19 +--
>> src/conf/domain_conf.c | 15 ++-
>> src/conf/domain_conf.h | 1 +
>> tests/qemuxml2argvdata/memfd-memory-numa.xml | 2 +-
>> 5 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 9b1b69bb4d..8e25474db0 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -1004,7 +1004,7 @@ Memory Backing
>>
>>
>>
>> -
>> +
>>
>>
>> ...
>> @@ -1054,7 +1054,9 @@ influence how virtual memory pages are backed by
>> host pages.
>> "private". This can be overridden per numa node by ``memAccess``.
>> ``allocation``
>> Using the ``mode`` attribute, specify when to allocate the memory by
>> - supplying either "immediate" or "ondemand".
>> + supplying either "immediate" or "ondemand". :since:`Since 8.2.0` this
>> + attribute is optional among with ``threads`` attribute, that sets
>> the number
>> + of threads that hypervisor uses to allocate memory.
>
> This is weird to read. Just say "Using the optional mode attribute" and
> refer to threads as optional too. If anyone wants to use just the
> allocation threads and leave out the mode they have to be on 8.2.0
> anyway, so no need to complicate things.
Alright, fair enough.
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e0dfc9e45f..2414a806d0 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18914,6 +18914,13 @@ virDomainDefParseMemory(virDomainDef *def,
>> VIR_FREE(tmp);
>> }
>>
>> + if (virXPathUInt("string(./memoryBacking/allocation/@threads)",
>> + ctxt, &def->mem.allocation_threads) == -2) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Failed to parse memory allocation threads"));
>> + return -1;
>> + }
>> +
>> if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
>> /* hugepages will be used */
>> if ((n = virXPathNodeSet("./memoryBacking/hugepages/page",
>> ctxt, &nodes)) < 0) {
>> @@ -27464,6 +27471,7 @@ virDomainMemorybackingFormat(virBuffer *buf,
>> const virDomainMemtune *mem)
>> {
>> g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>> + g_auto(virBuffer) allocAttrBuf = VIR_BUFFER_INITIALIZER;
>>
>> if (mem->nhugepages)
>> virDomainHugepagesFormat(&childBuf, mem->hugepages,
>> mem->nhugepages);
>> @@ -27478,8 +27486,13 @@ virDomainMemorybackingFormat(virBuffer *buf,
>> virBufferAsprintf(&childBuf, "\n",
>>
>> virDomainMemoryAccessTypeToString(mem->access));
>> if (mem->allocation)
>> - virBufferAsprintf(&childBuf, "\n",
>> + virBufferAsprintf(&allocAttrBuf, " mode='%s'",
>>
>> virDomainMemoryAllocationTypeToString(mem->allocation));
>> + if (mem->allocation_threads)
>
> Here you check if (mem->allocation_threads), but in 3/4 you check if
> (allocation_threads > 0), which is a bit inconsistent. I prefer the
> former although I know we had some disputes with this, so pick whatever
> one and make it consistent.
D'oh! I remember I wanted to fix this. > 0 it is.
Michal
[PATCH v2] conf: Introduce memory allocation threads
Since its v5.0.0 release QEMU is capable of specifying number of
threads used to allocate memory. It defaults to 1, which may be
too low for humongous guests with gigantic pages.
In general, on QEMU cmd line level it is possible to use
different number of threads per each memory-backend-* object, in
practical terms it's not useful. Therefore, use
to set guest wide value and let all memory devices 'inherit' it,
silently. IOW, don't introduce per device knob because that would
only complicate things for a little or no benefit.
Signed-off-by: Michal Privoznik
---
docs/formatdomain.rst| 8 +---
src/conf/domain_conf.c | 15 ++-
src/conf/domain_conf.h | 1 +
src/conf/schemas/domaincommon.rng| 19 +--
tests/qemuxml2argvdata/memfd-memory-numa.xml | 2 +-
5 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index d188de4858..e492532004 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1004,7 +1004,7 @@ Memory Backing
-
+
...
@@ -1053,8 +1053,10 @@ influence how virtual memory pages are backed by host
pages.
Using the ``mode`` attribute, specify if the memory is to be "shared" or
"private". This can be overridden per numa node by ``memAccess``.
``allocation``
- Using the ``mode`` attribute, specify when to allocate the memory by
- supplying either "immediate" or "ondemand".
+ Using the optional ``mode`` attribute, specify when to allocate the memory
by
+ supplying either "immediate" or "ondemand". :since:`Since 8.2.0` it is
+ possible to set the number of threads that hypervisor uses to allocate
+ memory via ``threads`` attribute.
``discard``
When set and supported by hypervisor the memory content is discarded just
before guest shuts down (or when DIMM module is unplugged). Please note that
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 153954a0b0..731139f80f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18915,6 +18915,13 @@ virDomainDefParseMemory(virDomainDef *def,
VIR_FREE(tmp);
}
+if (virXPathUInt("string(./memoryBacking/allocation/@threads)",
+ ctxt, &def->mem.allocation_threads) == -2) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Failed to parse memory allocation threads"));
+return -1;
+}
+
if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
/* hugepages will be used */
if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt,
&nodes)) < 0) {
@@ -27465,6 +27472,7 @@ virDomainMemorybackingFormat(virBuffer *buf,
const virDomainMemtune *mem)
{
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+g_auto(virBuffer) allocAttrBuf = VIR_BUFFER_INITIALIZER;
if (mem->nhugepages)
virDomainHugepagesFormat(&childBuf, mem->hugepages, mem->nhugepages);
@@ -27479,8 +27487,13 @@ virDomainMemorybackingFormat(virBuffer *buf,
virBufferAsprintf(&childBuf, "\n",
virDomainMemoryAccessTypeToString(mem->access));
if (mem->allocation)
-virBufferAsprintf(&childBuf, "\n",
+virBufferAsprintf(&allocAttrBuf, " mode='%s'",
virDomainMemoryAllocationTypeToString(mem->allocation));
+if (mem->allocation_threads > 0)
+virBufferAsprintf(&allocAttrBuf, " threads='%u'",
mem->allocation_threads);
+
+virXMLFormatElement(&childBuf, "allocation", &allocAttrBuf, NULL);
+
if (mem->discard)
virBufferAddLit(&childBuf, "\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b69abfa270..49c964e6e1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2702,6 +2702,7 @@ struct _virDomainMemtune {
int source; /* enum virDomainMemorySource */
int access; /* enum virDomainMemoryAccess */
int allocation; /* enum virDomainMemoryAllocation */
+unsigned int allocation_threads;
virTristateBool discard;
};
diff --git a/src/conf/schemas/domaincommon.rng
b/src/conf/schemas/domaincommon.rng
index 9c1b64a644..34bccee2f5 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -740,12 +740,19 @@
-
-
-immediate
-ondemand
-
-
+
+
+
+ immediate
+ ondemand
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvdata/memfd-memory-numa.xml
b/tests/qem
Re: [PATCH 1/4] conf: Introduce memory allocation threads
On Thu, Mar 24, 2022 at 12:15:43PM +0100, Michal Prívozník wrote: > On 3/24/22 10:26, Martin Kletzander wrote: > > On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote: > >> Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of > > > > Anything wrong with the old "commit ffac16fab33b" ? > > Not specifically no. But I do it this ways so that it's visible at the > first sight what QEMU version was this feature introduced in. And it > works with 'git show' too (for those who are interested in the > particular commit). IOW: > > "Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable .." How do you generate such a reference ? It is different from what a simple git describe shows $ git describe --match 'v*' ffac16fab33b v4.2.0-1618-gffac16fab3 and I feel the git describe output is much nicer, as it actually includes the commit short hash still. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 10:03:36 +, Andrea Bolognani wrote:
> On Thu, Mar 24, 2022 at 10:14:18AM +0100, Michal Prívozník wrote:
> > On 3/24/22 10:00, Peter Krempa wrote:
> > > On Wed, Mar 23, 2022 at 19:04:20 +0100, Andrea Bolognani wrote:
> > >> Fixes: 8861d96c880d25c940456c5997a2ac93fc073c78
> > >> Fixes: c8726ede83ac117cb18c0b0a1fbfeeac8b80384b
> >
> > I wouldn't say Fixes, because this is basically an RFE, not a bugfix.
>
> But it *is* a bugfix: we have stopped accepting values that we
> accepted in the past, and libvirtd has started logging spurious error
> messages as a consequence of that.
>
> I agree that the Fixes tags shown above are not accurate: they should
> instead point to commits like 593140dabd66, which are the ones that
> replaced an open-coded implementation that would accept 'default' as
> a valid value with a call to an helper that doesn't. I need to spend
> some time digging through the git history to come up with the actual
> list though :)
>
> > >> @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> > >> virXMLPropFlags flags,
> > >> virTristateBool *result)
> > >> {
> > >> -flags |= VIR_XML_PROP_NONZERO;
> > >> -
> > >> return virXMLPropEnumInternal(node, name,
> > >> virTristateBoolTypeFromString,
> > >>flags, result,
> > >> VIR_TRISTATE_BOOL_ABSENT);
> > >> }
> > >> @@ -573,8 +571,6 @@ virXMLPropTristateSwitch(xmlNodePtr node,
> > >> virXMLPropFlags flags,
> > >> virTristateSwitch *result)
> > >> {
> > >> -flags |= VIR_XML_PROP_NONZERO;
> > >> -
> > >> return virXMLPropEnumInternal(node, name,
> > >> virTristateSwitchTypeFromString,
> > >>flags, result,
> > >> VIR_TRISTATE_SWITCH_ABSENT);
> > >> }
> > >
> > > You can't do this without further consideration:
> > >
> > > At least the usage in 'virDomainDiskSourceNetworkParse' where 'tls'
> > > attribute is parsed declared in the schema as 'virYesNo' which allows
> > > only 'yes' and 'no'.
> > >
> > > If you want to do this you must fix all callers to pass
> > > VIR_XML_PROP_NONZERO explicitly and then remove it from those you care
> > > about.
>
> Have you perhaps missed the cover letter? I posted this as RFC
> specifically because I'm aware of the fact that I need to go through
> all the commits that introduced a call to virXMLPropTristate*() and
> check whether that changed the behavior compared to the existing
> code. I just didn't feel like doing all that archeology work without
> validating the approach first was a good idea :)
I did skip the cover letter at first. Your example though doesn't show
that you wanted to fix all the callers first, because patch like this
just breaks stuff.
Ideally with the flag removal you update _all_ callers to pass the flag
explicitly and remove the flag in a subsequent path which will be the
real bugfix.
Here the commit + cover letter looks like a "I want to fix my thing and
don't care about the rest" type of commit.
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 12:02:05 +0100, Martin Kletzander wrote:
> On Thu, Mar 24, 2022 at 10:32:51AM +, Andrea Bolognani wrote:
> > On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote:
> > > What I assume is that allowReboot is one of the few, if not the only
> > > exception where we format the default zero value.
> >
> > My guess is that you're right, but I haven't actually verified this
> > yet :)
> >
> > > > @@ -545,8 +545,6 @@ virXMLPropTristateBool(xmlNodePtr node,
> > > >virXMLPropFlags flags,
> > > >virTristateBool *result)
> > > > {
> > > > -flags |= VIR_XML_PROP_NONZERO;
> > > > -
> > >
> > > I would rather change this flag to something like
> > > VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this
> > > flag making the callers be able to opt in for this behaviour rather then
> > > all the others having to opt out.
> >
> > Yeah, this sounds better from the caller's point of view, but it
> > would require adding a check to make sure that only one of
> > VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed.
> > I'll see how clunky that looks.
> >
>
> I meant only keeping the new one, although I must admit I completely
> missed the fact that it is used properly somewhere else. If we went
> that way then the flag might itself be a tristate enum "default,
> allow_zero, non_zero" where default is different for numbers and enums,
> but that seems even clunkier and I don't like that myself.
Alternatively introduce a new helper e.g. virXMLPropTristateBoolDefault
which will allow also the 'default' value to be specified explicitly.
In such case we can document the difference in the comment and don't
have to touch every other usage.
Re: [PATCH 1/4] conf: Introduce memory allocation threads
On Thu, Mar 24, 2022 at 11:22:54AM +, Daniel P. Berrangé wrote: On Thu, Mar 24, 2022 at 12:15:43PM +0100, Michal Prívozník wrote: On 3/24/22 10:26, Martin Kletzander wrote: > On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote: >> Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of > > Anything wrong with the old "commit ffac16fab33b" ? Not specifically no. But I do it this ways so that it's visible at the first sight what QEMU version was this feature introduced in. And it works with 'git show' too (for those who are interested in the particular commit). IOW: "Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable .." How do you generate such a reference ? It is different from what a simple git describe shows $ git describe --match 'v*' ffac16fab33b v4.2.0-1618-gffac16fab3 and I feel the git describe output is much nicer, as it actually includes the commit short hash still. I think Michal used --contains which clearly shows from which tag is the commit included, but tracking down through merge commits is what makes it look like the above. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| signature.asc Description: PGP signature
Re: [RFC 1/1] virxml: Accept 'default' for virTristate* properties
On Thu, Mar 24, 2022 at 12:42:12PM +0100, Peter Krempa wrote: > On Thu, Mar 24, 2022 at 12:02:05 +0100, Martin Kletzander wrote: > > On Thu, Mar 24, 2022 at 10:32:51AM +, Andrea Bolognani wrote: > > > On Thu, Mar 24, 2022 at 09:58:28AM +0100, Martin Kletzander wrote: > > > > I would rather change this flag to something like > > > > VIR_XML_PROP_ALLOW_ZERO and only allow parsing default values with this > > > > flag making the callers be able to opt in for this behaviour rather then > > > > all the others having to opt out. > > > > > > Yeah, this sounds better from the caller's point of view, but it > > > would require adding a check to make sure that only one of > > > VIR_XML_PROP_NONZERO and VIR_XML_PROP_ALLOW_ZERO has been passed. > > > I'll see how clunky that looks. > > > > I meant only keeping the new one, although I must admit I completely > > missed the fact that it is used properly somewhere else. If we went > > that way then the flag might itself be a tristate enum "default, > > allow_zero, non_zero" where default is different for numbers and enums, > > but that seems even clunkier and I don't like that myself. > > Alternatively introduce a new helper e.g. virXMLPropTristateBoolDefault > which will allow also the 'default' value to be specified explicitly. > > In such case we can document the difference in the comment and don't > have to touch every other usage. Yeah, I thought of that approach too and it sounds like it would be the least terrible one. I'm going to give it a try and report back :) Thanks for all your input so far! -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 0/3] qemu: don't pause vm when creating internal snapshot
Please, disregard this series as it has a mistake. I'll send a new one. Nikolay пн, 21 мар. 2022 г. в 18:16, Nikolay Shirokovskiy : > > The is series is mostly about the first patch. The others patch are > minor cleanups. > > Maxim Nestratov (1): > qemu: don't pause vm when creating internal snapshot > > Nikolay Shirokovskiy (2): > qemu: remove excessive check in qemuSnapshotCreateActiveInternal > qemu: remove unnecessary goto in qemuSnapshotCreateActiveInternal > > src/qemu/qemu_snapshot.c | 65 +++- > 1 file changed, 18 insertions(+), 47 deletions(-) > > -- > 2.35.1 >
[PATCH v2 2/3] qemu: remove excessive check in qemuSnapshotCreateActiveInternal
After [1] it is not possible to get inactive state for domain while
communicating with QEMU (see also [2]).
Also even if it were possible the qemuProcessStopCPUs would return error
and the check would not be reached.
[1] commit 81f50cb92d16643bcd749e3ab5b404b8b7cec643
Author: Jiri Denemark
Date: Thu Feb 11 11:20:28 2016 +0100
qemu: Avoid calling qemuProcessStop without a job
[2] commit f1ea5bd506b6e00aa8bf55940b147d3c0fe7f124
Author: Ján Tomko
Date: Wed Nov 24 13:41:09 2021 +0100
qemu: turn qemuDomainObjExitMonitor into void
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_snapshot.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index cd94ddae24..540fda4854 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -305,11 +305,6 @@ qemuSnapshotCreateActiveInternal(virQEMUDriver *driver,
goto cleanup;
resume = true;
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest unexpectedly quit"));
-goto cleanup;
-}
}
if (qemuDomainObjEnterMonitorAsync(driver, vm,
--
2.35.1
[PATCH v2 1/3] qemu: don't pause vm when creating internal snapshot
From: Maxim Nestratov
The pause was introduced in [1] in ancient times when probably distro's
QEMU does not have RESUME event. Yet the event was in the upstream QEMU at
the time version according to [2].
So the event is supported since QEMU version 0.13 which is much older
then oldest currently supported 2.11.0 version and we can remove manual
pause/resume. Except for the halt case.
[1] commit 346236fea97602e9e6529c5d41a32ed26b126082
Author: Jiri Denemark
Date: Thu Feb 24 16:46:44 2011 +0100
qemu: Stop guest CPUs before creating a snapshot
commit 6ed2c484f261fd8bd217f855b9e5e5f981e63f0a
Author: Luiz Capitulino
Date: Tue Apr 27 20:35:59 2010 -0300
QMP: Introduce RESUME event
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_snapshot.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index a7901779fc..cd94ddae24 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -293,16 +293,13 @@ qemuSnapshotCreateActiveInternal(virQEMUDriver *driver,
virObjectEvent *event = NULL;
bool resume = false;
virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
+bool halt = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT);
int ret = -1;
if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
goto cleanup;
-if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
-/* savevm monitor command pauses the domain emitting an event which
- * confuses libvirt since it's not notified when qemu resumes the
- * domain. Thus we stop and start CPUs ourselves.
- */
+if (halt) {
if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE,
QEMU_ASYNC_JOB_SNAPSHOT) < 0)
goto cleanup;
@@ -329,7 +326,7 @@ qemuSnapshotCreateActiveInternal(virQEMUDriver *driver,
if (!(snapdef->cookie = (virObject *) qemuDomainSaveCookieNew(vm)))
goto cleanup;
-if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
+if (halt) {
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
--
2.35.1
[PATCH v2 0/3] qemu: don't pause VM when creating internal snapshot
Diff to v1 [1]: - fix mistake of dropping hunk that starts CPUs if snapshot was not successful. We still need it if halt upon snapshot is requested. The is series is mostly about the first patch. The others patch are minor cleanups. [1] v1 version https://listman.redhat.com/archives/libvir-list/2022-March/229437.html Maxim Nestratov (1): qemu: don't pause vm when creating internal snapshot Nikolay Shirokovskiy (2): qemu: remove excessive check in qemuSnapshotCreateActiveInternal qemu: remove unnecessary gotos in qemuSnapshotCreateActiveInternal src/qemu/qemu_snapshot.c | 24 +++- 1 file changed, 7 insertions(+), 17 deletions(-) -- 2.35.1
[PATCH v2 3/3] qemu: remove unnecessary gotos in qemuSnapshotCreateActiveInternal
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_snapshot.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 540fda4854..c23fb40849 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -297,21 +297,19 @@ qemuSnapshotCreateActiveInternal(virQEMUDriver *driver,
int ret = -1;
if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
-goto cleanup;
+return -1;
if (halt) {
if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE,
QEMU_ASYNC_JOB_SNAPSHOT) < 0)
-goto cleanup;
+return -1;
resume = true;
}
if (qemuDomainObjEnterMonitorAsync(driver, vm,
- QEMU_ASYNC_JOB_SNAPSHOT) < 0) {
-resume = false;
-goto cleanup;
-}
+ QEMU_ASYNC_JOB_SNAPSHOT) < 0)
+return -1;
ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
qemuDomainObjExitMonitor(driver, vm);
--
2.35.1
Re: [PATCH 1/4] conf: Introduce memory allocation threads
On 3/24/22 12:49, Martin Kletzander wrote: > On Thu, Mar 24, 2022 at 11:22:54AM +, Daniel P. Berrangé wrote: >> On Thu, Mar 24, 2022 at 12:15:43PM +0100, Michal Prívozník wrote: >>> On 3/24/22 10:26, Martin Kletzander wrote: >>> > On Tue, Mar 22, 2022 at 04:05:14PM +0100, Michal Privoznik wrote: >>> >> Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable of >>> > >>> > Anything wrong with the old "commit ffac16fab33b" ? >>> >>> Not specifically no. But I do it this ways so that it's visible at the >>> first sight what QEMU version was this feature introduced in. And it >>> works with 'git show' too (for those who are interested in the >>> particular commit). IOW: >>> >>> "Since its commit v5.0.0-rc0~75^2~1^2~3 QEMU is capable .." >> >> How do you generate such a reference ? It is different from >> what a simple git describe shows >> >> $ git describe --match 'v*' ffac16fab33b >> v4.2.0-1618-gffac16fab3 >> >> and I feel the git describe output is much nicer, as it actually >> includes the commit short hash still. >> > > I think Michal used --contains which clearly shows from which tag is the > commit included, but tracking down through merge commits is what makes > it look like the above. Indeed. qemu.git $ git describe --contains ffac16fab33bb42f17e47624985220c1fd864e9d v5.0.0-rc0~75^2~1^2~3 Michal
Re: [PATCH v2] conf: Introduce memory allocation threads
On Thu, Mar 24, 2022 at 12:16:01PM +0100, Michal Privoznik wrote: Since its v5.0.0 release QEMU is capable of specifying number of threads used to allocate memory. It defaults to 1, which may be too low for humongous guests with gigantic pages. In general, on QEMU cmd line level it is possible to use different number of threads per each memory-backend-* object, in practical terms it's not useful. Therefore, use to set guest wide value and let all memory devices 'inherit' it, silently. IOW, don't introduce per device knob because that would only complicate things for a little or no benefit. Signed-off-by: Michal Privoznik Reviewed-by: Martin Kletzander signature.asc Description: PGP signature
[PATCH 2/2] qemu: remove else branches after return in qemuMigrationSrcPerform()
Signed-off-by: Kristina Hanicova
---
src/qemu/qemu_migration.c | 46 +++
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index e16d579928..d8b00aca87 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5482,31 +5482,29 @@ qemuMigrationSrcPerform(virQEMUDriver *driver,
cookiein, cookieinlen,
cookieout, cookieoutlen,
flags, dname, resource, v3proto);
-} else {
-if (dconnuri) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Unexpected dconnuri parameter with
non-peer2peer migration"));
-return -1;
-}
-
-if (v3proto) {
-return qemuMigrationSrcPerformPhase(driver, conn, vm, persist_xml,
uri,
-graphicsuri,
-nmigrate_disks, migrate_disks,
-migParams,
-cookiein, cookieinlen,
-cookieout, cookieoutlen,
-flags, resource, nbdURI);
-} else {
-return qemuMigrationSrcPerformJob(driver, conn, vm, xmlin,
persist_xml, NULL,
- uri, graphicsuri, listenAddress,
- nmigrate_disks, migrate_disks,
nbdPort,
- nbdURI, migParams,
- cookiein, cookieinlen,
- cookieout, cookieoutlen, flags,
- dname, resource, v3proto);
-}
}
+if (dconnuri) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Unexpected dconnuri parameter with
non-peer2peer migration"));
+return -1;
+}
+
+if (v3proto) {
+return qemuMigrationSrcPerformPhase(driver, conn, vm, persist_xml, uri,
+graphicsuri,
+nmigrate_disks, migrate_disks,
+migParams,
+cookiein, cookieinlen,
+cookieout, cookieoutlen,
+flags, resource, nbdURI);
+}
+return qemuMigrationSrcPerformJob(driver, conn, vm, xmlin, persist_xml,
NULL,
+ uri, graphicsuri, listenAddress,
+ nmigrate_disks, migrate_disks, nbdPort,
+ nbdURI, migParams,
+ cookiein, cookieinlen,
+ cookieout, cookieoutlen, flags,
+ dname, resource, v3proto);
}
static int
--
2.35.1
[PATCH 0/2] qemu: remove else after return / goto
Kristina Hanicova (2): qemu: remove unnecessary else branches after return / goto qemu: remove else branches after return in qemuMigrationSrcPerform() src/qemu/qemu_domainjob.c | 3 +- src/qemu/qemu_migration.c | 59 +++ src/qemu/qemu_process.c | 4 +-- src/qemu/qemu_snapshot.c | 13 - 4 files changed, 38 insertions(+), 41 deletions(-) -- 2.35.1
[PATCH 1/2] qemu: remove unnecessary else branches after return / goto
I think the code looks cleaner without else branches.
Signed-off-by: Kristina Hanicova
---
src/qemu/qemu_domainjob.c | 3 +--
src/qemu/qemu_migration.c | 13 +++--
src/qemu/qemu_process.c | 4 ++--
src/qemu/qemu_snapshot.c | 13 ++---
4 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index cf1e093e22..6bf3a0ab42 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -153,8 +153,7 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
if (STREQ(phase, "none"))
return 0;
-else
-return -1;
+return -1;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f109598fb4..e16d579928 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4286,9 +4286,10 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
rc = qemuMigrationSrcWaitForCompletion(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn, waitFlags);
-if (rc == -2) {
+if (rc == -2)
goto error;
-} else if (rc == -1) {
+
+if (rc == -1) {
/* QEMU reported failed migration, nothing to cancel anymore */
cancel = false;
goto error;
@@ -4326,9 +4327,10 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
rc = qemuMigrationSrcWaitForCompletion(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn, waitFlags);
-if (rc == -2) {
+if (rc == -2)
goto error;
-} else if (rc == -1) {
+
+if (rc == -1) {
/* QEMU reported failed migration, nothing to cancel anymore */
cancel = false;
goto error;
@@ -6189,8 +6191,7 @@ qemuMigrationDstErrorInit(virQEMUDriver *driver)
driver->migrationErrors = virHashAtomicNew(qemuMigrationDstErrorFree);
if (driver->migrationErrors)
return 0;
-else
-return -1;
+return -1;
}
/**
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1ed60917ea..7a36f85d7e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8700,8 +8700,8 @@ qemuProcessRefreshBlockjobs(virQEMUDriver *driver,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
return qemuBlockJobRefreshJobs(driver, vm);
-else
-return qemuProcessRefreshLegacyBlockjobs(driver, vm);
+
+return qemuProcessRefreshLegacyBlockjobs(driver, vm);
}
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 5333730df1..212d37d3b4 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -583,13 +583,12 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm,
_("missing existing file for disk %s:
%s"),
snapdisk->name, snapdisk->src->path);
return -1;
-} else {
-if (snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("block device snapshot target '%s'
doesn't exist"),
- snapdisk->src->path);
-return -1;
-}
+}
+if (snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("block device snapshot target '%s' doesn't
exist"),
+ snapdisk->src->path);
+return -1;
}
} else {
/* at this point VIR_STORAGE_TYPE_DIR was already rejected */
--
2.35.1
Request to move my GitLab custom executor repo under the libvirt umbrella
So, we successfully enabled functional testing utilizing GitLab's custom executor feature. The code responsible for creating template images and provisioning and tearing down the VM workers currently lives under my GitLab account [1] and I think it would be better if we hosted it under the libvirt umbrella, especially since the project should serve as a uniform way of setting up custom runners for anyone in the libvirt community. If you think this is not a good idea, then please speak up and raise your concerns. Note there's still quite some work to be done with the project in general, but I'd be happy for any feedback in terms of what needs to be done prior moving the project to the libvirt namespace. Regards, Erik [1] https://gitlab.com/eskultety/libvirt-gitlab-executor
Re: Request to move my GitLab custom executor repo under the libvirt umbrella
On Thu, Mar 24, 2022 at 04:07:36PM +0100, Erik Skultety wrote: > So, we successfully enabled functional testing utilizing GitLab's custom > executor feature. The code responsible for creating template images and > provisioning and tearing down the VM workers currently lives under my GitLab > account [1] and I think it would be better if we hosted it under the libvirt > umbrella, especially since the project should serve as a uniform way of > setting > up custom runners for anyone in the libvirt community. > If you think this is not a good idea, then please speak up and raise your > concerns. > Note there's still quite some work to be done with the project in general, but > I'd be happy for any feedback in terms of what needs to be done prior moving > the project to the libvirt namespace. It is a core part of our new CI infrastructure. We don't want single points of administrative failure, so it is definitely desirable to have it under the libvirt umbrella. Even if you continue todo 99% of the work on it, we're prepared should other people want/need to engage, or eventually take over primary responsibility. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH 3/5] lxc: use virDomainJob enum instead of virLXCDomainJob
Signed-off-by: Kristina Hanicova
---
src/hypervisor/domain_job.h | 4 ++--
src/lxc/lxc_domain.c| 25
src/lxc/lxc_domain.h| 19 +++
src/lxc/lxc_driver.c| 46 ++---
4 files changed, 37 insertions(+), 57 deletions(-)
diff --git a/src/hypervisor/domain_job.h b/src/hypervisor/domain_job.h
index 4f165f730d..db8b8b1390 100644
--- a/src/hypervisor/domain_job.h
+++ b/src/hypervisor/domain_job.h
@@ -21,8 +21,8 @@
/* Only 1 job is allowed at any time
- * A job includes *all* monitor commands, even those just querying
- * information, not merely actions */
+ * A job includes *all* monitor commands / hypervisor.so api,
+ * even those just querying information, not merely actions */
typedef enum {
VIR_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive) conditions */
VIR_JOB_QUERY, /* Doesn't change any state */
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 85795d1805..fae56b35fb 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -31,17 +31,10 @@
#include "virsystemd.h"
#include "virinitctl.h"
#include "domain_driver.h"
+#include "domain_job.h"
#define VIR_FROM_THIS VIR_FROM_LXC
-VIR_ENUM_IMPL(virLXCDomainJob,
- LXC_JOB_LAST,
- "none",
- "query",
- "destroy",
- "modify",
-);
-
VIR_LOG_INIT("lxc.lxc_domain");
static int
@@ -60,7 +53,7 @@ virLXCDomainObjResetJob(virLXCDomainObjPrivate *priv)
{
struct virLXCDomainJobObj *job = &priv->job;
-job->active = LXC_JOB_NONE;
+job->active = VIR_JOB_NONE;
job->owner = 0;
}
@@ -85,7 +78,7 @@ virLXCDomainObjFreeJob(virLXCDomainObjPrivate *priv)
int
virLXCDomainObjBeginJob(virLXCDriver *driver G_GNUC_UNUSED,
virDomainObj *obj,
- enum virLXCDomainJob job)
+ virDomainJob job)
{
virLXCDomainObjPrivate *priv = obj->privateData;
unsigned long long now;
@@ -97,14 +90,14 @@ virLXCDomainObjBeginJob(virLXCDriver *driver G_GNUC_UNUSED,
while (priv->job.active) {
VIR_DEBUG("Wait normal job condition for starting job: %s",
- virLXCDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
goto error;
}
virLXCDomainObjResetJob(priv);
-VIR_DEBUG("Starting job: %s", virLXCDomainJobTypeToString(job));
+VIR_DEBUG("Starting job: %s", virDomainJobTypeToString(job));
priv->job.active = job;
priv->job.owner = virThreadSelfID();
@@ -113,9 +106,9 @@ virLXCDomainObjBeginJob(virLXCDriver *driver G_GNUC_UNUSED,
error:
VIR_WARN("Cannot start job (%s) for domain %s;"
" current job is (%s) owned by (%d)",
- virLXCDomainJobTypeToString(job),
+ virDomainJobTypeToString(job),
obj->def->name,
- virLXCDomainJobTypeToString(priv->job.active),
+ virDomainJobTypeToString(priv->job.active),
priv->job.owner);
if (errno == ETIMEDOUT)
@@ -139,10 +132,10 @@ virLXCDomainObjEndJob(virLXCDriver *driver G_GNUC_UNUSED,
virDomainObj *obj)
{
virLXCDomainObjPrivate *priv = obj->privateData;
-enum virLXCDomainJob job = priv->job.active;
+virDomainJob job = priv->job.active;
VIR_DEBUG("Stopping job: %s",
- virLXCDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
virLXCDomainObjResetJob(priv);
virCondSignal(&priv->job.cond);
diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
index 766837bdf1..1c4cb8c14a 100644
--- a/src/lxc/lxc_domain.h
+++ b/src/lxc/lxc_domain.h
@@ -25,6 +25,7 @@
#include "lxc_conf.h"
#include "lxc_monitor.h"
#include "virenum.h"
+#include "domain_job.h"
typedef enum {
@@ -53,23 +54,9 @@ struct _lxcDomainDef {
};
-/* Only 1 job is allowed at any time
- * A job includes *all* lxc.so api, even those just querying
- * information, not merely actions */
-
-enum virLXCDomainJob {
-LXC_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive)
conditions */
-LXC_JOB_QUERY, /* Doesn't change any state */
-LXC_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */
-LXC_JOB_MODIFY,/* May change state */
-LXC_JOB_LAST
-};
-VIR_ENUM_DECL(virLXCDomainJob);
-
-
struct virLXCDomainJobObj {
virCond cond; /* Use to coordinate jobs */
-enum virLXCDomainJob active;/* Currently running job */
+virDomainJob active;/* Currently running job */
int owner; /* Thread which set current job */
};
@@ -96,7 +83,7 @@ extern virDomainDefParserConfig
virLXCDriverDomainDefParserConfig;
int
virLXCDomainObjBeginJob(virLXCDriver *driver,
virDomainObj *obj,
-
[PATCH 4/5] libxl: use virDomainJob enum instead of libxlDomainJob
Signed-off-by: Kristina Hanicova
---
src/libxl/libxl_domain.c| 28 --
src/libxl/libxl_domain.h| 17 ++---
src/libxl/libxl_driver.c| 48 ++---
src/libxl/libxl_migration.c | 6 ++---
4 files changed, 39 insertions(+), 60 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2501f6b848..dbe44f4ffc 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -44,14 +44,6 @@
VIR_LOG_INIT("libxl.libxl_domain");
-VIR_ENUM_IMPL(libxlDomainJob,
- LIBXL_JOB_LAST,
- "none",
- "query",
- "destroy",
- "modify",
-);
-
static int
libxlDomainObjInitJob(libxlDomainObjPrivate *priv)
@@ -71,7 +63,7 @@ libxlDomainObjResetJob(libxlDomainObjPrivate *priv)
{
struct libxlDomainJobObj *job = &priv->job;
-job->active = LIBXL_JOB_NONE;
+job->active = VIR_JOB_NONE;
job->owner = 0;
}
@@ -97,7 +89,7 @@ libxlDomainObjFreeJob(libxlDomainObjPrivate *priv)
int
libxlDomainObjBeginJob(libxlDriverPrivate *driver G_GNUC_UNUSED,
virDomainObj *obj,
- enum libxlDomainJob job)
+ virDomainJob job)
{
libxlDomainObjPrivate *priv = obj->privateData;
unsigned long long now;
@@ -109,14 +101,14 @@ libxlDomainObjBeginJob(libxlDriverPrivate *driver
G_GNUC_UNUSED,
while (priv->job.active) {
VIR_DEBUG("Wait normal job condition for starting job: %s",
- libxlDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0)
goto error;
}
libxlDomainObjResetJob(priv);
-VIR_DEBUG("Starting job: %s", libxlDomainJobTypeToString(job));
+VIR_DEBUG("Starting job: %s", virDomainJobTypeToString(job));
priv->job.active = job;
priv->job.owner = virThreadSelfID();
priv->job.current->started = now;
@@ -127,9 +119,9 @@ libxlDomainObjBeginJob(libxlDriverPrivate *driver
G_GNUC_UNUSED,
error:
VIR_WARN("Cannot start job (%s) for domain %s;"
" current job is (%s) owned by (%d)",
- libxlDomainJobTypeToString(job),
+ virDomainJobTypeToString(job),
obj->def->name,
- libxlDomainJobTypeToString(priv->job.active),
+ virDomainJobTypeToString(priv->job.active),
priv->job.owner);
if (errno == ETIMEDOUT)
@@ -157,10 +149,10 @@ libxlDomainObjEndJob(libxlDriverPrivate *driver
G_GNUC_UNUSED,
virDomainObj *obj)
{
libxlDomainObjPrivate *priv = obj->privateData;
-enum libxlDomainJob job = priv->job.active;
+virDomainJob job = priv->job.active;
VIR_DEBUG("Stopping job: %s",
- libxlDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
libxlDomainObjResetJob(priv);
virCondSignal(&priv->job.cond);
@@ -510,7 +502,7 @@ libxlDomainShutdownThread(void *opaque)
goto cleanup;
}
-if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+if (libxlDomainObjBeginJob(driver, vm, VIR_JOB_MODIFY) < 0)
goto cleanup;
if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) {
@@ -639,7 +631,7 @@ libxlDomainDeathThread(void *opaque)
goto cleanup;
}
-if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+if (libxlDomainObjBeginJob(driver, vm, VIR_JOB_MODIFY) < 0)
goto cleanup;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_DESTROYED);
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 157f480b93..aa15e0352f 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -28,23 +28,10 @@
#include "virenum.h"
#include "domain_job.h"
-/* Only 1 job is allowed at any time
- * A job includes *all* libxl.so api, even those just querying
- * information, not merely actions */
-enum libxlDomainJob {
-LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive)
conditions */
-LIBXL_JOB_QUERY, /* Doesn't change any state */
-LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */
-LIBXL_JOB_MODIFY,/* May change state */
-
-LIBXL_JOB_LAST
-};
-VIR_ENUM_DECL(libxlDomainJob);
-
struct libxlDomainJobObj {
virCond cond; /* Use to coordinate jobs */
-enum libxlDomainJob active; /* Currently running job */
+virDomainJob active;/* Currently running job */
int owner; /* Thread which set current job */
virDomainJobData *current;/* Statistics for the current job */
};
@@ -76,7 +63,7 @@ libxlDomainObjPrivateInitCtx(virDomainObj *vm);
int
libxlDomainObjBeginJob(libxlDriverPrivate *driver,
virDomainObj *obj,
- enum libxlDomainJob job)
+
[PATCH 5/5] ch: use virDomainJob enum instead of virCHDomainJob
Signed-off-by: Kristina Hanicova
---
src/ch/ch_domain.c | 24
src/ch/ch_domain.h | 18 +++---
src/ch/ch_driver.c | 20 ++--
3 files changed, 21 insertions(+), 41 deletions(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 25f581c1c3..bb489a74e3 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -31,14 +31,6 @@
#define VIR_FROM_THIS VIR_FROM_CH
-VIR_ENUM_IMPL(virCHDomainJob,
- CH_JOB_LAST,
- "none",
- "query",
- "destroy",
- "modify",
-);
-
VIR_LOG_INIT("ch.ch_domain");
static int
@@ -57,7 +49,7 @@ virCHDomainObjResetJob(virCHDomainObjPrivate *priv)
{
struct virCHDomainJobObj *job = &priv->job;
-job->active = CH_JOB_NONE;
+job->active = VIR_JOB_NONE;
job->owner = 0;
}
@@ -77,7 +69,7 @@ virCHDomainObjFreeJob(virCHDomainObjPrivate *priv)
* Successful calls must be followed by EndJob eventually.
*/
int
-virCHDomainObjBeginJob(virDomainObj *obj, enum virCHDomainJob job)
+virCHDomainObjBeginJob(virDomainObj *obj, virDomainJob job)
{
virCHDomainObjPrivate *priv = obj->privateData;
unsigned long long now;
@@ -89,13 +81,13 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum
virCHDomainJob job)
while (priv->job.active) {
VIR_DEBUG("Wait normal job condition for starting job: %s",
- virCHDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) {
VIR_WARN("Cannot start job (%s) for domain %s;"
" current job is (%s) owned by (%d)",
- virCHDomainJobTypeToString(job),
+ virDomainJobTypeToString(job),
obj->def->name,
- virCHDomainJobTypeToString(priv->job.active),
+ virDomainJobTypeToString(priv->job.active),
priv->job.owner);
if (errno == ETIMEDOUT)
@@ -110,7 +102,7 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum
virCHDomainJob job)
virCHDomainObjResetJob(priv);
-VIR_DEBUG("Starting job: %s", virCHDomainJobTypeToString(job));
+VIR_DEBUG("Starting job: %s", virDomainJobTypeToString(job));
priv->job.active = job;
priv->job.owner = virThreadSelfID();
@@ -127,10 +119,10 @@ void
virCHDomainObjEndJob(virDomainObj *obj)
{
virCHDomainObjPrivate *priv = obj->privateData;
-enum virCHDomainJob job = priv->job.active;
+virDomainJob job = priv->job.active;
VIR_DEBUG("Stopping job: %s",
- virCHDomainJobTypeToString(job));
+ virDomainJobTypeToString(job));
virCHDomainObjResetJob(priv);
virCondSignal(&priv->job.cond);
diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
index 11a20a874a..f75a08ec87 100644
--- a/src/ch/ch_domain.h
+++ b/src/ch/ch_domain.h
@@ -24,27 +24,15 @@
#include "ch_monitor.h"
#include "virchrdev.h"
#include "vircgroup.h"
+#include "domain_job.h"
/* Give up waiting for mutex after 30 seconds */
#define CH_JOB_WAIT_TIME (1000ull * 30)
-/* Only 1 job is allowed at any time
- * A job includes *all* ch.so api, even those just querying
- * information, not merely actions */
-
-enum virCHDomainJob {
-CH_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive)
conditions */
-CH_JOB_QUERY, /* Doesn't change any state */
-CH_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */
-CH_JOB_MODIFY,/* May change state */
-CH_JOB_LAST
-};
-VIR_ENUM_DECL(virCHDomainJob);
-
struct virCHDomainJobObj {
virCond cond; /* Use to coordinate jobs */
-enum virCHDomainJob active;/* Currently running job */
+virDomainJob active;/* Currently running job */
int owner; /* Thread which set current job */
};
@@ -82,7 +70,7 @@ extern virDomainXMLPrivateDataCallbacks
virCHDriverPrivateDataCallbacks;
extern virDomainDefParserConfig virCHDriverDomainDefParserConfig;
int
-virCHDomainObjBeginJob(virDomainObj *obj, enum virCHDomainJob job)
+virCHDomainObjBeginJob(virDomainObj *obj, virDomainJob job)
G_GNUC_WARN_UNUSED_RESULT;
void
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index ac9298c0b5..2fe7aba9d0 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -224,7 +224,7 @@ chDomainCreateXML(virConnectPtr conn,
NULL)))
goto cleanup;
-if (virCHDomainObjBeginJob(vm, CH_JOB_MODIFY) < 0)
+if (virCHDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
goto cleanup;
if (virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED) < 0)
@@ -258,7 +258,7 @@ chDomainCreateWithFlags(virDomainPtr dom, unsigned int
flags)
if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
-if (virCHDomainObjBeginJob(vm, CH_
[PATCH 2/5] qemu: move macros QEMU_X into hypervisor as VIR_X
It makes sense to have these in the same file as the definitions
of enums.
Signed-off-by: Kristina Hanicova
---
src/hypervisor/domain_job.h | 12
src/qemu/qemu_backup.c | 2 +-
src/qemu/qemu_domainjob.c | 4 ++--
src/qemu/qemu_domainjob.h | 11 ---
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_process.c | 4 ++--
src/qemu/qemu_snapshot.c| 4 ++--
7 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/src/hypervisor/domain_job.h b/src/hypervisor/domain_job.h
index b9d1107580..4f165f730d 100644
--- a/src/hypervisor/domain_job.h
+++ b/src/hypervisor/domain_job.h
@@ -8,6 +8,18 @@
#include "internal.h"
#include "virenum.h"
+#define JOB_MASK(job) (job == 0 ? 0 : 1 << (job - 1))
+#define VIR_JOB_DEFAULT_MASK \
+(JOB_MASK(VIR_JOB_QUERY) | \
+ JOB_MASK(VIR_JOB_DESTROY) | \
+ JOB_MASK(VIR_JOB_ABORT))
+
+/* Jobs which have to be tracked in domain state XML. */
+#define VIR_DOMAIN_TRACK_JOBS \
+(JOB_MASK(VIR_JOB_DESTROY) | \
+ JOB_MASK(VIR_JOB_ASYNC))
+
+
/* Only 1 job is allowed at any time
* A job includes *all* monitor commands, even those just querying
* information, not merely actions */
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 5d24155628..427c090dd8 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -795,7 +795,7 @@ qemuBackupBegin(virDomainObj *vm,
VIR_DOMAIN_JOB_OPERATION_BACKUP, flags) < 0)
return -1;
-qemuDomainObjSetAsyncJobMask(vm, (QEMU_JOB_DEFAULT_MASK |
+qemuDomainObjSetAsyncJobMask(vm, (VIR_JOB_DEFAULT_MASK |
JOB_MASK(VIR_JOB_SUSPEND) |
JOB_MASK(VIR_JOB_MODIFY)));
qemuDomainJobSetStatsType(priv->job.current,
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 71876fe6a3..ab542865ca 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -204,7 +204,7 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObj *job)
g_clear_pointer(&job->asyncOwnerAPI, g_free);
job->asyncStarted = 0;
job->phase = 0;
-job->mask = QEMU_JOB_DEFAULT_MASK;
+job->mask = VIR_JOB_DEFAULT_MASK;
job->abortJob = false;
VIR_FREE(job->error);
g_clear_pointer(&job->current, virDomainJobDataFree);
@@ -256,7 +256,7 @@ qemuDomainObjClearJob(qemuDomainJobObj *job)
bool
qemuDomainTrackJob(virDomainJob job)
{
-return (QEMU_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0;
+return (VIR_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0;
}
diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h
index 6520b42c80..faf47105a3 100644
--- a/src/qemu/qemu_domainjob.h
+++ b/src/qemu/qemu_domainjob.h
@@ -22,17 +22,6 @@
#include "qemu_monitor.h"
#include "domain_job.h"
-#define JOB_MASK(job) (job == 0 ? 0 : 1 << (job - 1))
-#define QEMU_JOB_DEFAULT_MASK \
-(JOB_MASK(VIR_JOB_QUERY) | \
- JOB_MASK(VIR_JOB_DESTROY) | \
- JOB_MASK(VIR_JOB_ABORT))
-
-/* Jobs which have to be tracked in domain state XML. */
-#define QEMU_DOMAIN_TRACK_JOBS \
-(JOB_MASK(VIR_JOB_DESTROY) | \
- JOB_MASK(VIR_JOB_ASYNC))
-
typedef enum {
QEMU_DOMAIN_JOB_STATS_TYPE_NONE = 0,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 43ffe2357a..b591514533 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -6105,7 +6105,7 @@ qemuMigrationJobStart(virQEMUDriver *driver,
mask = VIR_JOB_NONE;
} else {
op = VIR_DOMAIN_JOB_OPERATION_MIGRATION_OUT;
-mask = QEMU_JOB_DEFAULT_MASK |
+mask = VIR_JOB_DEFAULT_MASK |
JOB_MASK(VIR_JOB_SUSPEND) |
JOB_MASK(VIR_JOB_MIGRATION_OP);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 189e4671d1..bf514bc963 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3657,7 +3657,7 @@ qemuProcessRecoverJob(virQEMUDriver *driver,
priv->job.asyncOwnerAPI = g_strdup(virThreadJobGet());
priv->job.asyncStarted = now;
-qemuDomainObjSetAsyncJobMask(vm, (QEMU_JOB_DEFAULT_MASK |
+qemuDomainObjSetAsyncJobMask(vm, (VIR_JOB_DEFAULT_MASK |
JOB_MASK(VIR_JOB_SUSPEND) |
JOB_MASK(VIR_JOB_MODIFY)));
@@ -3682,7 +3682,7 @@ qemuProcessRecoverJob(virQEMUDriver *driver,
return -1;
/* In case any special handling is added for job type that has been ignored
- * before, QEMU_DOMAIN_TRACK_JOBS (from qemu_domain.h) needs to be updated
+ * before, VIR_DOMAIN_TRACK_JOBS (from qemu_domain.h) needs to be updated
* for the job to be properly tracked in domain state XML.
*/
switch (job->active) {
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 185fcb04a2..4a05cddc54 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1433,7 +1433,7 @@ qemuSn
[PATCH 0/5] Generalize job enums across all hypervisors
Kristina Hanicova (5): move jobs enums QEMU_X into hypervisor as VIR_X qemu: move macros QEMU_X into hypervisor as VIR_X lxc: use virDomainJob enum instead of virLXCDomainJob libxl: use virDomainJob enum instead of libxlDomainJob ch: use virDomainJob enum instead of virCHDomainJob src/ch/ch_domain.c | 24 +-- src/ch/ch_domain.h | 18 +- src/ch/ch_driver.c | 20 +- src/hypervisor/domain_job.c | 32 +++ src/hypervisor/domain_job.h | 64 ++ src/hypervisor/meson.build | 1 + src/libvirt_private.syms | 5 + src/libxl/libxl_domain.c | 29 +-- src/libxl/libxl_domain.h | 17 +- src/libxl/libxl_driver.c | 48 ++--- src/libxl/libxl_migration.c | 6 +- src/lxc/lxc_domain.c | 25 +-- src/lxc/lxc_domain.h | 19 +- src/lxc/lxc_driver.c | 46 ++--- src/qemu/MIGRATION.txt | 6 +- src/qemu/THREADS.txt | 16 +- src/qemu/qemu_backup.c | 24 +-- src/qemu/qemu_block.c| 20 +- src/qemu/qemu_block.h| 12 +- src/qemu/qemu_blockjob.c | 32 +-- src/qemu/qemu_checkpoint.c | 18 +- src/qemu/qemu_domain.c | 26 +-- src/qemu/qemu_domain.h | 4 +- src/qemu/qemu_domainjob.c| 240 ++ src/qemu/qemu_domainjob.h| 86 ++-- src/qemu/qemu_driver.c | 332 +++ src/qemu/qemu_hotplug.c | 50 ++--- src/qemu/qemu_hotplug.h | 10 +- src/qemu/qemu_migration.c| 220 ++-- src/qemu/qemu_migration.h| 8 +- src/qemu/qemu_migration_params.c | 4 +- src/qemu/qemu_process.c | 192 +- src/qemu/qemu_process.h | 22 +- src/qemu/qemu_saveimage.c| 4 +- src/qemu/qemu_saveimage.h| 4 +- src/qemu/qemu_snapshot.c | 60 +++--- src/qemu/qemu_snapshot.h | 2 +- 37 files changed, 849 insertions(+), 897 deletions(-) -- 2.35.1
[PATCH] error message in virDomainDiskDefIotuneParse shifted to virDomainDefPostParseCheck
From: Moteen Shah
domain_conf.c: all the option collision total and... error messages in
virDomainDiskDefIotuneParse shifted to new function virDomainDefPostParseCheck
---
src/conf/domain_conf.c | 86 ++
1 file changed, 45 insertions(+), 41 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 153954a0b0..f2480f37f6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6229,6 +6229,50 @@ virDomainDefPostParseCheckFailure(virDomainDef *def,
return 0;
}
+static int
+virDomainDefPostParseCheck(virDomainDiskDef *def)
+{
+if ((def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.read_bytes_sec) ||
+(def->blkdeviotune.total_bytes_sec &&
+ def->blkdeviotune.write_bytes_sec)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec "
+ "cannot be set at the same time"));
+return -1;
+}
+
+if ((def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.read_iops_sec) ||
+(def->blkdeviotune.total_iops_sec &&
+ def->blkdeviotune.write_iops_sec)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec "
+ "cannot be set at the same time"));
+return -1;
+}
+
+if ((def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.read_bytes_sec_max) ||
+(def->blkdeviotune.total_bytes_sec_max &&
+ def->blkdeviotune.write_bytes_sec_max)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write bytes_sec_max "
+ "cannot be set at the same time"));
+return -1;
+}
+
+if ((def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.read_iops_sec_max) ||
+(def->blkdeviotune.total_iops_sec_max &&
+ def->blkdeviotune.write_iops_sec_max)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("total and read/write iops_sec_max "
+ "cannot be set at the same time"));
+return -1;
+}
+return 0;
+}
int
virDomainDefPostParse(virDomainDef *def,
@@ -8784,47 +8828,7 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
def->blkdeviotune.group_name =
virXPathString("string(./iotune/group_name)", ctxt);
-if ((def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.read_bytes_sec) ||
-(def->blkdeviotune.total_bytes_sec &&
- def->blkdeviotune.write_bytes_sec)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec "
- "cannot be set at the same time"));
-return -1;
-}
-
-if ((def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.read_iops_sec) ||
-(def->blkdeviotune.total_iops_sec &&
- def->blkdeviotune.write_iops_sec)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec "
- "cannot be set at the same time"));
-return -1;
-}
-
-if ((def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.read_bytes_sec_max) ||
-(def->blkdeviotune.total_bytes_sec_max &&
- def->blkdeviotune.write_bytes_sec_max)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write bytes_sec_max "
- "cannot be set at the same time"));
-return -1;
-}
-
-if ((def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.read_iops_sec_max) ||
-(def->blkdeviotune.total_iops_sec_max &&
- def->blkdeviotune.write_iops_sec_max)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("total and read/write iops_sec_max "
- "cannot be set at the same time"));
-return -1;
-}
-
-return 0;
+return virDomainDefPostParseCheck(def);
}
#undef PARSE_IOTUNE
--
2.35.1
Re: [PATCH v2 16/19] Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath
On Tue, May 04, 2021 at 01:40:10PM +0200, Kristina Hanicova wrote:
> -} else if (virXMLNodeNameEqual(cur, "playback")) {
> -int compressionVal;
> -g_autofree char *compression = virXMLPropString(cur,
> "compression");
> -
> -if (!compression) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("spice playback missing compression"));
> -return -1;
> -}
> -
> -if ((compressionVal =
> - virTristateSwitchTypeFromString(compression)) <= 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("unknown spice playback compression"));
> -return -1;
> -}
> -
> -def->data.spice.playback = compressionVal;
> +if ((playback_compression =
> virXPathString("string(./playback/@compression)", ctxt))) {
> +if ((value = virTristateSwitchTypeFromString(playback_compression))
> <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unknown spice playback compression"));
> +return -1;
> +}
> +def->data.spice.playback = value;
> +}
Apologies for the thread necromancy :)
If I'm reading the change above correctly, then the presence of the
compression property of the element has gone from being
required, with an error being raised and the function terminating
early if it's not there, to being parsed if found and ignored
otherwise.
Tim later restored the original behavior in
commit bb94b3d28db909d43d83b3f2ab73aa3f881b5c95
Author: Tim Wiederhake
Date: Wed May 5 12:55:48 2021 +0200
virDomainGraphicsDefParseXMLSpice: Use virXMLProp*
Signed-off-by: Tim Wiederhake
Reviewed-by: Michal Privoznik
with the hunk
> -if ((playback_compression =
> virXPathString("string(./playback/@compression)", ctxt))) {
> -if ((value = virTristateSwitchTypeFromString(playback_compression))
> <= 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("unknown spice playback compression"));
> +if ((cur = virXPathNode("./playback", ctxt))) {
> +if (virXMLPropTristateSwitch(cur, "compression",
> + VIR_XML_PROP_REQUIRED,
> + &def->data.spice.playback) < 0)
> return -1;
> -}
> -def->data.spice.playback = value;
> }
and specifically the VIR_XML_PROP_REQUIRED bit, but I can't help
wondering if there are other cases where switching to
virXPathString() in the way seen here might have introduced undesired
changes in behavior.
Thoughts?
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 16/19] Refactoring virDomainGraphicsDefParseXMLSpice() to use XPath
On 3/24/22 18:02, Andrea Bolognani wrote: > > but I can't help > wondering if there are other cases where switching to > virXPathString() in the way seen here might have introduced undesired > changes in behavior. > > Thoughts? > Unless you want to audit every virXPath*() call with its history, I say we're good as we didn't get any bug reports. It is unfortunate, but the master was broken only for one day. Michal
[libvirt PATCH v2 0/4] Enable copy/paste for vnc displays
This patch series enables support for the qemu-vdagent character device which enables copy/paste support between guest and client when using vnc graphics. The guest must be configured with something like the following: Copy/paste sync requires a vnc client that has support for copy/paste commands. Currently virt-viewer does not work, but the version of tigervnc provided by fedora (executable name 'vncviewer') does work. More details about this device on Gerd's blog: https://www.kraxel.org/blog/2021/05/qemu-cut-paste/ For now I have left the target to be configurable to match the spicevmc channel, although Marc-Andre has suggested to simply hard-code it to the virtio name com.redhat.spice.0 Changes in v2: - change xml syntax to use and sub-elements of defined in the same way as they are for the spice display. - fix a build failure when apparmor was enabled - Add another test for when features are turned off Jonathon Jongsma (4): qemu: add capability for qemu-vdagent chardev Rename virDomainGraphicsSpiceMouseMode to virDomainMouseMode conf: add qemu-vdagent channel qemu: add support for qemu-vdagent channel docs/formatdomain.rst | 23 ++ src/conf/domain_conf.c| 70 +-- src/conf/domain_conf.h| 24 --- src/conf/domain_validate.c| 1 + src/conf/schemas/domaincommon.rng | 51 +- src/libvirt_private.syms | 4 +- src/libxl/libxl_conf.c| 8 +-- src/libxl/xen_xl.c| 16 ++--- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 32 +++-- src/qemu/qemu_monitor_json.c | 27 +++ src/qemu/qemu_process.c | 1 + src/qemu/qemu_validate.c | 9 +++ src/security/security_apparmor.c | 2 + src/security/security_dac.c | 2 + .../caps_6.1.0.x86_64.xml | 1 + .../caps_6.2.0.aarch64.xml| 1 + .../caps_6.2.0.x86_64.xml | 1 + .../caps_7.0.0.x86_64.xml | 1 + ...l-qemu-vdagent-features.x86_64-latest.args | 41 +++ .../channel-qemu-vdagent-features.xml | 37 ++ .../channel-qemu-vdagent.x86_64-latest.args | 41 +++ .../qemuxml2argvdata/channel-qemu-vdagent.xml | 37 ++ tests/qemuxml2argvtest.c | 2 + ...el-qemu-vdagent-features.x86_64-latest.xml | 58 +++ .../channel-qemu-vdagent.x86_64-latest.xml| 58 +++ tests/qemuxml2xmltest.c | 2 + tests/testutilsqemu.c | 1 + 29 files changed, 500 insertions(+), 54 deletions(-) create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent-features.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent-features.xml create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent.xml create mode 100644 tests/qemuxml2xmloutdata/channel-qemu-vdagent-features.x86_64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/channel-qemu-vdagent.x86_64-latest.xml -- 2.35.1
[libvirt PATCH v2 2/4] Rename virDomainGraphicsSpiceMouseMode to virDomainMouseMode
This enumeration will be useful for vnc with the upcoming qemu-vdagent
device so make the name more generic.
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 8
src/conf/domain_conf.h | 14 +++---
src/libvirt_private.syms | 4 ++--
src/libxl/libxl_conf.c | 8
src/libxl/xen_xl.c | 16 +++-
src/qemu/qemu_command.c | 10 +-
6 files changed, 29 insertions(+), 31 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 731139f80f..6f9954638c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -995,8 +995,8 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceZlibCompression,
"always",
);
-VIR_ENUM_IMPL(virDomainGraphicsSpiceMouseMode,
- VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST,
+VIR_ENUM_IMPL(virDomainMouseMode,
+ VIR_DOMAIN_MOUSE_MODE_LAST,
"default",
"server",
"client",
@@ -12727,7 +12727,7 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDef
*def,
if ((cur = virXPathNode("./mouse", ctxt))) {
if (virXMLPropEnum(cur, "mode",
- virDomainGraphicsSpiceMouseModeTypeFromString,
+ virDomainMouseModeTypeFromString,
VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
&def->data.spice.mousemode) < 0)
return -1;
@@ -26616,7 +26616,7 @@ virDomainGraphicsDefFormat(virBuffer *buf,
virDomainGraphicsSpiceStreamingModeTypeToString(def->data.spice.streaming));
if (def->data.spice.mousemode)
virBufferAsprintf(buf, "\n",
-
virDomainGraphicsSpiceMouseModeTypeToString(def->data.spice.mousemode));
+
virDomainMouseModeTypeToString(def->data.spice.mousemode));
if (def->data.spice.copypaste)
virBufferAsprintf(buf, "\n",
virTristateBoolTypeToString(def->data.spice.copypaste));
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 49c964e6e1..2b00099431 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1844,12 +1844,12 @@ typedef enum {
} virDomainGraphicsSpiceZlibCompression;
typedef enum {
-VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT = 0,
-VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER,
-VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT,
+VIR_DOMAIN_MOUSE_MODE_DEFAULT = 0,
+VIR_DOMAIN_MOUSE_MODE_SERVER,
+VIR_DOMAIN_MOUSE_MODE_CLIENT,
-VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST
-} virDomainGraphicsSpiceMouseMode;
+VIR_DOMAIN_MOUSE_MODE_LAST
+} virDomainMouseMode;
typedef enum {
VIR_DOMAIN_GRAPHICS_SPICE_STREAMING_MODE_DEFAULT = 0,
@@ -1927,7 +1927,7 @@ struct _virDomainGraphicsDef {
int tlsPort;
bool portReserved;
bool tlsPortReserved;
-virDomainGraphicsSpiceMouseMode mousemode;
+virDomainMouseMode mousemode;
char *keymap;
virDomainGraphicsAuthDef auth;
bool autoport;
@@ -3990,7 +3990,7 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceImageCompression);
VIR_ENUM_DECL(virDomainGraphicsSpiceJpegCompression);
VIR_ENUM_DECL(virDomainGraphicsSpiceZlibCompression);
VIR_ENUM_DECL(virDomainGraphicsSpiceStreamingMode);
-VIR_ENUM_DECL(virDomainGraphicsSpiceMouseMode);
+VIR_ENUM_DECL(virDomainMouseMode);
VIR_ENUM_DECL(virDomainGraphicsVNCSharePolicy);
VIR_ENUM_DECL(virDomainHyperv);
VIR_ENUM_DECL(virDomainKVM);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 03697d81a8..400adf14b5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -444,8 +444,6 @@ virDomainGraphicsSpiceImageCompressionTypeFromString;
virDomainGraphicsSpiceImageCompressionTypeToString;
virDomainGraphicsSpiceJpegCompressionTypeFromString;
virDomainGraphicsSpiceJpegCompressionTypeToString;
-virDomainGraphicsSpiceMouseModeTypeFromString;
-virDomainGraphicsSpiceMouseModeTypeToString;
virDomainGraphicsSpiceStreamingModeTypeFromString;
virDomainGraphicsSpiceStreamingModeTypeToString;
virDomainGraphicsSpiceZlibCompressionTypeFromString;
@@ -519,6 +517,8 @@ virDomainMemoryModelTypeToString;
virDomainMemoryRemove;
virDomainMemorySourceTypeFromString;
virDomainMemorySourceTypeToString;
+virDomainMouseModeTypeFromString;
+virDomainMouseModeTypeToString;
virDomainNetAllocateActualDevice;
virDomainNetAppendIPAddress;
virDomainNetARPInterfaces;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a0fc51c74a..0b4662e6dc 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1645,14 +1645,14 @@ libxlMakeBuildInfoVfb(virPortAllocatorRange
*graphicsports,
switch (l_vfb->data.spice.mousemode) {
/* client mouse mode is default in xl.cfg */
-case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT:
-case VIR_DOMAIN_GRAPHICS_SPICE_MOU
[libvirt PATCH v2 3/4] conf: add qemu-vdagent channel
Add the ability to configure a qemu-vdagent in guest domains. This device is similar to the spice vdagent channel except that qemu handles the spice-vdagent protocol messages itself rather than routing them over a spice protocol channel. The qemu-vdagent device has two notable configuration options which determine whether qemu will handle particular vdagent features: 'clipboard' and 'mouse'. The 'clipboard' option allows qemu to synchronize its internal clipboard manager with the guest clipboard, which enables client<->guest clipboard synchronization for non-spice guests such as vnc. The 'mouse' option allows absolute mouse positioning to be sent over the vdagent channel rather than using a usb or virtio tablet device. Sample configuration: Signed-off-by: Jonathon Jongsma --- docs/formatdomain.rst | 23 src/conf/domain_conf.c| 62 ++- src/conf/domain_conf.h| 20 ++ src/conf/domain_validate.c| 1 + src/conf/schemas/domaincommon.rng | 51 - src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_monitor_json.c | 1 + src/qemu/qemu_process.c | 1 + src/qemu/qemu_validate.c | 1 + src/security/security_apparmor.c | 2 + src/security/security_dac.c | 2 + tests/testutilsqemu.c | 1 + 12 files changed, 140 insertions(+), 28 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e492532004..75339c2fda 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6662,6 +6662,29 @@ types have different ``target`` attributes. ``name='com.redhat.spice.0'``. The optional ``address`` element can tie the channel to a particular ``type='virtio-serial'`` controller. :since:`Since 0.8.8` +``qemu-vdagent`` + Paravirtualized qemu vdagent channel. This channel implements the SPICE + vdagent protocol, but is handled internally by qemu and therefore does not + require a SPICE graphics device. Like the spicevmc channel, the ``target`` + element must be present, with attribute ``type='virtio'``; an optional + attribute ``name`` controls how the guest will have access to the channel, + and defaults to ``name='com.redhat.spice.0'``. The optional ``address`` + element can tie the channel to a particular ``type='virtio-serial'`` + controller. Certain vdagent protocol features can by enabled or disabled + using the ``source`` element. + + Copy & Paste functionality is set by the ``clipboard`` element. It is + disabled by default, and can be enabled by setting the ``copypaste`` + property to ``yes``. This allows the guest's clipboard to be synchronized + with the qemu clipboard manager. This can enable copy and paste between a + guest and a client when using a VNC `graphics device <#elementsGraphics>`__ + (when using a VNC client that supports the copy/paste feature) or other + graphics types that support the qemu clipboard manager. + + Mouse mode is set by the ``mouse`` element, setting its ``mode`` attribute + to one of ``server`` or ``client``. If no mode is specified, the qemu + default will be used (client mode). + :since:`Since 8.2.0` :anchor:`` diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6f9954638c..3b97de89f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -713,6 +713,7 @@ VIR_ENUM_IMPL(virDomainChr, "spicevmc", "spiceport", "nmdm", + "qemu-vdagent", ); VIR_ENUM_IMPL(virDomainChrTcpProtocol, @@ -2698,6 +2699,7 @@ virDomainChrSourceDefGetPath(virDomainChrSourceDef *chr) case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_SPICEPORT: +case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT: case VIR_DOMAIN_CHR_TYPE_LAST: return NULL; } @@ -2807,6 +2809,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDef *dest, dest->data.spiceport.channel = g_strdup(src->data.spiceport.channel); break; +case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT: +dest->data.qemuVdagent.clipboard = src->data.qemuVdagent.clipboard; +dest->data.qemuVdagent.mouse = src->data.qemuVdagent.mouse; +break; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: @@ -2888,6 +2895,10 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, case VIR_DOMAIN_CHR_TYPE_SPICEVMC: return src->data.spicevmc == tgt->data.spicevmc; +case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT: +return src->data.qemuVdagent.clipboard == tgt->data.qemuVdagent.clipboard && +src->data.qemuVdagent.mouse == tgt->data.qemuVdagent.mouse; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: @@ -11244,6 +11255,33 @@ virDomainChrSourceDefParseLog(virDomainChrSourc
[libvirt PATCH v2 1/4] qemu: add capability for qemu-vdagent chardev
Detect whether qemu supports the qemu-vdagent character device. This
enables support for copy/paste with VNC graphics.
Signed-off-by: Jonathon Jongsma
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
6 files changed, 7 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 6b4ed08499..f31e7395c3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -669,6 +669,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 425 */
"blockdev.nbd.tls-hostname", /*
QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME */
"memory-backend-file.prealloc-threads", /*
QEMU_CAPS_MEMORY_BACKEND_PREALLOC_THREADS */
+ "chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
);
@@ -1629,6 +1630,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
{ "object-add/arg-type/+sev-guest/kernel-hashes",
QEMU_CAPS_SEV_GUEST_KERNEL_HASHES },
{ "calc-dirty-rate/arg-type/mode", QEMU_CAPS_DIRTYRATE_MODE },
+{ "chardev-add/arg-type/backend/+qemu-vdagent",
QEMU_CAPS_CHARDEV_QEMU_VDAGENT },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 948029d60d..f5a9609b8c 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -644,6 +644,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 425 */
QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME, /* tls hostname can be overriden for
NBD clients */
QEMU_CAPS_MEMORY_BACKEND_PREALLOC_THREADS, /* -object
memory-backend-*.prealloc-threads */
+QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 0b58210335..fceb7dbea6 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -240,6 +240,7 @@
+
6001000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
index d08b2c0213..6a5c39a0de 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml
@@ -204,6 +204,7 @@
+
6001050
0
61700244
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index cdf72b9ebf..6f4fa26808 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -242,6 +242,7 @@
+
6002000
0
43100244
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index 5227e3ee0b..8219347f2a 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -245,6 +245,7 @@
+
6002050
0
43100243
--
2.35.1
[libvirt PATCH v2 4/4] qemu: add support for qemu-vdagent channel
Implement the qemu-vdagent channel introduced in the previous commit.
Signed-off-by: Jonathon Jongsma
---
src/qemu/qemu_command.c | 21 ++-
src/qemu/qemu_monitor_json.c | 28 -
src/qemu/qemu_validate.c | 10 +++-
...l-qemu-vdagent-features.x86_64-latest.args | 41 +
.../channel-qemu-vdagent-features.xml | 37
.../channel-qemu-vdagent.x86_64-latest.args | 41 +
.../qemuxml2argvdata/channel-qemu-vdagent.xml | 37
tests/qemuxml2argvtest.c | 2 +
...el-qemu-vdagent-features.x86_64-latest.xml | 58 +++
.../channel-qemu-vdagent.x86_64-latest.xml| 58 +++
tests/qemuxml2xmltest.c | 2 +
11 files changed, 332 insertions(+), 3 deletions(-)
create mode 100644
tests/qemuxml2argvdata/channel-qemu-vdagent-features.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent-features.xml
create mode 100644
tests/qemuxml2argvdata/channel-qemu-vdagent.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/channel-qemu-vdagent.xml
create mode 100644
tests/qemuxml2xmloutdata/channel-qemu-vdagent-features.x86_64-latest.xml
create mode 100644
tests/qemuxml2xmloutdata/channel-qemu-vdagent.x86_64-latest.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1eef9fb6d0..67921229dd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1389,8 +1389,27 @@ qemuBuildChardevStr(const virDomainChrSourceDef *dev,
dev->data.spiceport.channel);
break;
-case VIR_DOMAIN_CHR_TYPE_NMDM:
case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
+virBufferAsprintf(&buf, "qemu-vdagent,id=%s,name=vdagent",
+ charAlias);
+if (dev->data.qemuVdagent.clipboard != VIR_TRISTATE_BOOL_ABSENT)
+virBufferAsprintf(&buf, ",clipboard=%s",
+
virTristateSwitchTypeToString(dev->data.qemuVdagent.clipboard));
+switch (dev->data.qemuVdagent.mouse) {
+case VIR_DOMAIN_MOUSE_MODE_CLIENT:
+virBufferAddLit(&buf, ",mouse=on");
+break;
+case VIR_DOMAIN_MOUSE_MODE_SERVER:
+virBufferAddLit(&buf, ",mouse=off");
+break;
+case VIR_DOMAIN_MOUSE_MODE_DEFAULT:
+case VIR_DOMAIN_MOUSE_MODE_LAST:
+default:
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_TYPE_NMDM:
case VIR_DOMAIN_CHR_TYPE_LAST:
default:
break;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 1ac5377449..c7e18073c2 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6814,11 +6814,37 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID,
break;
+case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT: {
+virTristateBool mouse = VIR_TRISTATE_BOOL_ABSENT;
+switch (chr->data.qemuVdagent.mouse) {
+case VIR_DOMAIN_MOUSE_MODE_CLIENT:
+mouse = VIR_TRISTATE_BOOL_YES;
+break;
+case VIR_DOMAIN_MOUSE_MODE_SERVER:
+mouse = VIR_TRISTATE_BOOL_NO;
+break;
+case VIR_DOMAIN_MOUSE_MODE_DEFAULT:
+break;
+case VIR_DOMAIN_MOUSE_MODE_LAST:
+default:
+virReportEnumRangeError(virDomainMouseMode,
+chr->data.qemuVdagent.mouse);
+return NULL;
+}
+backendType = "qemu-vdagent";
+
+if (virJSONValueObjectAdd(&backendData,
+ "T:clipboard",
chr->data.qemuVdagent.clipboard,
+ "T:mouse", mouse,
+ NULL) < 0)
+return NULL;
+break;
+}
+
case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
case VIR_DOMAIN_CHR_TYPE_PIPE:
case VIR_DOMAIN_CHR_TYPE_STDIO:
case VIR_DOMAIN_CHR_TYPE_NMDM:
-case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
virReportError(VIR_ERR_OPERATION_FAILED,
_("Hotplug unsupported for char device type '%s'"),
virDomainChrTypeToString(chr->type));
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 5ad3d914dc..2b60e63a2b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1981,6 +1981,15 @@ qemuValidateDomainChrSourceDef(const
virDomainChrSourceDef *def,
}
break;
+case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_QEMU_VDAGENT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("chardev '%s' not supported in this QEMU binary"),
+ virDomainChrTypeToString(def->type));
+return -1;
+}
+break;
+
[PATCH] kbase: Fix the syntax of the snapshot link in index.rst
Signed-off-by: Han Han --- docs/kbase/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/kbase/index.rst b/docs/kbase/index.rst index 77b23e96a8..860bfde71f 100644 --- a/docs/kbase/index.rst +++ b/docs/kbase/index.rst @@ -55,7 +55,7 @@ Usage `Memory devices `__ Memory devices and their use -`Snapshots ` +`Snapshots `__ Details about snapshotting a VM -- 2.35.1
