Re: [PATCH] docs: Mention vhostuser for queues and queue_size
On 7/20/23 08:22, Han Han wrote: > These two attributes are supported for vhost-user-blk as well. > > Signed-off-by: Han Han > --- > docs/formatdomain.rst | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index 4af0b82569..447ab32c01 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -3275,9 +3275,9 @@ paravirtualized driver is specified via the ``disk`` > element. >"virtio" ``bus`` and "pci" or "ccw" ``address`` types. :since:`Since > 1.2.8 >(QEMU 2.1)` > - The optional ``queues`` attribute specifies the number of virt queues > for > - virtio-blk. ( :since:`Since 3.9.0` ) > + virtio-blk or vhost-user-blk. ( :since:`Since 3.9.0` ) This doesn't feel right. The vhost-user-blk disk was introduced fairly recently and 3.9.0 is just ancient. Digging into commits, vhost-user-blk disk was introduced in v7.1.0 and I didn't check whether it supported the attribute from the very beginning. I think if we want to document the attribute then we should probably do it like this: The optional ``queues`` attribute specifies the number of virt queues for virtio-blk ( :sinnce:`Since 3.9.0) or vhost-user-blk ( :since:`Since X.Y.Z` ). > - The optional ``queue_size`` attribute specifies the size of each virt > - queue for virtio-blk. ( :since:`Since 7.8.0` ) > + queue for virtio-blk or vhost-user-blk. ( :since:`Since 7.8.0` ) > - For virtio disks, `Virtio-related options`_ can also >be set. ( :since:`Since 3.5.0` ) > - The optional ``metadata_cache`` subelement controls aspects related to > the I have not checked this later hunk. Michal
[PATCH] docs: Mention vhostuser for queues and queue_size
These two attributes are supported for vhost-user-blk as well. Signed-off-by: Han Han --- docs/formatdomain.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 4af0b82569..447ab32c01 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3275,9 +3275,9 @@ paravirtualized driver is specified via the ``disk`` element. "virtio" ``bus`` and "pci" or "ccw" ``address`` types. :since:`Since 1.2.8 (QEMU 2.1)` - The optional ``queues`` attribute specifies the number of virt queues for - virtio-blk. ( :since:`Since 3.9.0` ) + virtio-blk or vhost-user-blk. ( :since:`Since 3.9.0` ) - The optional ``queue_size`` attribute specifies the size of each virt - queue for virtio-blk. ( :since:`Since 7.8.0` ) + queue for virtio-blk or vhost-user-blk. ( :since:`Since 7.8.0` ) - For virtio disks, `Virtio-related options`_ can also be set. ( :since:`Since 3.5.0` ) - The optional ``metadata_cache`` subelement controls aspects related to the -- 2.41.0
Re: [libvirt PATCH 3/4] Add virNetworkObj Get and Set Methods for Metadata
On 19/07/23 14:48, Michal Prívozník wrote:
On 7/11/23 08:47, K Shiva Kiran wrote:
- Introduces virNetworkObjGetMetadata() and
virNetworkObjSetMetadata().
- These functions implement common behaviour that can be reused by
network drivers that use the virNetworkObj struct.
- Also adds helper functions that resolve the live/persistent state of
the network before setting metadata.
Signed-off-by: K Shiva Kiran
---
src/conf/virnetworkobj.c | 325 +++
src/conf/virnetworkobj.h | 17 ++
2 files changed, 342 insertions(+)
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index b8b86da06f..41d0a1d971 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
return 0;
}
+
+
+/**
+ * virNetworkObjUpdateModificationImpact:
+ *
+ * @net: network object
+ * @flags: flags to update the modification impact on
+ *
+ * Resolves virNetworkUpdateFlags in @flags so that they correctly
+ * apply to the actual state of @net. @flags may be modified after call to this
+ * function.
+ *
+ * Returns 0 on success if @flags point to a valid combination for @net or -1
on
+ * error.
+ */
+static int
+virNetworkObjUpdateModificationImpact(virNetworkObj *net,
+ unsigned int *flags)
+{
+bool isActive = virNetworkObjIsActive(net);
+
+if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE |
VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
+VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
+if (isActive)
+*flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
+else
+*flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
+}
+
+if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("network is not running"));
+return -1;
+}
+
+if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("transient networks do not have any "
+ "persistent config"));
I'm going to mention it here, but it applies to the all of your patches,
and all the new code in fact. We made an exemption for error messages
and thus they don't need to be broken at 80 chars limit. In fact, they
shouldn't. The reason is simple: easier grep as one doesn't have to try
and guess how is the message broken into individual substrings. Of
course, you will find plenty of "bad" examples throughout the code, but
that's because it is an old code. Whenever we rewrite something or
introduce new code this exception applies and the old code is, well,
gradually fixed.
+return -1;
+}
+
+return 0;
This code is basically the same as in networkUpdate(). The first part
that sets _LIVE or _CONFIG is there verbatim, the second one is hidden
under virNetworkObjConfigChangeSetup(). There's one extra step that the
function does - it calls virNetworkObjSetDefTransient() but I don't
think that's necessary. Either the network is active and
virNetworkObjSetDefTransient() was already called, or is inactive and
thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be
removed.
After this, virNetworkObjUpdateModificationImpact() could be exported
(just like corresponding virDomain* sibling function is) so that it can
be called from both src/conf/virnetworkobj.c and
src/network/bridge_driver.c. Because I think we can get away with the
networkUpdate() doing the following:
networkUpdate() {
...
virNetworkObjUpdateModificationImpact();
if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
/* This is the check that possibly calls
networkRemoveFirewallRules() and sets needFirewallRefresh = true */
}
virNetworkObjUpdate();
...
}
BTW: when cooking the patch, don't forget the same pattern is copied to
our test driver (src/test/test_driver.c).
Michal
Applied in v2:
https://listman.redhat.com/archives/libvir-list/2023-July/240831.html
I have forgotten to mention the diff w.r.t v1 within the v2 patch itself,
thus have included it in a reply.
Shiva
Re: [libvirt PATCH v2 3/4] Add virNetworkObj Get and Set Methods for Metadata
On 19/07/23 21:47, K Shiva Kiran wrote: - Introduces virNetworkObjGetMetadata() and virNetworkObjSetMetadata(). - These functions implement common behaviour that can be reused by network drivers that use the virNetworkObj struct. - Introduces virNetworkObjUpdateModificationImpact() among other helper functions that resolve the live/persistent state of the network before setting metadata. - Replace redundant flag resolving code with the forementioned function in bridge_driver.c and test_driver.c - Eliminates redundant call of virNetworkObjSetDefTransient() in virNetworkConfigChangeSetup(). Signed-off-by: K Shiva Kiran --- This patch is a v2 of: https://listman.redhat.com/archives/libvir-list/2023-July/240662.html Diff to v1: - Replace redundant call of virNetworkObjSetDefTransient() in virNetworkConfigChangeSetup() - Replace redundant virNetworkFlags setting code with virNetworkObjUpdateModificationImpact() in bridge_driver.c and test_driver.c - Eliminated newlines in message strings that followed the 80 character limit. Shiva
Re: [libvirt PATCH 2/4] Adding Public Get and Set APIs for Network Metadata
On 19/07/23 14:48, Michal Prívozník wrote:
On 7/11/23 08:47, K Shiva Kiran wrote:
This patch introduces public Get and Set APIs for modifying ,
and elements of the Network object.
- Added enum to select one of the above elements to operate on.
- Added error code and messages for missing metadata.
- Added public API implementation.
- Added driver support.
- Defined wire protocol format.
Signed-off-by: K Shiva Kiran
---
include/libvirt/libvirt-network.h | 29 ++
include/libvirt/virterror.h | 1 +
src/driver-network.h | 16 +++
src/libvirt-network.c | 167 ++
src/libvirt_public.syms | 6 ++
src/remote/remote_driver.c| 2 +
src/remote/remote_protocol.x | 36 ++-
src/remote_protocol-structs | 19
src/util/virerror.c | 3 +
9 files changed, 278 insertions(+), 1 deletion(-)
We usually introduce public APIs in one patch and implement remote
driver in another. But I guess I can live with this.
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 3c6c230a16..14898a0bc7 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -3130,6 +3130,23 @@ struct remote_network_port_delete_args {
remote_nonnull_network_port port;
u_int flags;
};
+struct remote_network_set_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string metadata;
+remote_string key;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_ret {
+remote_nonnull_string metadata;
+};
This is misplaced.
struct remote_domain_checkpoint_create_xml_args {
remote_nonnull_domain dom;
remote_nonnull_string xml_desc;
@@ -3717,4 +3734,6 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441,
REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
+REMOTE_PROC_NETWORK_SET_METADATA = 444,
+REMOTE_PROC_NETWORK_GET_METADATA = 445
Here we want the trailing comma.
};
Squash in the following:
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 14898a0bc7..c07e0af1e6 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2687,6 +2687,23 @@ struct remote_network_event_lifecycle_msg {
intevent;
intdetail;
};
+struct remote_network_set_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string metadata;
+remote_string key;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_ret {
+remote_nonnull_string metadata;
+};
struct remote_connect_storage_pool_event_register_any_args {
inteventID;
remote_storage_poolpool;
@@ -3130,23 +3147,6 @@ struct remote_network_port_delete_args {
remote_nonnull_network_port port;
u_int flags;
};
-struct remote_network_set_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string metadata;
-remote_string key;
-remote_string uri;
-u_int flags;
-};
-struct remote_network_get_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string uri;
-u_int flags;
-};
-struct remote_network_get_metadata_ret {
-remote_nonnull_string metadata;
-};
struct remote_domain_checkpoint_create_xml_args {
remote_nonnull_domain dom;
remote_nonnull_string xml_desc;
@@ -3735,5 +3735,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
REMOTE_PROC_NETWORK_SET_METADATA = 444,
-REMOTE_PROC_NETWORK_GET_METADATA = 445
+REMOTE_PROC_NETWORK_GET_METADATA = 445,
};
Michal
Squashed in v2:
https://listman.redhat.com/archives/libvir-list/2023-July/240830.html
I realize it's a mistake to send a v2 patch as a reply to the sa
[libvirt PATCH v2 3/4] Add virNetworkObj Get and Set Methods for Metadata
- Introduces virNetworkObjGetMetadata() and
virNetworkObjSetMetadata().
- These functions implement common behaviour that can be reused by
network drivers that use the virNetworkObj struct.
- Introduces virNetworkObjUpdateModificationImpact() among other
helper functions that resolve the live/persistent state of
the network before setting metadata.
- Replace redundant flag resolving code with the forementioned
function in bridge_driver.c and test_driver.c
- Eliminates redundant call of virNetworkObjSetDefTransient() in
virNetworkConfigChangeSetup().
Signed-off-by: K Shiva Kiran
---
src/conf/virnetworkobj.c| 329 ++--
src/conf/virnetworkobj.h| 21 +++
src/libvirt_private.syms| 1 +
src/network/bridge_driver.c | 14 +-
src/test/test_driver.c | 16 +-
5 files changed, 345 insertions(+), 36 deletions(-)
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index b8b86da06f..20ee8eb58a 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -725,7 +725,6 @@ virNetworkObjReplacePersistentDef(virNetworkObj *obj,
*/
static int
virNetworkObjConfigChangeSetup(virNetworkObj *obj,
- virNetworkXMLOption *xmlopt,
unsigned int flags)
{
bool isActive;
@@ -738,17 +737,10 @@ virNetworkObjConfigChangeSetup(virNetworkObj *obj,
return -1;
}
-if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
-if (!obj->persistent) {
+if ((flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) &&
+ !obj->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot change persistent config of a "
- "transient network"));
-return -1;
-}
-/* this should already have been done by the driver, but do it
- * anyway just in case.
- */
-if (isActive && (virNetworkObjSetDefTransient(obj, false, xmlopt) < 0))
+ _("cannot change persistent config of a transient
network"));
return -1;
}
@@ -1187,7 +1179,7 @@ virNetworkObjUpdate(virNetworkObj *obj,
g_autoptr(virNetworkDef) configdef = NULL;
/* normalize config data, and check for common invalid requests. */
-if (virNetworkObjConfigChangeSetup(obj, xmlopt, flags) < 0)
+if (virNetworkObjConfigChangeSetup(obj, flags) < 0)
return -1;
if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) {
@@ -1822,3 +1814,316 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
return 0;
}
+
+
+/**
+ * virNetworkObjUpdateModificationImpact:
+ *
+ * @obj: network object
+ * @flags: flags to update the modification impact on
+ *
+ * Resolves virNetworkUpdateFlags in @flags so that they correctly
+ * apply to the actual state of @obj. @flags may be modified after call to this
+ * function.
+ *
+ * Returns 0 on success if @flags point to a valid combination for @obj or -1
on
+ * error.
+ */
+int
+virNetworkObjUpdateModificationImpact(virNetworkObj *obj,
+ unsigned int *flags)
+{
+bool isActive = virNetworkObjIsActive(obj);
+
+if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE |
VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
+VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
+if (isActive)
+*flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
+else
+*flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
+}
+
+if (virNetworkObjConfigChangeSetup(obj, *flags) < 0)
+return -1;
+
+return 0;
+}
+
+
+/**
+ * virNetworkObjGetDefs:
+ *
+ * @net: network object
+ * @flags: for virNetworkUpdateFlags
+ * @liveDef: Set the pointer to the live definition of @net.
+ * @persDef: Set the pointer to the config definition of @net.
+ *
+ * Helper function to resolve @flags and retrieve correct network pointer
+ * objects. This function should be used only when the network driver
+ * creates net->newDef once the network has started.
+ *
+ * If @liveDef or @persDef are set it implies that @flags request modification
+ * thereof.
+ *
+ * Returns 0 on success and sets @liveDef and @persDef; -1 if @flags are
+ * inappropriate.
+ */
+static int
+virNetworkObjGetDefs(virNetworkObj *net,
+unsigned int flags,
+virNetworkDef **liveDef,
+virNetworkDef **persDef)
+{
+if (liveDef)
+*liveDef = NULL;
+
+if (persDef)
+*persDef = NULL;
+
+if (virNetworkObjUpdateModificationImpact(net, &flags) < 0)
+return -1;
+
+if (virNetworkObjIsActive(net)) {
+if (liveDef && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE))
+*liveDef = net->def;
+
+if (persDef && (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG))
+*persDef = net->newDef;
+} else {
+if (persDef)
+*persDef = net->def;
+}
+
+return 0;
+}
+
+
+/**
+ * virNetworkObjGetOneDefState:
+ *
+ * @net: Network obje
Re: [PATCH] node_device: Don't leak error message buffer from virMdevctlListDefined|Active
On 7/19/23 3:41 PM, Peter Krempa wrote: nodeDeviceUpdateMediatedDevices invokes virMdevctlListDefined and virMdevctlListActive both of which were passed the same 'errmsg' buffer. Since virCommandSetErrorBuffer() always allocates the error buffer one of them was leaked. Fix it by populating the 'errmsg' buffer only on failure of virMdevctlListActive|Defined which invoke the command. Add a comment to nodeDeviceGetMdevctlListCommand reminding how virCommandSetErrorBuffer() works. Fixes: 44a0f2f0c8f Signed-off-by: Peter Krempa LGTM Thanks for catching and fixing it. Reviewed-by: Boris Fiuczynski -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [libvirt PATCH 0/4] Introduce new Metadata fields for Network object with corresponding APIs
On 19/07/23 14:48, Michal Prívozník wrote: On 7/11/23 08:47, K Shiva Kiran wrote: This commit introduces and fields to the XML schema of the Network object. It also adds public Get/Set APIs for modifying all metadata fields of the same, along with a test program. K Shiva Kiran (4): Add and for Network Objects Adding Public Get and Set APIs for Network Metadata Add virNetworkObj Get and Set Methods for Metadata Add Test driver and testcase for Network Metadata change APIs docs/formatnetwork.rst| 11 + include/libvirt/libvirt-network.h | 29 +++ include/libvirt/virterror.h | 1 + src/conf/network_conf.c | 21 ++ src/conf/network_conf.h | 2 + src/conf/schemas/basictypes.rng | 15 ++ src/conf/schemas/domaincommon.rng | 15 -- src/conf/schemas/network.rng | 10 + src/conf/virnetworkobj.c | 325 ++ src/conf/virnetworkobj.h | 17 ++ src/driver-network.h | 16 ++ src/libvirt-network.c | 167 +++ src/libvirt_public.syms | 6 + src/remote/remote_driver.c| 2 + src/remote/remote_protocol.x | 36 +++- src/remote_protocol-structs | 19 ++ src/test/test_driver.c| 67 ++ src/util/virerror.c | 3 + tests/meson.build | 1 + tests/networkmetadatatest.c | 297 +++ 20 files changed, 1044 insertions(+), 16 deletions(-) create mode 100644 tests/networkmetadatatest.c No virsh exposure of these new APIs? :-( Michal I had planned to introduce virsh as well as metadata change callbacks soon after these patches got pushed, is that ok? Shiva
Re: [PATCH] qemu: Adapt to new way of specifying PC speaker
On Mon, Jun 26, 2023 at 2:13 PM Michal Privoznik
wrote:
> Historically, the way to set PC speaker for a guest was to pass:
>
> -soundhw pcspk
>
> but as of QEMU commit v5.1.0-rc0~28^2~3 this is deprecated and we
> should use:
>
> -machine pcspk-audiodev=$id
>
> instead. The old way was then removed in commit v7.1.0-rc0~99^2~3.
>
> Now, ideally we would have a capability selecting whether we talk
> to a QEMU that understands the new way or not. But it's not that
> simple - the machine attribute is just an alias to the .audiodev=
> attribute of 'isa-pcspk' object and both are created in
> pc_machine_initfn() function, i.e. not then the PC_MACHINE() class
> is initialized, but when it's instantiated. IOW, it's not possible
> for us to query whether we're dealing with older or newer QEMU.
>
> But given that the newer version is supported since v5.1.0 and the
> minimal version we require is v4.2.0 (i.e. there are two releases
> which don't understand the newer cmd line) and how frequently this
> feature is (un-)used (the issue was reported after ~1 year since it
> stopped working), I believe we can live without any capability and
> just use the newer cmd line unconditionally.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/490
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_command.c | 57 ---
> .../sound-device.x86_64-4.2.0.args| 3 +-
> .../sound-device.x86_64-latest.args | 3 +-
> 3 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a19902988c..3b0b162b8b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4477,31 +4477,30 @@ qemuBuildSoundCommandLine(virCommand *cmd,
> for (i = 0; i < def->nsounds; i++) {
> virDomainSoundDef *sound = def->sounds[i];
>
> -/* Sadly pcspk device doesn't use -device syntax. Fortunately
> - * we don't need to set any PCI address on it, so we don't
> - * mind too much */
> -if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) {
> -virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL);
> -} else {
> -if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps)
> < 0)
> -return -1;
> +/* Sadly pcspk device doesn't use -device syntax. And it
> + * was handled already in qemuBuildMachineCommandLine().
> + */
> +if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK)
> +continue;
>
> -if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
> -return -1;
> +if (qemuCommandAddExtDevice(cmd, &sound->info, def, qemuCaps) < 0)
> +return -1;
>
> -if (virDomainSoundModelSupportsCodecs(sound)) {
> -for (j = 0; j < sound->ncodecs; j++) {
> -if (qemuBuildSoundCodecCmd(cmd, def, sound,
> sound->codecs[j],
> - qemuCaps) < 0)
> -return -1;
> -}
> +if (qemuBuildSoundDevCmd(cmd, def, sound, qemuCaps) < 0)
> +return -1;
>
> -if (j == 0) {
> -virDomainSoundCodecDef codec = {
> VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
> +if (virDomainSoundModelSupportsCodecs(sound)) {
> +for (j = 0; j < sound->ncodecs; j++) {
> +if (qemuBuildSoundCodecCmd(cmd, def, sound,
> sound->codecs[j],
> + qemuCaps) < 0)
> +return -1;
> +}
>
> -if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec,
> qemuCaps) < 0)
> -return -1;
> -}
> +if (j == 0) {
> +virDomainSoundCodecDef codec = {
> VIR_DOMAIN_SOUND_CODEC_TYPE_DUPLEX, 0 };
> +
> +if (qemuBuildSoundCodecCmd(cmd, def, sound, &codec,
> qemuCaps) < 0)
> +return -1;
> }
> }
> }
> @@ -7029,6 +7028,22 @@ qemuBuildMachineCommandLine(virCommand *cmd,
> }
> }
>
> +/* PC speaker is a bit different than the rest of sound cards
> + * which is handled in qemuBuildSoundCommandLine(). */
>
maybe
*which are handled...
?
> +for (i = 0; i < def->nsounds; i++) {
> +const virDomainSoundDef *sound = def->sounds[i];
> +g_autofree char *audioid = NULL;
> +
> +if (sound->model != VIR_DOMAIN_SOUND_MODEL_PCSPK)
> +continue;
> +
> +if (!(audioid = qemuGetAudioIDString(def, sound->audioId)))
> +return -1;
> +
> +virBufferAsprintf(&buf, ",pcspk-audiodev=%s", audioid);
> +break;
> +}
> +
> qemuBuildMachineACPI(&buf, def, qemuCaps);
>
> virCommandAddArgBuffer(cmd, &buf);
> diff --git a/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
> b/tests/qemuxml2argvdata/sound-device.x86_64-4.2.0.args
>
[PATCH] node_device: Don't leak error message buffer from virMdevctlListDefined|Active
nodeDeviceUpdateMediatedDevices invokes virMdevctlListDefined and
virMdevctlListActive both of which were passed the same 'errmsg' buffer.
Since virCommandSetErrorBuffer() always allocates the error buffer one
of them was leaked.
Fix it by populating the 'errmsg' buffer only on failure of
virMdevctlListActive|Defined which invoke the command.
Add a comment to nodeDeviceGetMdevctlListCommand reminding how
virCommandSetErrorBuffer() works.
Fixes: 44a0f2f0c8f
Signed-off-by: Peter Krempa
---
src/node_device/node_device_driver.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index a2d0600560..f8fae71194 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1044,6 +1044,15 @@ virMdevctlSetAutostart(virNodeDeviceDef *def, bool
autostart, char **errmsg)
}
+/**
+ * nodeDeviceGetMdevctlListCommand:
+ * @defined: list mdevctl entries with persistent config
+ * @output: filled with the output of mdevctl once invoked
+ * @errmsg: always allocated, optionally filled with error from 'mdevctl'
+ *
+ * Prepares a virCommand structure to invoke 'mdevctl' caller is responsible to
+ * free the buffers which are filled by the virCommand infrastructure.
+ */
virCommand*
nodeDeviceGetMdevctlListCommand(bool defined,
char **output,
@@ -1623,9 +1632,11 @@ virMdevctlListDefined(virNodeDeviceDef ***devs, char
**errmsg)
{
int status;
g_autofree char *output = NULL;
-g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output,
errmsg);
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output,
&errbuf);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
+*errmsg = g_steal_pointer(&errbuf);
return -1;
}
@@ -1641,9 +1652,11 @@ virMdevctlListActive(virNodeDeviceDef ***devs, char
**errmsg)
{
int status;
g_autofree char *output = NULL;
-g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false,
&output, errmsg);
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false,
&output, &errbuf);
if (virCommandRun(cmd, &status) < 0 || status != 0) {
+*errmsg = g_steal_pointer(&errbuf);
return -1;
}
--
2.41.0
[libvirt PATCH v2 2/4] Adding Public Get and Set APIs for Network Metadata
From 61d699de0603ca49a67cc898a219cce2613b82ba Mon Sep 17 00:00:00 2001
From: K Shiva Kiran
Date: Tue, 27 Jun 2023 20:48:52 +0530
Subject: [libvirt PATCH v2 2/4] Adding Public Get and Set APIs for
Network Metadata
This patch introduces public Get and Set APIs for modifying ,
and elements of the Network object.
- Added enum to select one of the above elements to operate on.
- Added error code and messages for missing metadata.
- Added public API implementation.
- Added driver support.
- Defined wire protocol format.
---
This is a v2 of:
https://listman.redhat.com/archives/libvir-list/2023-July/240666.html
include/libvirt/libvirt-network.h | 29 ++
include/libvirt/virterror.h | 1 +
src/driver-network.h | 16 +++
src/libvirt-network.c | 167 ++
src/libvirt_public.syms | 6 ++
src/remote/remote_driver.c | 2 +
src/remote/remote_protocol.x | 36 ++-
src/remote_protocol-structs | 19
src/util/virerror.c | 3 +
9 files changed, 278 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-network.h
b/include/libvirt/libvirt-network.h
index 90cde0cf24..147dc8c6b8 100644
--- a/include/libvirt/libvirt-network.h
+++ b/include/libvirt/libvirt-network.h
@@ -547,4 +547,33 @@ virNetworkPortFree(virNetworkPortPtr port);
int
virNetworkPortRef(virNetworkPortPtr port);
+/**
+ * virNetworkMetadataType:
+ *
+ * Since: 9.6.0
+ */
+typedef enum {
+ VIR_NETWORK_METADATA_DESCRIPTION = 0, /* Operate on
(Since: 9.6.0) */
+ VIR_NETWORK_METADATA_TITLE = 1, /* Operate on (Since:
9.6.0) */
+ VIR_NETWORK_METADATA_ELEMENT = 2, /* Operate on
(Since: 9.6.0) */
+
+# ifdef VIR_ENUM_SENTINELS
+ VIR_NETWORK_METADATA_LAST /* (Since: 9.6.0) */
+# endif
+} virNetworkMetadataType;
+
+int
+virNetworkSetMetadata(virNetworkPtr network,
+ int type,
+ const char *metadata,
+ const char *key,
+ const char *uri,
+ unsigned int flags);
+
+char *
+virNetworkGetMetadata(virNetworkPtr network,
+ int type,
+ const char *uri,
+ unsigned int flags);
+
#endif /* LIBVIRT_NETWORK_H */
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index df13e4f11e..55bc4431d9 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -348,6 +348,7 @@ typedef enum {
VIR_ERR_NO_HOSTNAME = 108, /* no domain's hostname found
(Since: 6.1.0) */
VIR_ERR_CHECKPOINT_INCONSISTENT = 109, /* checkpoint can't be used
(Since: 6.10.0) */
VIR_ERR_MULTIPLE_DOMAINS = 110, /* more than one matching
domain found (Since: 7.1.0) */
+ VIR_ERR_NO_NETWORK_METADATA = 111, /* Network metadata is not
present (Since: 9.6.0) */
# ifdef VIR_ENUM_SENTINELS
VIR_ERR_NUMBER_LAST /* (Since: 5.0.0) */
diff --git a/src/driver-network.h b/src/driver-network.h
index 99efd4c8aa..1d19b013c9 100644
--- a/src/driver-network.h
+++ b/src/driver-network.h
@@ -161,6 +161,20 @@ typedef int
virNetworkPortPtr **ports,
unsigned int flags);
+typedef int
+(*virDrvNetworkSetMetadata)(virNetworkPtr network,
+ int type,
+ const char *metadata,
+ const char *key,
+ const char *uri,
+ unsigned int flags);
+
+typedef char *
+(*virDrvNetworkGetMetadata)(virNetworkPtr network,
+ int type,
+ const char *uri,
+ unsigned int flags);
+
typedef struct _virNetworkDriver virNetworkDriver;
/**
@@ -202,4 +216,6 @@ struct _virNetworkDriver {
virDrvNetworkPortGetParameters networkPortGetParameters;
virDrvNetworkPortDelete networkPortDelete;
virDrvNetworkListAllPorts networkListAllPorts;
+ virDrvNetworkSetMetadata networkSetMetadata;
+ virDrvNetworkGetMetadata networkGetMetadata;
};
diff --git a/src/libvirt-network.c b/src/libvirt-network.c
index 236dfe2f5d..2dd11cf205 100644
--- a/src/libvirt-network.c
+++ b/src/libvirt-network.c
@@ -1915,3 +1915,170 @@ virNetworkPortRef(virNetworkPortPtr port)
virObjectRef(port);
return 0;
}
+
+
+/**
+ * virNetworkSetMetadata:
+ * @network: a network object
+ * @type: type of metadata, from virNetworkMetadataType
+ * @metadata: new metadata text
+ * @key: XML namespace key, or NULL
+ * @uri: XML namespace URI, or NULL
+ * @flags: bitwise-OR of virNetworkUpdateFlags
+ *
+ * Sets the appropriate network element given by @type to the
+ * value of @metadata. A @type of VIR_NETWORK_METADATA_DESCRIPTION
+ * is free-form text; VIR_NETWORK_METADATA_TITLE is free-form, but no
+ * newlines are permitted, and should be short (although the length is
+ * not enforced). For these two options @key
Re: [PATCH 2/2] tests: Refresh valgrind suppressions
On 7/19/23 12:50, Peter Krempa wrote: > On Tue, Jul 18, 2023 at 14:28:57 +0200, Michal Privoznik wrote: >> Since nobody is expected to run valgrind over scripts now, we can >> drop plenty of suppressions. Also, there are some old ones that >> no longer exist and new ones, that are not covered. >> >> Signed-off-by: Michal Privoznik >> --- >> tests/.valgrind.supp | 188 ++- >> 1 file changed, 41 insertions(+), 147 deletions(-) > > Is it really worth keeping this file in the repository? I don't think > we can keep it current. > We have valgrind test setup (add_test_setup() in tests/meson.build) that passes this file. What we could have is a configure option that specifies where the file is so that everybody can keep a copy specific to their system. Would that work? Michal
Re: [libvirt PATCH 7/9] util: relax requirement for logind to be running
On Wed, Jun 21, 2023 at 14:32:30 +0100, Daniel P. Berrangé wrote: > Historically we wanted to check if logind was actually running, not > merely activatable, because on systems where systemd is installed, > but the OS is booted into non-systemd init, we want to fallback to > pm-utils. > > Requiring logind to be running, however, forces us to serialize libvirtd > startup on startup of logind which is undesirable. We can relax this > dependancy if we check whether systemd itself is running, which implies > that logind will activated when we need it. > > https://gitlab.com/libvirt/libvirt/-/issues/489 > Signed-off-by: Daniel P. Berrangé > --- > src/util/virsystemd.c | 12 > 1 file changed, 12 insertions(+) Based on the discussion in the other thread: Reviewed-by: Peter Krempa
Re: [PATCH 2/2] tests: Refresh valgrind suppressions
On Wed, Jul 19, 2023 at 14:31:24 +0200, Michal Prívozník wrote: > On 7/19/23 12:50, Peter Krempa wrote: > > On Tue, Jul 18, 2023 at 14:28:57 +0200, Michal Privoznik wrote: > >> Since nobody is expected to run valgrind over scripts now, we can > >> drop plenty of suppressions. Also, there are some old ones that > >> no longer exist and new ones, that are not covered. > >> > >> Signed-off-by: Michal Privoznik > >> --- > >> tests/.valgrind.supp | 188 ++- > >> 1 file changed, 41 insertions(+), 147 deletions(-) > > > > Is it really worth keeping this file in the repository? I don't think > > we can keep it current. > > > > We have valgrind test setup (add_test_setup() in tests/meson.build) that > passes this file. What we could have is a configure option that > specifies where the file is so that everybody can keep a copy specific > to their system. Would that work? If we invoke it from the testsuite then I guess it makes sense to distribute it. Arguably we could make it empty or contain a comment saying what to put there. Reviewed-by: Peter Krempa
Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE
On 7/19/23 12:43, Peter Krempa wrote: > On Thu, Jun 29, 2023 at 17:13:42 +0200, Michal Privoznik wrote: >> Introduced in QEMU commit of v0.14.0-rc0~83^2~1 and not being >> able to compile the .removable attribute of the "usb-storage" >> object out, renders our corresponding capability >> QEMU_CAPS_USB_STORAGE_REMOVABLE useless. Retire it. >> >> Signed-off-by: Michal Privoznik >> --- >> src/qemu/qemu_capabilities.c | 9 +- >> src/qemu/qemu_capabilities.h | 2 +- >> src/qemu/qemu_command.c | 10 +- >> src/qemu/qemu_validate.c | 8 - >> .../caps_4.2.0_aarch64.replies| 195 -- >> .../caps_4.2.0_aarch64.xml| 1 - >> .../caps_4.2.0_ppc64.replies | 179 +++- >> .../qemucapabilitiesdata/caps_4.2.0_ppc64.xml | 1 - >> .../caps_4.2.0_x86_64.replies | 211 +-- >> .../caps_4.2.0_x86_64.xml | 1 - >> .../caps_5.0.0_aarch64.replies| 211 --- >> .../caps_5.0.0_aarch64.xml| 1 - >> .../caps_5.0.0_ppc64.replies | 203 -- >> .../qemucapabilitiesdata/caps_5.0.0_ppc64.xml | 1 - >> .../caps_5.0.0_riscv64.replies| 191 +++-- >> .../caps_5.0.0_riscv64.xml| 1 - >> .../caps_5.0.0_x86_64.replies | 227 +--- >> .../caps_5.0.0_x86_64.xml | 1 - >> .../caps_5.1.0_x86_64.replies | 231 +--- >> .../caps_5.1.0_x86_64.xml | 1 - >> .../caps_5.2.0_aarch64.replies| 219 --- >> .../caps_5.2.0_aarch64.xml| 1 - >> .../caps_5.2.0_ppc64.replies | 207 -- >> .../qemucapabilitiesdata/caps_5.2.0_ppc64.xml | 1 - >> .../caps_5.2.0_riscv64.replies| 195 +++--- >> .../caps_5.2.0_riscv64.xml| 1 - >> .../caps_5.2.0_x86_64.replies | 235 +--- >> .../caps_5.2.0_x86_64.xml | 1 - >> .../caps_6.0.0_aarch64.replies| 217 --- >> .../caps_6.0.0_aarch64.xml| 1 - >> .../caps_6.0.0_x86_64.replies | 233 +--- >> .../caps_6.0.0_x86_64.xml | 1 - >> .../caps_6.1.0_x86_64.replies | 239 +--- >> .../caps_6.1.0_x86_64.xml | 1 - >> .../caps_6.2.0_aarch64.replies| 223 --- >> .../caps_6.2.0_aarch64.xml| 1 - >> .../caps_6.2.0_ppc64.replies | 211 --- >> .../qemucapabilitiesdata/caps_6.2.0_ppc64.xml | 1 - >> .../caps_6.2.0_x86_64.replies | 243 + >> .../caps_6.2.0_x86_64.xml | 1 - >> .../caps_7.0.0_aarch64+hvf.replies| 227 >> .../caps_7.0.0_aarch64+hvf.xml| 1 - >> .../caps_7.0.0_aarch64.replies| 227 >> .../caps_7.0.0_aarch64.xml| 1 - >> .../caps_7.0.0_ppc64.replies | 211 --- >> .../qemucapabilitiesdata/caps_7.0.0_ppc64.xml | 1 - >> .../caps_7.0.0_x86_64.replies | 243 + >> .../caps_7.0.0_x86_64.xml | 1 - >> .../caps_7.1.0_ppc64.replies | 211 --- >> .../qemucapabilitiesdata/caps_7.1.0_ppc64.xml | 1 - >> .../caps_7.1.0_x86_64.replies | 243 + >> .../caps_7.1.0_x86_64.xml | 1 - >> .../caps_7.2.0_ppc.replies| 211 +++ >> tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml | 1 - >> .../caps_7.2.0_x86_64+hvf.replies | 255 +- >> .../caps_7.2.0_x86_64+hvf.xml | 1 - >> .../caps_7.2.0_x86_64.replies | 255 +- >> .../caps_7.2.0_x86_64.xml | 1 - >> .../caps_8.0.0_riscv64.replies| 207 +++--- >> .../caps_8.0.0_riscv64.xml| 1 - >> .../caps_8.0.0_x86_64.replies | 255 +- >> .../caps_8.0.0_x86_64.xml | 1 - >> .../caps_8.1.0_x86_64.replies | 255 +- >> .../caps_8.1.0_x86_64.xml | 1 - >> 64 files changed, 1704 insertions(+), 5025 deletions(-) > > Please split this at least into a patch that retires the capability > (removes the code), you can include removal of the detection of the > capability but usually split that separately too. > > For those one/two patches you can use: > > Reviewed-by: Peter Krempa > > A separate patch then drops the actual probing of the usb-storage qemu > device. > > Additionally consider querying the devic
Re: [PATCH 1/5] virsh: Make cmdVersion() work with split daemon
On 7/18/23 17:47, Peter Krempa wrote:
> On Tue, Jul 18, 2023 at 17:27:33 +0200, Michal Privoznik wrote:
>> When virsh connects to a non-hypervisor daemon directly (e.g.
>> "nodedev:///system") and user executes 'version' they are met
>> with an error message. This is because cmdVersion() calls
>> virConnectGetVersion() which fails, hence the error.
>>
>> The reason for virConnectGetVersion() fail is simple - it's
>> documented as:
>>
>> Get the version level of the Hypervisor running.
>>
>> Well, there's no hypervisor in non-hypervisor daemons and thus it
>> doesn't make sense to provide an implementation in each driver's
>> virConnectDriver.hypervisorDriver table (just like we do for
>> other APIs, e.g. nodeConnectIsSecure()).
>>
>> Given all of this, just make cmdVersion() deal with the error in
>> a non-fatal fashion.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> tools/virsh-host.c | 26 --
>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
>> index 0bda327cae..35e6a2eb98 100644
>> --- a/tools/virsh-host.c
>> +++ b/tools/virsh-host.c
>> @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd
>> G_GNUC_UNUSED)
>> vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
>> major, minor, rel);
>>
>> -if (virConnectGetVersion(priv->conn, &hvVersion) < 0) {
>> -vshError(ctl, "%s", _("failed to get the hypervisor version"));
>> -return false;
>> -}
>> -if (hvVersion == 0) {
>> -vshPrint(ctl,
>> - _("Cannot extract running %1$s hypervisor version\n"),
>> hvType);
>> -} else {
>> -major = hvVersion / 100;
>> -hvVersion %= 100;
>> -minor = hvVersion / 1000;
>> -rel = hvVersion % 1000;
>> +if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) {
>> +if (hvVersion == 0) {
>> +vshPrint(ctl,
>> + _("Cannot extract running %1$s hypervisor version\n"),
>> hvType);
>> +} else {
>> +major = hvVersion / 100;
>> +hvVersion %= 100;
>> +minor = hvVersion / 1000;
>> +rel = hvVersion % 1000;
>>
>> -vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> - hvType, major, minor, rel);
>> +vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> + hvType, major, minor, rel);
>> +}
>> }
>
> Ideally you'd also add an else branch with 'vshResetLibvirtError(); but
> the other call to virConnectGetLibVersion() doesn't do that so ...
> whatever ... I guess :)
Oh, you're right. In fact we should also ignore VIR_ERR_NO_SUPPORT.
Other errors might be actually a good reason to return early. Consider
this squashed in:
diff --git i/tools/virsh-host.c w/tools/virsh-host.c
index 35e6a2eb98..ad440d5123 100644
--- i/tools/virsh-host.c
+++ w/tools/virsh-host.c
@@ -1447,7 +1447,14 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd
G_GNUC_UNUSED)
vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
major, minor, rel);
-if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) {
+if (virConnectGetVersion(priv->conn, &hvVersion) < 0) {
+if (last_error->code == VIR_ERR_NO_SUPPORT) {
+vshResetLibvirtError();
+} else {
+vshError(ctl, "%s", _("failed to get the hypervisor version"));
+return false;
+}
+} else {
>
>>
>> if (vshCommandOptBool(cmd, "daemon")) {
>> --
>> 2.41.0
>>
>
> Reviewed-by: Peter Krempa
>
Thanks,
Michal
Re: [PATCH 1/2] meson: Annotate each test() with 'suite'
On Tue, Jul 18, 2023 at 14:28:56 +0200, Michal Privoznik wrote: > A test case can be part of a test suite (just like we already > have 'syntax-check'). This then allows developers to run only a > subset of tests. For instance - when using valgrind test setup > (`meson test -C _build/ --setup valgrind`) it makes zero sense to > run syntax-check tests or other script based tests (e.g. > check-augeas-*, check-remote_protocol, etc.). What does makes > sense is to run compiled binaries. > > Strictly speaking, reaching that goal is as trivial as annotating > only those compiled tests (declared in tests/meson.build) and > running them selectively: > > meson test -C _build/ --setup valgrind --suite $TAG > > But it may be also desirable to run test scripts separately. > > Therefore, introduce two new tags: 'bin' for compiled tests, and > 'script' for script based tests and annotate each test() > accordingly. Tbh this annotation feels very arbitrary but since we don't use it and I don't really see us running anything besides unit tests from this repository I don't think we'll need to use this any time soon for anything else. Reviewed-by: Peter Krempa
Re: [PATCH 2/2] tests: Refresh valgrind suppressions
On Tue, Jul 18, 2023 at 14:28:57 +0200, Michal Privoznik wrote: > Since nobody is expected to run valgrind over scripts now, we can > drop plenty of suppressions. Also, there are some old ones that > no longer exist and new ones, that are not covered. > > Signed-off-by: Michal Privoznik > --- > tests/.valgrind.supp | 188 ++- > 1 file changed, 41 insertions(+), 147 deletions(-) Is it really worth keeping this file in the repository? I don't think we can keep it current.
Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE
On Thu, Jun 29, 2023 at 17:13:42 +0200, Michal Privoznik wrote: > Introduced in QEMU commit of v0.14.0-rc0~83^2~1 and not being > able to compile the .removable attribute of the "usb-storage" > object out, renders our corresponding capability > QEMU_CAPS_USB_STORAGE_REMOVABLE useless. Retire it. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_capabilities.c | 9 +- > src/qemu/qemu_capabilities.h | 2 +- > src/qemu/qemu_command.c | 10 +- > src/qemu/qemu_validate.c | 8 - > .../caps_4.2.0_aarch64.replies| 195 -- > .../caps_4.2.0_aarch64.xml| 1 - > .../caps_4.2.0_ppc64.replies | 179 +++- > .../qemucapabilitiesdata/caps_4.2.0_ppc64.xml | 1 - > .../caps_4.2.0_x86_64.replies | 211 +-- > .../caps_4.2.0_x86_64.xml | 1 - > .../caps_5.0.0_aarch64.replies| 211 --- > .../caps_5.0.0_aarch64.xml| 1 - > .../caps_5.0.0_ppc64.replies | 203 -- > .../qemucapabilitiesdata/caps_5.0.0_ppc64.xml | 1 - > .../caps_5.0.0_riscv64.replies| 191 +++-- > .../caps_5.0.0_riscv64.xml| 1 - > .../caps_5.0.0_x86_64.replies | 227 +--- > .../caps_5.0.0_x86_64.xml | 1 - > .../caps_5.1.0_x86_64.replies | 231 +--- > .../caps_5.1.0_x86_64.xml | 1 - > .../caps_5.2.0_aarch64.replies| 219 --- > .../caps_5.2.0_aarch64.xml| 1 - > .../caps_5.2.0_ppc64.replies | 207 -- > .../qemucapabilitiesdata/caps_5.2.0_ppc64.xml | 1 - > .../caps_5.2.0_riscv64.replies| 195 +++--- > .../caps_5.2.0_riscv64.xml| 1 - > .../caps_5.2.0_x86_64.replies | 235 +--- > .../caps_5.2.0_x86_64.xml | 1 - > .../caps_6.0.0_aarch64.replies| 217 --- > .../caps_6.0.0_aarch64.xml| 1 - > .../caps_6.0.0_x86_64.replies | 233 +--- > .../caps_6.0.0_x86_64.xml | 1 - > .../caps_6.1.0_x86_64.replies | 239 +--- > .../caps_6.1.0_x86_64.xml | 1 - > .../caps_6.2.0_aarch64.replies| 223 --- > .../caps_6.2.0_aarch64.xml| 1 - > .../caps_6.2.0_ppc64.replies | 211 --- > .../qemucapabilitiesdata/caps_6.2.0_ppc64.xml | 1 - > .../caps_6.2.0_x86_64.replies | 243 + > .../caps_6.2.0_x86_64.xml | 1 - > .../caps_7.0.0_aarch64+hvf.replies| 227 > .../caps_7.0.0_aarch64+hvf.xml| 1 - > .../caps_7.0.0_aarch64.replies| 227 > .../caps_7.0.0_aarch64.xml| 1 - > .../caps_7.0.0_ppc64.replies | 211 --- > .../qemucapabilitiesdata/caps_7.0.0_ppc64.xml | 1 - > .../caps_7.0.0_x86_64.replies | 243 + > .../caps_7.0.0_x86_64.xml | 1 - > .../caps_7.1.0_ppc64.replies | 211 --- > .../qemucapabilitiesdata/caps_7.1.0_ppc64.xml | 1 - > .../caps_7.1.0_x86_64.replies | 243 + > .../caps_7.1.0_x86_64.xml | 1 - > .../caps_7.2.0_ppc.replies| 211 +++ > tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml | 1 - > .../caps_7.2.0_x86_64+hvf.replies | 255 +- > .../caps_7.2.0_x86_64+hvf.xml | 1 - > .../caps_7.2.0_x86_64.replies | 255 +- > .../caps_7.2.0_x86_64.xml | 1 - > .../caps_8.0.0_riscv64.replies| 207 +++--- > .../caps_8.0.0_riscv64.xml| 1 - > .../caps_8.0.0_x86_64.replies | 255 +- > .../caps_8.0.0_x86_64.xml | 1 - > .../caps_8.1.0_x86_64.replies | 255 +- > .../caps_8.1.0_x86_64.xml | 1 - > 64 files changed, 1704 insertions(+), 5025 deletions(-) Please split this at least into a patch that retires the capability (removes the code), you can include removal of the detection of the capability but usually split that separately too. For those one/two patches you can use: Reviewed-by: Peter Krempa A separate patch then drops the actual probing of the usb-storage qemu device. Additionally consider querying the device regardless. In case when there will need to be another feature to be probed all of the .replies changes will need to be re-added. In c
Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE
On a Thursday in 2023, Michal Privoznik wrote: Introduced in QEMU commit of v0.14.0-rc0~83^2~1 and not being able to compile the .removable attribute of the "usb-storage" object out, renders our corresponding capability QEMU_CAPS_USB_STORAGE_REMOVABLE useless. Retire it. Signed-off-by: Michal Privoznik --- Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 4/5] test_driver: Pass virt_type to virNodeDeviceDefParse() in testNodeDeviceCreateXML()
On Tue, Jul 18, 2023 at 17:27:37 +0200, Michal Privoznik wrote: > This brings the code closer to real implementation: > nodeDeviceCreateXML(). For the unique OUI, let's take the value > from tests/virrandommock.c: 10. > > Signed-off-by: Michal Privoznik > --- > src/test/test_driver.c | 7 +-- > src/util/virrandom.c | 3 +++ > 2 files changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH 5/5] virrandommock: Drop virRandomGenerateWWN
On Tue, Jul 18, 2023 at 17:27:38 +0200, Michal Privoznik wrote: > After previous commit, there's no functional difference between > real virRandomGenerateWWN() and the mocked version. Drop the mock > then. > > Signed-off-by: Michal Privoznik > --- > tests/virrandommock.c | 8 > 1 file changed, 8 deletions(-) Also adjust the following line, as this function isn't being mocked any more: src/util/virrandom.h:int virRandomGenerateWWN(char **wwn, const char *virt_type) G_NO_INLINE; Reviewed-by: Peter Krempa
Re: [PATCH Libvirt v2 1/3] conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes
On Wed, Jul 19, 2023 at 11:52:45 +0800, Yong Huang wrote: > On Tue, Jul 18, 2023 at 9:31 PM Peter Krempa wrote: > > > On Sun, Jul 16, 2023 at 22:36:21 +0800, ~hyman wrote: > > > From: Hyman Huang(黄勇) > > > > > > Add 'virtio_discard' and 'virtio_write_zeroes' attribute to control > > > whether discard and write-zeroes requests are handled by the > > > virtio-blk device. > > > > > > To distinguish the attributes in block drive layer, use the 'virtio' > > > prefix to indicate that these attributes are specific for virtio-blk. > > > > > > Signed-off-by: Hyman Huang(黄勇) > > > --- > > > docs/formatdomain.rst | 8 > > > src/conf/domain_conf.c| 16 > > > src/conf/domain_conf.h| 2 ++ > > > src/conf/schemas/domaincommon.rng | 10 ++ > > > src/conf/storage_source_conf.c| 2 ++ > > > src/conf/storage_source_conf.h| 2 ++ > > > src/qemu/qemu_domain.c| 2 ++ > > > src/qemu/qemu_driver.c| 4 +++- > > > src/vz/vz_utils.c | 12 > > > 9 files changed, 57 insertions(+), 1 deletion(-) > > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > > > index 4af0b82569..7be12ff08e 100644 > > > --- a/docs/formatdomain.rst > > > +++ b/docs/formatdomain.rst > > > @@ -3259,6 +3259,14 @@ paravirtualized driver is specified via the > > ``disk`` element. > > >value can be either "unmap" (allow the discard request to be > > passed) or > > >"ignore" (ignore the discard request). :since:`Since 1.0.6 (QEMU > > and KVM > > >only)` > > > + - The optional ``virtio_discard`` and ``virtio_write_zeroes`` are > > attributes > > > + that control whether discard and write-zeroes requests are > > handled by the > > > + virtio-blk device. The feature is based on DISCARD and > > WRITE_ZEROES > > > + commands introduced in virtio-blk protocol to improve performance > > when > > > + using SSD backend. The value can be either 'on' or 'off'. Note > > that > > > + ``discard`` and ``write_zeroes`` implementations in the block > > drive layer > > > + are parts of the feature logically and should be turned on when > > enabling > > > + the feature. :since:`Since 9.6.0 (QEMU and KVM only)` > > > > Based on current released qemu both 'discard' and 'write-zeroes' feature > > of the 'virtio-blk' device is enabled by default: > > > > $ qemu-system-x86_64 -device virtio-blk-pci,? | grep discard > >discard= - on/off (default: true) > >discard_granularity= - (default: 4294967295) > >max-discard-sectors= - (default: 4194303) > >report-discard-granularity= - (default: true) > > $ qemu-system-x86_64 -device virtio-blk-pci,? | grep write-zeroes > >max-write-zeroes-sectors= - (default: 4194303) > >write-zeroes=- on/off (default: true) > > > > Do you need a way to disable this feature? Based on the description the > > default seems to be sane and actually what you'd want users to set. > > > The default is reasonable indeed. But there are still some scenarios > in the production environment that discard may need to be disabled. > For example: > - The virtio-blk discard/write-zeroes commands was introduced in > 2017 in virtio-blk spec, so the OS distribution before 2017 can not > use this feature. In that case, the cloud management platform(CMP) > could recognize this issue and disable the feature in advance. Older drivers which do not support the feature are supposed to ignore any new feature bits: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-140002 2.2 Feature Bits Each virtio device offers all the features it understands. During device initialization, the driver reads this and tells the device the subset that it accepts. The only way to renegotiate is to reset the device. This allows for forwards and backwards compatibility: if the device is enhanced with a new feature bit, older drivers will not write that feature bit back to the device. Similarly, if a driver is enhanced with a feature that the device doesn’t support, it see the new feature is not offered. Based on the above it's just fine to use a older OS with the new device definition and management doesn't need to do anything to ensure compatibility. > - Discard/write-zeroes may need to be configured at disk granularity > in some scenarios. Assume that CMP support 2 distributed storage > clusters, one cluster supports discard and another does not. > If the VM is configured with 2 disks - vda, vdb. Which are located in > 2 clusters respectively. Or, the VM with disks located in the > discard-supportive cluster and want to migrate disks to another > cluster for some reason(eg. storage capability is exhausted) > CMP may want to turn discard off explicitly. There are two distinct operations which have different requirements and limitations based on the specification: - discard: The v
Re: [PATCH 2/5] virrandom: Accept "nodedev" driver in virRandomGenerateWWN()
On Tue, Jul 18, 2023 at 17:27:35 +0200, Michal Privoznik wrote:
> The virRandomGenerateWWN() is used solely by nodedev driver to
> autogenerate WWNN and WWNP when parsing a nodedev XML. Now, the
> idea was (at least during monolithic daemon) that depending on
> which hypervisor driver called the nodedev XML parsing (and
> virRandomGenerateWWN() under the hood) the corresponding OUI is
> used (e.g. "001a4a" for the QEMU driver).
>
> But in era of split daemons things are not that easy. We do not
> know which hypervisor driver called us. And there might be no
> hypervisor driver at all - users are allowed to connect to
> individual drivers directly (e.g. "nodedev:///system").
>
> In this case, we can't use proper OUI. Well, do the next best
> thing: pick one. By a fair roll of dice the one used by the QEMU
> driver (QUMRANET_OUI) was chosen.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virrandom.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 73c5832a05..7606dd1684 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -138,7 +138,13 @@ virRandomGenerateWWN(char **wwn,
> return -1;
> }
>
> -if (STREQ(virt_type, "QEMU")) {
> +/* In case of split daemon we don't really see the hypervisor
> + * driver that just re-routed the nodedev driver API. There
> + * might not be any hypervisor driver even. Yet, we have to
> + * pick OUI. Pick "QEMU". */
> +
> +if (STREQ(virt_type, "QEMU") ||
> +STREQ(virt_type, "nodedev")) {
> oui = QUMRANET_OUI;
> } else if (STREQ(virt_type, "Xen") ||
> STREQ(virt_type, "xenlight")) {
I at first wanted to suggest to simply drop the code selecting the
prefix based on the hypervisor type, but this would introduce a
behaviour change for any existing monolithic deployment.
Since this simply didn't work with modular daemons until now I guess we
can argue to change the behaviour in the way you propose.
Two other solution I can see in this case are:
- use our own OUI (but we don't have one)
- try to plumb the hypervisor name if possible through a new API
(complex, unclear benefit, doesn't solve all cases anywyas)
>From my side, please drop the commit message bit about it being chosen
by a dice roll. I don't really buy that.
Additionally please allow others to chime in (don't push it right
after):
Reviewed-by: Peter Krempa
Re: [PATCH] Added support for disk model string for QEMU IDE and SATA devices
On Tue, Jul 18, 2023 at 22:39:26 +0200, Benedek Major wrote: > The QEMU drivers ide-hd and ide-cd used for IDE and SATA bus devices support > adding a disk model that is returned on guest query by the IDE or SATA disk > to the guest. > The already existing product tag got changed to allow for up to 40 characters, > which is the maximum allowed by the specification, if the controller is set > to SATA or IDE. > If set, the product gets passed to QEMU using the -model parameter. > The product represents the model until command creation, > where it gets processed depending on the current type of disk controller. > > Dead code checking for QEMU version 1.2.0 got removed, since minimum is 4.2.0. I've posted proper patches to do this part properly as I've originally replied in the review. I'll rebase this patch on top of that series so that it's done properly. I'll also also adjust the commit message to go along with that, so there's no need to resend. > Signed-off-by: Benedek Major > Reviewed-by: Peter Krempa > --- > src/conf/domain_validate.c | 18 +++--- > src/qemu/qemu_command.c| 8 ++-- > src/qemu/qemu_validate.c | 28 +--- > 3 files changed, 34 insertions(+), 20 deletions(-)
Re: [PATCH 3/5] virrandom: Fix printf format string in virRandomGenerateWWN()
On Tue, Jul 18, 2023 at 17:27:36 +0200, Michal Privoznik wrote: > Firstly, drop needless concatenation of two static strings. > Secondly, use proper (portable) formatter for uint64_t so that > typecast to ULL can be dropped. > > Signed-off-by: Michal Privoznik > --- > src/util/virrandom.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH] qemu: Retire QEMU_CAPS_USB_STORAGE_REMOVABLE
On 6/29/23 17:13, Michal Privoznik wrote: > Polite ping. Michal
Re: [PATCH 1/2] NEWS: qemu: Support removable attribute for scsi disk
On 7/17/23 12:56, Han Han wrote: > Signed-off-by: Han Han > --- > NEWS.rst | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index 42c2c53091..5ee880efcb 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -27,6 +27,11 @@ v9.6.0 (unreleased) > ``abstractions/foo`` can be overridden by creating ``local/foo`` and > ``abstractions/foo.d`` respectively. > > + * qemu: Support removable attribute for scsi disk > + > +Now the scsi disk device (``/disk@device='disk'`` and > ``/disk/target@bus='scsi'``) supports Long line. > +the removable attribute at ``/disk/target@removable```. > + > * **Bug fixes** > > Michal
Re: [libvirt PATCH] nodedev: report mdev persistence properly
On 7/18/23 9:55 PM, Jonathon Jongsma wrote: Since commit 44a0f2f0, we now query mdevctl for transient (active) mdevs in order to gather attributes for the mdev. Unfortunately, this commit introduced a regression because nodeDeviceUpdateMediatedDevice() assumed that all mdevs returned from mdevctl were actually persistent mdevs but we were using it to update transient mdevs. Refactor the function so that we can use it to update both persistent and transient mdevs. Signed-off-by: Jonathon Jongsma --- src/node_device/node_device_driver.c | 21 +++-- 1 file changed, 11 insertions(+), 10 deletions(-) LGTM and thanks for fixing it. Reviewed-by: Boris Fiuczynski -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [libvirt PATCH 2/4] Adding Public Get and Set APIs for Network Metadata
On 7/11/23 08:47, K Shiva Kiran wrote:
> This patch introduces public Get and Set APIs for modifying ,
> and elements of the Network object.
>
> - Added enum to select one of the above elements to operate on.
> - Added error code and messages for missing metadata.
> - Added public API implementation.
> - Added driver support.
> - Defined wire protocol format.
>
> Signed-off-by: K Shiva Kiran
> ---
> include/libvirt/libvirt-network.h | 29 ++
> include/libvirt/virterror.h | 1 +
> src/driver-network.h | 16 +++
> src/libvirt-network.c | 167 ++
> src/libvirt_public.syms | 6 ++
> src/remote/remote_driver.c| 2 +
> src/remote/remote_protocol.x | 36 ++-
> src/remote_protocol-structs | 19
> src/util/virerror.c | 3 +
> 9 files changed, 278 insertions(+), 1 deletion(-)
>
We usually introduce public APIs in one patch and implement remote
driver in another. But I guess I can live with this.
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 3c6c230a16..14898a0bc7 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -3130,6 +3130,23 @@ struct remote_network_port_delete_args {
> remote_nonnull_network_port port;
> u_int flags;
> };
> +struct remote_network_set_metadata_args {
> +remote_nonnull_network network;
> +inttype;
> +remote_string metadata;
> +remote_string key;
> +remote_string uri;
> +u_int flags;
> +};
> +struct remote_network_get_metadata_args {
> +remote_nonnull_network network;
> +inttype;
> +remote_string uri;
> +u_int flags;
> +};
> +struct remote_network_get_metadata_ret {
> +remote_nonnull_string metadata;
> +};
This is misplaced.
> struct remote_domain_checkpoint_create_xml_args {
> remote_nonnull_domain dom;
> remote_nonnull_string xml_desc;
> @@ -3717,4 +3734,6 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441,
> REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
> REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
> +REMOTE_PROC_NETWORK_SET_METADATA = 444,
> +REMOTE_PROC_NETWORK_GET_METADATA = 445
Here we want the trailing comma.
> };
Squash in the following:
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 14898a0bc7..c07e0af1e6 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2687,6 +2687,23 @@ struct remote_network_event_lifecycle_msg {
intevent;
intdetail;
};
+struct remote_network_set_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string metadata;
+remote_string key;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_args {
+remote_nonnull_network network;
+inttype;
+remote_string uri;
+u_int flags;
+};
+struct remote_network_get_metadata_ret {
+remote_nonnull_string metadata;
+};
struct remote_connect_storage_pool_event_register_any_args {
inteventID;
remote_storage_poolpool;
@@ -3130,23 +3147,6 @@ struct remote_network_port_delete_args {
remote_nonnull_network_port port;
u_int flags;
};
-struct remote_network_set_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string metadata;
-remote_string key;
-remote_string uri;
-u_int flags;
-};
-struct remote_network_get_metadata_args {
-remote_nonnull_network network;
-inttype;
-remote_string uri;
-u_int flags;
-};
-struct remote_network_get_metadata_ret {
-remote_nonnull_string metadata;
-};
struct remote_domain_checkpoint_create_xml_args {
remote_nonnull_domain dom;
remote_nonnull_string xml_desc;
@@ -3735,5 +3735,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442,
REMOTE_PROC_DOMAIN_FD_ASSOCIATE = 443,
REMOTE_PROC_NETWORK_SET_METADATA = 444,
-REMOTE_PROC_NETWORK_GET_METADATA = 445
+REMOTE_PROC_NETWORK_GET_METADATA = 445,
};
Michal
Re: [libvirt PATCH 0/4] Introduce new Metadata fields for Network object with corresponding APIs
On 7/11/23 08:47, K Shiva Kiran wrote: > This commit introduces and fields to the > XML schema of the Network object. > It also adds public Get/Set APIs for modifying all metadata fields > of the same, along with a test program. > > K Shiva Kiran (4): > Add and for Network Objects > Adding Public Get and Set APIs for Network Metadata > Add virNetworkObj Get and Set Methods for Metadata > Add Test driver and testcase for Network Metadata change APIs > > docs/formatnetwork.rst| 11 + > include/libvirt/libvirt-network.h | 29 +++ > include/libvirt/virterror.h | 1 + > src/conf/network_conf.c | 21 ++ > src/conf/network_conf.h | 2 + > src/conf/schemas/basictypes.rng | 15 ++ > src/conf/schemas/domaincommon.rng | 15 -- > src/conf/schemas/network.rng | 10 + > src/conf/virnetworkobj.c | 325 ++ > src/conf/virnetworkobj.h | 17 ++ > src/driver-network.h | 16 ++ > src/libvirt-network.c | 167 +++ > src/libvirt_public.syms | 6 + > src/remote/remote_driver.c| 2 + > src/remote/remote_protocol.x | 36 +++- > src/remote_protocol-structs | 19 ++ > src/test/test_driver.c| 67 ++ > src/util/virerror.c | 3 + > tests/meson.build | 1 + > tests/networkmetadatatest.c | 297 +++ > 20 files changed, 1044 insertions(+), 16 deletions(-) > create mode 100644 tests/networkmetadatatest.c > No virsh exposure of these new APIs? :-( Michal
Re: [libvirt PATCH 3/4] Add virNetworkObj Get and Set Methods for Metadata
On 7/11/23 08:47, K Shiva Kiran wrote:
> - Introduces virNetworkObjGetMetadata() and
> virNetworkObjSetMetadata().
> - These functions implement common behaviour that can be reused by
> network drivers that use the virNetworkObj struct.
> - Also adds helper functions that resolve the live/persistent state of
> the network before setting metadata.
>
> Signed-off-by: K Shiva Kiran
> ---
> src/conf/virnetworkobj.c | 325 +++
> src/conf/virnetworkobj.h | 17 ++
> 2 files changed, 342 insertions(+)
>
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index b8b86da06f..41d0a1d971 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
>
> return 0;
> }
> +
> +
> +/**
> + * virNetworkObjUpdateModificationImpact:
> + *
> + * @net: network object
> + * @flags: flags to update the modification impact on
> + *
> + * Resolves virNetworkUpdateFlags in @flags so that they correctly
> + * apply to the actual state of @net. @flags may be modified after call to
> this
> + * function.
> + *
> + * Returns 0 on success if @flags point to a valid combination for @net or
> -1 on
> + * error.
> + */
> +static int
> +virNetworkObjUpdateModificationImpact(virNetworkObj *net,
> + unsigned int *flags)
> +{
> +bool isActive = virNetworkObjIsActive(net);
> +
> +if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE |
> VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
> +VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
> +if (isActive)
> +*flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
> +else
> +*flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
> +}
> +
> +if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("network is not running"));
> +return -1;
> +}
> +
> +if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("transient networks do not have any "
> + "persistent config"));
I'm going to mention it here, but it applies to the all of your patches,
and all the new code in fact. We made an exemption for error messages
and thus they don't need to be broken at 80 chars limit. In fact, they
shouldn't. The reason is simple: easier grep as one doesn't have to try
and guess how is the message broken into individual substrings. Of
course, you will find plenty of "bad" examples throughout the code, but
that's because it is an old code. Whenever we rewrite something or
introduce new code this exception applies and the old code is, well,
gradually fixed.
> +return -1;
> +}
> +
> +return 0;
This code is basically the same as in networkUpdate(). The first part
that sets _LIVE or _CONFIG is there verbatim, the second one is hidden
under virNetworkObjConfigChangeSetup(). There's one extra step that the
function does - it calls virNetworkObjSetDefTransient() but I don't
think that's necessary. Either the network is active and
virNetworkObjSetDefTransient() was already called, or is inactive and
thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be
removed.
After this, virNetworkObjUpdateModificationImpact() could be exported
(just like corresponding virDomain* sibling function is) so that it can
be called from both src/conf/virnetworkobj.c and
src/network/bridge_driver.c. Because I think we can get away with the
networkUpdate() doing the following:
networkUpdate() {
...
virNetworkObjUpdateModificationImpact();
if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
/* This is the check that possibly calls
networkRemoveFirewallRules() and sets needFirewallRefresh = true */
}
virNetworkObjUpdate();
...
}
BTW: when cooking the patch, don't forget the same pattern is copied to
our test driver (src/test/test_driver.c).
Michal
Re: [PATCH Libvirt v2 1/3] conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes
On Tue, Jul 18, 2023 at 9:31 PM Peter Krempa wrote: > On Sun, Jul 16, 2023 at 22:36:21 +0800, ~hyman wrote: > > From: Hyman Huang(黄勇) > > > > Add 'virtio_discard' and 'virtio_write_zeroes' attribute to control > > whether discard and write-zeroes requests are handled by the > > virtio-blk device. > > > > To distinguish the attributes in block drive layer, use the 'virtio' > > prefix to indicate that these attributes are specific for virtio-blk. > > > > Signed-off-by: Hyman Huang(黄勇) > > --- > > docs/formatdomain.rst | 8 > > src/conf/domain_conf.c| 16 > > src/conf/domain_conf.h| 2 ++ > > src/conf/schemas/domaincommon.rng | 10 ++ > > src/conf/storage_source_conf.c| 2 ++ > > src/conf/storage_source_conf.h| 2 ++ > > src/qemu/qemu_domain.c| 2 ++ > > src/qemu/qemu_driver.c| 4 +++- > > src/vz/vz_utils.c | 12 > > 9 files changed, 57 insertions(+), 1 deletion(-) > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > > index 4af0b82569..7be12ff08e 100644 > > --- a/docs/formatdomain.rst > > +++ b/docs/formatdomain.rst > > @@ -3259,6 +3259,14 @@ paravirtualized driver is specified via the > ``disk`` element. > >value can be either "unmap" (allow the discard request to be > passed) or > >"ignore" (ignore the discard request). :since:`Since 1.0.6 (QEMU > and KVM > >only)` > > + - The optional ``virtio_discard`` and ``virtio_write_zeroes`` are > attributes > > + that control whether discard and write-zeroes requests are > handled by the > > + virtio-blk device. The feature is based on DISCARD and > WRITE_ZEROES > > + commands introduced in virtio-blk protocol to improve performance > when > > + using SSD backend. The value can be either 'on' or 'off'. Note > that > > + ``discard`` and ``write_zeroes`` implementations in the block > drive layer > > + are parts of the feature logically and should be turned on when > enabling > > + the feature. :since:`Since 9.6.0 (QEMU and KVM only)` > > Based on current released qemu both 'discard' and 'write-zeroes' feature > of the 'virtio-blk' device is enabled by default: > > $ qemu-system-x86_64 -device virtio-blk-pci,? | grep discard >discard= - on/off (default: true) >discard_granularity= - (default: 4294967295) >max-discard-sectors= - (default: 4194303) >report-discard-granularity= - (default: true) > $ qemu-system-x86_64 -device virtio-blk-pci,? | grep write-zeroes >max-write-zeroes-sectors= - (default: 4194303) >write-zeroes=- on/off (default: true) > > Do you need a way to disable this feature? Based on the description the > default seems to be sane and actually what you'd want users to set. > The default is reasonable indeed. But there are still some scenarios in the production environment that discard may need to be disabled. For example: - The virtio-blk discard/write-zeroes commands was introduced in 2017 in virtio-blk spec, so the OS distribution before 2017 can not use this feature. In that case, the cloud management platform(CMP) could recognize this issue and disable the feature in advance. - Discard/write-zeroes may need to be configured at disk granularity in some scenarios. Assume that CMP support 2 distributed storage clusters, one cluster supports discard and another does not. If the VM is configured with 2 disks - vda, vdb. Which are located in 2 clusters respectively. Or, the VM with disks located in the discard-supportive cluster and want to migrate disks to another cluster for some reason(eg. storage capability is exhausted) CMP may want to turn discard off explicitly. Though the above scenarios are quite rare and the virtio spec can ensure the feature can be negotiated and used correctly. CMP still wants to control the features it supports carefully and precisely. To summarise, IMHO, libvirt may not forbid the upper layer to control the 2 features of the virtio-blk disk. Leaving the option configurable for CMP or even customers. There's one more background may need to be stated: Current released QEMU do not provide hmp/qmp to query the final features that are negotiated between front-end and back-end from the hypervisor side. So if CMP wants to query what features a guest VM uses, it has to query it inside the guest VM or hack the qemu process. This way is inconvenient for control-planes, so the CMP needs to control the feature as aggressively as it can. Thank Peter for the attention to this patchset. Yong > > The feature was introduced to qemu by: > > commit 5c81161f804144b146607f890e84613a4cbad95c > Author: Stefano Garzarella > Date: Thu Feb 21 11:33:07 2019 +0100 > > virtio-blk: add "discard" and "write-zeroes" properties > > In order to avoid migration issues, we enable DISCARD and > WRITE_ZEROES features only for machine type >= 4.0 > > As discus
Re: [libvirt PATCH] qemu: require memfd memory for virtio 'blob' support
On 7/18/23 16:48, Jonathon Jongsma wrote:
> The virtio-gpu 'blob' support was insufficiently validated. Qemu
> requires a memfd memory backing in order to use udmabuf and enable blob
> support. Example error:
>
> $ virsh start rhel9
> error: Failed to start domain 'rhel9'
> error: internal error: qemu unexpectedly closed the monitor:
> 2023-07-18T02:33:57.083178Z qemu-kvm: -device
> {"driver":"virtio-vga","id":"video0","max_outputs":1,"blob":true,"bus":"pcie.0","addr":"0x1"}:
> cannot enable blob resources without udmabuf
>
> Signed-off-by: Jonathon Jongsma
> ---
> src/qemu/qemu_validate.c | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
You forgot to update tests. Squash this in:
diff --git i/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
w/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
index ef37e32e5e..577422426b 100644
--- i/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
+++ w/tests/qemuxml2argvdata/video-virtio-blob-on.x86_64-latest.args
@@ -14,7 +14,7 @@
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-accel tcg \
-cpu qemu64 \
-m size=1048576k \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-object
'{"qom-type":"memory-backend-memfd","id":"pc.ram","x-use-canonical-path-for-ramblock-id":false,"size":1073741824}'
\
-overcommit mem-lock=off \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
diff --git i/tests/qemuxml2argvdata/video-virtio-blob-on.xml
w/tests/qemuxml2argvdata/video-virtio-blob-on.xml
index 2b8a913f49..96ccf13079 100644
--- i/tests/qemuxml2argvdata/video-virtio-blob-on.xml
+++ w/tests/qemuxml2argvdata/video-virtio-blob-on.xml
@@ -3,6 +3,9 @@
c7a5fdbd-edaf-9455-926a-d65c16db1809
1048576
1048576
+
+
+
1
hvm
diff --git i/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
w/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
index 410db67592..40f40b4132 100644
--- i/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
+++ w/tests/qemuxml2xmloutdata/video-virtio-blob-on.x86_64-latest.xml
@@ -3,6 +3,9 @@
c7a5fdbd-edaf-9455-926a-d65c16db1809
1048576
1048576
+
+
+
1
hvm
Reviewed-by: Michal Privoznik
Michal
Re: [PATCH Libvirt 1/3] qemu_capabilities: Introduce virtio-blk DISCARD and WRITE_ZEROES capabilities
On 7/17/23 21:43, Jonathon Jongsma wrote:
> I believe that qemu 4.2.0 is the oldest version of qemu that we still
> support, so I don't think that a new capability would actually be
> necessary for this.
Just for completeness, since Hyman is new contributor: this fact
Jonathon mentions is necessary but not sufficient. If a capability
depends on compile time/runtime configuration then we still want to have
capability. For instance, we have QEMU_CAPS_SPICE which tells whether
QEMU was built with SPICE support. Now, SPICE was introduced to QEMU
somewhere in 0.14 (according to [1]) which is way older than 4.2.0 (~8
years difference), and yet we still have a capability for it, because
QEMU can be built without SPICE support.
But this is not the case for '.discard' and '.write-zeroes' attributes
you are introducing as they are declared unconditionally in QEMU code:
hw/block/virtio-blk.c=1706=static Property virtio_blk_properties[] = {
--
hw/block/virtio-blk.c-1725-DEFINE_PROP_BIT64("discard", VirtIOBlock,
host_features,
hw/block/virtio-blk.c-1726- VIRTIO_BLK_F_DISCARD, true),
hw/block/virtio-blk.c-1727-DEFINE_PROP_BOOL("report-discard-granularity",
VirtIOBlock,
hw/block/virtio-blk.c-1728-
conf.report_discard_granularity, true),
hw/block/virtio-blk.c:1729:DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock,
host_features,
hw/block/virtio-blk.c-1730- VIRTIO_BLK_F_WRITE_ZEROES,
true),
and both were introduced in QEMU commit v4.0.0-rc0~80^2~7. IOW, this
capability would be always set and thus is redundant.
BTW: to check the minimal version required by libvirt you can use plain
grep:
libvirt.git $ grep QEMU_MIN_ src/qemu/qemu_capabilities.c
#define QEMU_MIN_MAJOR 4
#define QEMU_MIN_MINOR 2
#define QEMU_MIN_MICRO 0
1: https://wiki.qemu.org/ChangeLog/0.14
Michal
Re: [PATCH 0/2] NEWS update for 9.4 and 9.6
On 7/17/23 12:56, Han Han wrote: > Han Han (2): > NEWS: qemu: Support removable attribute for scsi disk > NEWS: cpu_map: Add SapphireRapids cpu model > > NEWS.rst | 9 + > 1 file changed, 9 insertions(+) > Reviewed-by: Michal Privoznik and pushed. Michal
