Re: [libvirt PATCH] Fix incorrect uses of g_clear_pointer() introduced in 8.1.0

[Ján Tomko]

On a Monday in 2022, Mark Mielke wrote:

Sounds good - thank you...

Patch is

Signed-off-by: Mark Mielke 


Thanks, pushed now.

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] Fix incorrect uses of g_clear_pointer() introduced in 8.1.0

[Mark Mielke]

Sounds good - thank you...

Patch is

Signed-off-by: Mark Mielke 


On Mon, Jun 13, 2022 at 5:08 AM Ján Tomko  wrote:

> On a Sunday in 2022, Mark Mielke wrote:
> >This is a partial revert of 87a43a907f0ad4897a28ad7c216bc70f37270b93.
>
> I'll drop the trailing period before pushing.
>
> >
> >The change to use g_clear_pointer() in more places was accidentally
> >applied to cases involving vir_g_source_unref().
> >
> >In some cases, the ordering of g_source_destroy() and
> >vir_g_source_unref() was reversed, which resulted in the source being
> >marked as destroyed, after it is already unreferenced.
>
> Oops, sorry I missed that during review.
>
> >This
> >use-after-free case might work in many cases, but with versions of
> >glibc older than 2.64.0 it may defer unref to run within the main
>
> s/glibc/glib/
>
> >thread to avoid a race condition, which creates a large distance
> >between the g_source_unref() and g_source_destroy().
> >
> >In some cases, the call to vir_g_source_unref() was replaced with a
> >second call to g_source_destroy(), leading to a memory leak or worse.
> >
> >In our experience, the symptoms were that use of libvirt-python became
> >slower over time, with OpenStack nova-compute initially taking around
> >one second to periodically query the host PCI devices, and within an
> >hour it was taking over a minute to complete the same operation, until
> >it is was eventually running this query back-to-back, resulting in the
> >nova-compute process consuming 100% of one CPU thread, losing its
> >RabbitMQ connection frequently, and showing up as down to the control
> >plane.
>
> Your patch is missing a sign-off
> https://libvirt.org/hacking.html#developer-certificate-of-origin
>
> Just replying to this e-mail with the Signed-off-by line is enough - no
> need to resend the patch. I'll push it with the sign-off included after
> the pipeline succeds:
> https://gitlab.com/janotomko/libvirt/-/pipelines/562139546
>
> >---
> > src/qemu/qemu_agent.c   |  3 ++-
> > src/qemu/qemu_monitor.c |  3 ++-
> > src/util/vireventglib.c | 12 
> > 3 files changed, 12 insertions(+), 6 deletions(-)
> >
>
> Reviewed-by: Ján Tomko 
>
> Jano
>


-- 
Mark Mielke 

Re: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()

[Laine Stump]

On 6/13/22 9:18 AM, Michal Privoznik wrote:

For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.


Wow. This may be in competition for the longest living "how did this 
ever work?" bug in the code :-/


Since my name was on the git blame for the most recent change to this 
line, I had to figure out if it was really me that had 
misunderstood/misused virFileWriteStr() so grievously (wouldn't be the 
first or the last time). What I found was that this code had been moved 
around by multiple different people (including me) since originally 
being included in new code all the way back in commit cbd8227ee in June 
2011.


Anyway, Peter has already acked it, but still

Reviewed-by: Laine Stump 

Re: [PATCH] spec: Xen arches have changed on Fedora 36+

[Cole Robinson]

On 6/13/22 9:56 AM, Erik Skultety wrote:
> On Mon, Jun 13, 2022 at 09:16:09AM -0400, Cole Robinson wrote:
>> On 6/13/22 3:03 AM, Erik Skultety wrote:
>>> On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote:
 Latest fedora 36+ xen builds have dropped i686 and armv7hl builds.

 Signed-off-by: Cole Robinson 
 ---
>>> Reviewed-by: Erik Skultety 
>>>
>>
>> Pushed, but I neglected to run the test suite before submitting, and
>> spec indentation was wrong :/ I pushed a trivial fix
>>
>> commit aabace2aa53b4e792f94a442fe03d3c596350494
>> Author: Cole Robinson 
>> Date:   Mon Jun 13 09:09:35 2022 -0400
>>
>> spec: Fix indentation
>>
>> - Cole
>>
> 
> Weird, I ran the rpmbuild with your patch in the morning hoping that any 
> syntax
> problems would be revealed and it finished just fine.
> 

Yeah rpm build is fine, I verified that before submitting. But I forgot
about the syntax check for prettyifying the spec indentation.

- Cole

Re: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array

[Michal Prívozník]

On 6/13/22 16:04, Peter Krempa wrote:
> On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
>> When creating a TAP interface we can end up with multiple FDs,
>> each representing one queue. However, these FDs must be
>> relabelled as they are then passed to QEMU. In case of
>> qemuBuildInterfaceConnect() we allocate the array for the FDs and
>> then let function corresponding to the  type to fill
>> the array with FDs. When any of the functions meets an error,
>> it's also responsible for closing previously opened FDs. However,
>> the functions take a shortcut: iterate through each member of the
>> array and close it (if it's non-negative). This assumes that the
>> array is initialized to negative values, which use to be the case
>> before rewrite in v8.4.0-rc1~170 but after it it's no longer the
>> case. Subsequently, "random" FDs are closed (okay, not that
>> random since the array is allocated via g_new0(), but hey - FD 0
>> is still valid FD and might be valuable, actually).
>>
>> Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_command.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index be20053c0d..ecfe6020f3 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
>>  size_t tapfdSize = net->driver.virtio.queues;
>>  g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
>>  
>> +memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
>> +
> 
> Reviewed-by: Peter Krempa 
> 
> but I'm adding to my todo list to fix the offending functions :)
> 

Yeah, that might be worth fixing. Pushed thanks.

Michal

[PATCH] kbase: launch_security_sev: Break up overly long line

[Peter Krempa]

Standard text is aligned to 80 colums in all .rst files.

Signed-off-by: Peter Krempa 
---

Pushed as trivial.

 docs/kbase/launch_security_sev.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/kbase/launch_security_sev.rst 
b/docs/kbase/launch_security_sev.rst
index 51b3e14dbf..2734832487 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -295,9 +295,9 @@ In order to make virtio devices work, we need to use
  inside the given device XML element in order
 to enable DMA API in the virtio driver.

-Starting with QEMU 6.0.0 QEMU will set this for us by default. For earlier 
versions though, you will need to explicitly enable this in the device XML as 
follows:
-
-::
+Starting with QEMU 6.0.0 QEMU will set this for us by default. For earlier
+versions though, you will need to explicitly enable this in the device XML as
+follows::

# virsh edit 

-- 
2.36.1

Re: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array

[Peter Krempa]

On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
> When creating a TAP interface we can end up with multiple FDs,
> each representing one queue. However, these FDs must be
> relabelled as they are then passed to QEMU. In case of
> qemuBuildInterfaceConnect() we allocate the array for the FDs and
> then let function corresponding to the  type to fill
> the array with FDs. When any of the functions meets an error,
> it's also responsible for closing previously opened FDs. However,
> the functions take a shortcut: iterate through each member of the
> array and close it (if it's non-negative). This assumes that the
> array is initialized to negative values, which use to be the case
> before rewrite in v8.4.0-rc1~170 but after it it's no longer the
> case. Subsequently, "random" FDs are closed (okay, not that
> random since the array is allocated via g_new0(), but hey - FD 0
> is still valid FD and might be valuable, actually).
> 
> Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index be20053c0d..ecfe6020f3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
>  size_t tapfdSize = net->driver.virtio.queues;
>  g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
>  
> +memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
> +

Reviewed-by: Peter Krempa 

but I'm adding to my todo list to fix the offending functions :)

Re: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()

[Peter Krempa]

On Mon, Jun 13, 2022 at 15:18:13 +0200, Michal Privoznik wrote:
> For some types of SRIOV interfaces we create a temporary file
> where the state of the interface is saved before we start
> modifying it. The file is used then to restore the original
> configuration when the interface is no longer associated with any
> guest. For writing the file virFileWriteStr() is used. However,
> it's given wrong argument: the last argument is supposed to be
> mode to create the file with but virNetDevSaveNetConfig() passes
> open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
> writable and readable by root only (0600). Therefore, pass that
> mode instead of gibberish.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virnetdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 0/2] qemu: virtiofs: add --thread-pool-size option

[Michal Prívozník]

On 6/10/22 15:25, Ján Tomko wrote:
> Ján Tomko (2):
>   conf: virtiofs: add thread_pool element
>   qemu: virtiofs: format --thread-pool-size
> 
>  docs/formatdomain.rst  |  6 ++
>  src/conf/domain_conf.c | 14 ++
>  src/conf/domain_conf.h |  1 +
>  src/conf/schemas/domaincommon.rng  |  9 +
>  src/qemu/qemu_virtiofs.c   |  4 
>  tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml |  1 +
>  6 files changed, 35 insertions(+)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 1/2] conf: virtiofs: add thread_pool element

[Michal Prívozník]

On 6/10/22 15:25, Ján Tomko wrote:
> Add an element to configure the thread pool size:
> 
> ...
> 
>   
> 
> ...
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=2072905
> 
> Signed-off-by: Ján Tomko 
> ---
>  docs/formatdomain.rst  |  6 ++
>  src/conf/domain_conf.c | 14 ++
>  src/conf/domain_conf.h |  1 +
>  src/conf/schemas/domaincommon.rng  |  9 +
>  tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml |  1 +
>  5 files changed, 31 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 7da625380c..e8bff7632a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3329,6 +3329,7 @@ A directory on the host that can be accessed directly 
> from the guest.
>  
>  
>  
> +

I'm not sure this is extensible. I mean  is pretty
specific.  might look better, but then what is
? I don't have any better idea, sorry.

>   
>   
>   
> @@ -3462,6 +3463,11 @@ A directory on the host that can be accessed directly 
> from the guest.
> ``chroot``, see the
> `virtiofsd documentation 
> `__
> for more details. ( :since:`Since 7.2.0` )
> +   Element ``thread_pool`` accepts one attribute ``size`` which defines the
> +   maximum thread pool size. A value of "0" disables the pool.
> +   The thread pool helps increase the number of requests in flight when used 
> with
> +   storage that has a higher latency.  However, it has an overhead, and so 
> for
> +   fast, low latency filesystems, it may be best to turn it off. ( 
> :since:`Since 8.5.0` )
>  ``source``
> The resource on the host that is being accessed in the guest. The ``name``
> attribute must be used with ``type='template'``, and the ``dir`` attribute
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 761c3f4d87..05fac7b39c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2476,6 +2476,8 @@ virDomainFSDefNew(virDomainXMLOption *xmlopt)
>  
>  ret->src = virStorageSourceNew();
>  
> +ret->thread_pool_size = -1;
> +
>  if (xmlopt &&
>  xmlopt->privateData.fsNew &&
>  !(ret->privateData = xmlopt->privateData.fsNew()))
> @@ -9914,6 +9916,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
>  if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
>  g_autofree char *queue_size = 
> virXPathString("string(./driver/@queue)", ctxt);
>  g_autofree char *binary = virXPathString("string(./binary/@path)", 
> ctxt);
> +g_autofree char *thread_pool_size = 
> virXPathString("string(./binary/thread_pool/@size)", ctxt);
>  xmlNodePtr binary_node = virXPathNode("./binary", ctxt);
>  xmlNodePtr binary_lock_node = virXPathNode("./binary/lock", ctxt);
>  xmlNodePtr binary_cache_node = virXPathNode("./binary/cache", ctxt);
> @@ -9926,6 +9929,13 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
>  goto error;
>  }
>  
> +if (thread_pool_size && virStrToLong_i(thread_pool_size, NULL, 10, 
> >thread_pool_size) < 0) {

Very long line.

> +virReportError(VIR_ERR_XML_ERROR,
> +   _("cannot parse thread pool size '%s' for 
> virtiofs"),
> +   queue_size);
> +goto error;
> +}
> +
>  if (binary)
>  def->binary = virFileSanitizePath(binary);
>  
> @@ -24211,6 +24221,10 @@ virDomainFSDefFormat(virBuffer *buf,
>  }
>  
>  virXMLFormatElement(, "lock", , NULL);
> +
> +if (def->thread_pool_size >= 0)
> +virBufferAsprintf(, "\n", 
> def->thread_pool_size);

I believe %d is preferred.

> +
>  }
>  

Michal

[PATCH 6/6] kbase: debuglogs: Add a note about auto-shutdown of daemons

[Peter Krempa]

When using runtime setting of logging with 'virt-admin' it can be
confusing that the settings are discarded when the shutdown timeout of a
daemon is reached.

Add a note about this behaviour along with a suggestion to use
virt-admin to disable the behaviour if needed.

Signed-off-by: Peter Krempa 
---
 docs/kbase/debuglogs.rst | 20 
 1 file changed, 20 insertions(+)

diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst
index 83bc0e6ad7..68b9472a83 100644
--- a/docs/kbase/debuglogs.rst
+++ b/docs/kbase/debuglogs.rst
@@ -194,6 +194,26 @@ package manager provided by your distribution to install 
this package.
 guideline in the sections above in place of ``virt-admin`` in the examples
 below if needed.

+
+Timeout of the configured daemon
+
+
+Common deployments of libvirt start the libvirt daemons via socket activation
+and with automatic shutdown timeout of 120 seconds when no client or object is
+handled by the daemon. When a timeout is reached the daemon stops and all
+settings done during runtime via ``virt-admin`` are discarded. The daemon then
+is re-started with another command.
+
+To prevent auto-shutdown of the daemon you can use the following command::
+
+  virt-admin daemon-timeout 0
+
+The above is introduced in libvirt-8.5.0.
+
+
+Adding filters and outputs
+~~
+
 The following command allows to query the list of currently active log filters:

 ::
-- 
2.36.1

[PATCH 5/6] virt-admin: Introduce 'daemon-timeout'

[Peter Krempa]

Add a simple command to drive the new 'virAdmConnectSetDaemonTimeout'
API.

Signed-off-by: Peter Krempa 
---
 docs/manpages/virt-admin.rst | 12 ++
 tools/virt-admin.c   | 45 
 2 files changed, 57 insertions(+)

diff --git a/docs/manpages/virt-admin.rst b/docs/manpages/virt-admin.rst
index 30cfd24e73..479e27b2c2 100644
--- a/docs/manpages/virt-admin.rst
+++ b/docs/manpages/virt-admin.rst
@@ -313,6 +313,18 @@ To define multiple outputs at once they need to be 
delimited by spaces:

$ virt-admin daemon-log-outputs "4:stderr 2:syslog:"

+daemon-timeout
+--
+
+**Syntax:**
+
+::
+
+   daemon-timeout --timeout NUM
+
+Sets the daemon timeout to the value of '--timeout' argument. Use ``--timeout 
0``
+to disable auto-shutdown of the daemon.
+

 SERVER COMMANDS
 ===
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index e010763e21..718df62854 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1072,6 +1072,45 @@ static const vshCmdInfo info_daemon_log_outputs[] = {
 {.name = NULL}
 };

+static const vshCmdOptDef opts_daemon_timeout[] = {
+{.name = "timeout",
+ .type = VSH_OT_INT,
+ .help = N_("number of seconds the daemon will run without any active 
connection"),
+ .flags = VSH_OFLAG_REQ | VSH_OFLAG_REQ_OPT
+},
+{.name = NULL}
+};
+
+static bool
+cmdDaemonTimeout(vshControl *ctl, const vshCmd *cmd)
+{
+vshAdmControl *priv = ctl->privData;
+unsigned int timeout = 0;
+
+if (vshCommandOptUInt(ctl, cmd, "timeout", ) < 0)
+return false;
+
+if (virAdmConnectSetDaemonTimeout(priv->conn, timeout, 0) < 0)
+return false;
+
+return true;
+}
+
+
+/* --
+ * Command daemon-timeout
+ * --
+ */
+static const vshCmdInfo info_daemon_timeout[] = {
+{.name = "help",
+ .data = N_("set the auto shutdown timeout of the daemon")
+},
+{.name = "desc",
+ .data = N_("set the auto shutdown timeout of the daemon")
+},
+{.name = NULL}
+};
+
 static const vshCmdOptDef opts_daemon_log_outputs[] = {
 {.name = "outputs",
  .type = VSH_OT_STRING,
@@ -1499,6 +1538,12 @@ static const vshCmdDef managementCmds[] = {
  .info = info_daemon_log_outputs,
  .flags = 0
 },
+{.name = "daemon-timeout",
+ .handler = cmdDaemonTimeout,
+ .opts = opts_daemon_timeout,
+ .info = info_daemon_timeout,
+ .flags = 0
+},
 {.name = NULL}
 };

-- 
2.36.1

[PATCH 4/6] admin: Introduce virAdmConnectSetDaemonTimeout

[Peter Krempa]

Use of the admin APIs to modify logging temporarily has a rather serious
deficiency when the daemon whose config is being changed is using
auto-shutdown (default with socket-activated deployments) as the
configuration is discarded if there is no client or VM/other object
blocking auto shutdown.

This API allows users to disable/postpone shutdown timeout so that the
configuration doesn't change under their hands.

Signed-off-by: Peter Krempa 
---
 include/libvirt/libvirt-admin.h |  4 
 src/admin/admin_protocol.x  | 12 +-
 src/admin/admin_server_dispatch.c   | 12 ++
 src/admin/libvirt-admin.c   | 34 +
 src/admin/libvirt_admin_public.syms |  5 +
 src/admin_protocol-structs  |  5 +
 6 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index 8533658932..ae4703f89b 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -480,6 +480,10 @@ int virAdmConnectSetLoggingFilters(virAdmConnectPtr conn,
const char *filters,
unsigned int flags);

+int virAdmConnectSetDaemonTimeout(virAdmConnectPtr conn,
+  unsigned int timeout,
+  unsigned int flags);
+
 # ifdef __cplusplus
 }
 # endif
diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x
index 7dc6724032..f3130efd2d 100644
--- a/src/admin/admin_protocol.x
+++ b/src/admin/admin_protocol.x
@@ -214,6 +214,11 @@ struct admin_connect_set_logging_filters_args {
 unsigned int flags;
 };

+struct admin_connect_set_daemon_timeout_args {
+unsigned int timeout;
+unsigned int flags;
+};
+
 /* Define the program number, protocol version and procedure numbers here. */
 const ADMIN_PROGRAM = 0x06900690;
 const ADMIN_PROTOCOL_VERSION = 1;
@@ -324,5 +329,10 @@ enum admin_procedure {
 /**
  * @generate: both
  */
-ADMIN_PROC_SERVER_UPDATE_TLS_FILES = 18
+ADMIN_PROC_SERVER_UPDATE_TLS_FILES = 18,
+
+/**
+ * @generate: both
+ */
+ADMIN_PROC_CONNECT_SET_DAEMON_TIMEOUT = 19
 };
diff --git a/src/admin/admin_server_dispatch.c 
b/src/admin/admin_server_dispatch.c
index 893c7f1de2..8ab243c8eb 100644
--- a/src/admin/admin_server_dispatch.c
+++ b/src/admin/admin_server_dispatch.c
@@ -466,6 +466,18 @@ adminConnectSetLoggingFilters(virNetDaemon *dmn 
G_GNUC_UNUSED,
 return virLogSetFilters(filters);
 }

+
+static int
+adminConnectSetDaemonTimeout(virNetDaemon *dmn,
+ unsigned int timeout,
+ unsigned int flags)
+{
+virCheckFlags(0, -1);
+
+return virNetDaemonAutoShutdown(dmn, timeout);
+}
+
+
 static int
 adminDispatchConnectGetLoggingOutputs(virNetServer *server G_GNUC_UNUSED,
   virNetServerClient *client G_GNUC_UNUSED,
diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index 5f64784a13..4b5d615e9d 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -1329,3 +1329,37 @@ virAdmConnectSetLoggingFilters(virAdmConnectPtr conn,
 virDispatchError(NULL);
 return -1;
 }
+
+
+/**
+ * virAdmConnectSetDaemonTimeout:
+ * @conn: pointer to an active admin connection
+ * @timeout: timeout to set in seconds (0 disables timeout)
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Reconfigure the existing timeout of the daemon to @timeout. Setting timeout
+ * to 0 disables the daemon timeout.
+ *
+ * Returns 0 on success, -1 on error.
+ *
+ * Since: 8.5.0
+ */
+int
+virAdmConnectSetDaemonTimeout(virAdmConnectPtr conn,
+  unsigned int timeout,
+  unsigned int flags)
+{
+int ret;
+
+VIR_DEBUG("conn=%p, timeout=%u, flags=0x%x", conn, timeout, flags);
+
+virResetLastError();
+virCheckAdmConnectReturn(conn, -1);
+
+if ((ret = remoteAdminConnectSetDaemonTimeout(conn, timeout, flags)) < 0) {
+virDispatchError(NULL);
+return -1;
+}
+
+return ret;
+}
diff --git a/src/admin/libvirt_admin_public.syms 
b/src/admin/libvirt_admin_public.syms
index 8126973e5b..554269613c 100644
--- a/src/admin/libvirt_admin_public.syms
+++ b/src/admin/libvirt_admin_public.syms
@@ -48,3 +48,8 @@ LIBVIRT_ADMIN_3.0.0 {
 virAdmConnectSetLoggingOutputs;
 virAdmConnectSetLoggingFilters;
 } LIBVIRT_ADMIN_2.0.0;
+
+LIBVIRT_ADMIN_8.5.0 {
+global:
+virAdmConnectSetDaemonTimeout;
+} LIBVIRT_ADMIN_3.0.0;
diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs
index 76c511babf..8caac59824 100644
--- a/src/admin_protocol-structs
+++ b/src/admin_protocol-structs
@@ -144,6 +144,10 @@ struct admin_connect_set_logging_filters_args {
 admin_string   filters;
 u_int  flags;
 };
+struct admin_connect_set_daemon_timeout_args {
+  

[PATCH 2/6] virNetDaemonAutoShutdown: Allow live update of shutdown timeout

[Peter Krempa]

Modify the code so that calling 'virNetDaemonAutoShutdown' will update
the auto shutdown timeout also for running daemons.

This involves changing the logic when to do the update of the timer so
that it can be called from both when the daemon is not yet runnign and
when doing a live update.

Signed-off-by: Peter Krempa 
---
 src/locking/lock_daemon.c  |  5 ++---
 src/logging/log_daemon.c   |  5 ++---
 src/remote/remote_daemon.c |  4 ++--
 src/rpc/virnetdaemon.c | 29 +
 src/rpc/virnetdaemon.h |  4 ++--
 5 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 75f6c708db..59c110f62b 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -1051,9 +1051,8 @@ int main(int argc, char **argv) {
 }

 if (timeout > 0) {
-VIR_DEBUG("Registering shutdown timeout %d", timeout);
-virNetDaemonAutoShutdown(lockDaemon->dmn,
- timeout);
+if (virNetDaemonAutoShutdown(lockDaemon->dmn, timeout) < 0)
+goto cleanup;
 }

 if ((virLockDaemonSetupSignals(lockDaemon->dmn)) < 0) {
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 8ab334ff6d..8345d31a8c 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -859,9 +859,8 @@ int main(int argc, char **argv) {
 }

 if (timeout > 0) {
-VIR_DEBUG("Registering shutdown timeout %d", timeout);
-virNetDaemonAutoShutdown(logDaemon->dmn,
- timeout);
+if (virNetDaemonAutoShutdown(logDaemon->dmn, timeout) < 0)
+return -1;
 }

 if ((virLogDaemonSetupSignals(logDaemon->dmn)) < 0) {
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index b8ecc51758..9ad71c37db 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -1126,8 +1126,8 @@ int main(int argc, char **argv) {
 }

 if (timeout > 0) {
-VIR_DEBUG("Registering shutdown timeout %d", timeout);
-virNetDaemonAutoShutdown(dmn, timeout);
+if (virNetDaemonAutoShutdown(dmn, timeout) < 0)
+goto cleanup;
 }

 if ((daemonSetupSignals(dmn)) < 0) {
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 7bf27eed9d..84af1adc06 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -75,6 +75,7 @@ struct _virNetDaemon {
 bool finished;
 bool graceful;
 bool execRestart;
+bool running; /* the daemon has reached the running phase */

 unsigned int autoShutdownTimeout;
 int autoShutdownTimerID;
@@ -424,7 +425,7 @@ virNetDaemonAutoShutdownTimer(int timerid G_GNUC_UNUSED,
 static int
 virNetDaemonShutdownTimerRegister(virNetDaemon *dmn)
 {
-if (dmn->autoShutdownTimeout == 0)
+if (dmn->autoShutdownTimerID != -1)
 return 0;

 if ((dmn->autoShutdownTimerID = virEventAddTimeout(-1,
@@ -442,7 +443,7 @@ virNetDaemonShutdownTimerRegister(virNetDaemon *dmn)
 static void
 virNetDaemonShutdownTimerUpdate(virNetDaemon *dmn)
 {
-if (dmn->autoShutdownTimeout == 0)
+if (dmn->autoShutdownTimerID == -1)
 return;

 /* A shutdown timeout is specified, so check
@@ -450,13 +451,15 @@ virNetDaemonShutdownTimerUpdate(virNetDaemon *dmn)
  * shutdown after timeout seconds
  */
 if (dmn->autoShutdownTimerActive) {
-if (virNetDaemonHasClients(dmn)) {
+if (virNetDaemonHasClients(dmn) ||
+dmn->autoShutdownTimeout == 0) {
 VIR_DEBUG("Deactivating shutdown timer %d", 
dmn->autoShutdownTimerID);
 virEventUpdateTimeout(dmn->autoShutdownTimerID, -1);
 dmn->autoShutdownTimerActive = false;
 }
 } else {
-if (!virNetDaemonHasClients(dmn)) {
+if (!virNetDaemonHasClients(dmn) ||
+dmn->autoShutdownTimeout != 0) {
 VIR_DEBUG("Activating shutdown timer %d", 
dmn->autoShutdownTimerID);
 virEventUpdateTimeout(dmn->autoShutdownTimerID,
   dmn->autoShutdownTimeout * 1000);
@@ -466,13 +469,25 @@ virNetDaemonShutdownTimerUpdate(virNetDaemon *dmn)
 }


-void
+int
 virNetDaemonAutoShutdown(virNetDaemon *dmn,
  unsigned int timeout)
 {
 VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

+VIR_DEBUG("Registering shutdown timeout %u", timeout);
+
+if (timeout > 0) {
+if (virNetDaemonShutdownTimerRegister(dmn) < 0)
+return -1;
+}
+
 dmn->autoShutdownTimeout = timeout;
+
+if (dmn->running)
+virNetDaemonShutdownTimerUpdate(dmn);
+
+return 0;
 }


@@ -811,9 +826,7 @@ virNetDaemonRun(virNetDaemon *dmn)
 dmn->finishTimer = -1;
 dmn->finished = false;
 dmn->graceful = false;
-
-if (virNetDaemonShutdownTimerRegister(dmn) < 0)
-goto cleanup;
+dmn->running = true;

 /* We are accepting connections now. Notify systemd
  * so it can start dependent 

[PATCH 1/6] virnetdaemon: Extract autoShutdownTimer operations from virNetDaemonRun

[Peter Krempa]

Introduce 'virNetDaemonShutdownTimerRegister' and
'virNetDaemonShutdownTimerUpdate' to aggregate the code to deal with the
auto-shutdown timer.

The code is also placed so that it can be called from
'virNetDaemonAutoShutdown' which involved the move of
'virNetDaemonAutoShutdownTimer'.

Signed-off-by: Peter Krempa 
---
 src/rpc/virnetdaemon.c | 108 +
 1 file changed, 66 insertions(+), 42 deletions(-)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 1f32bae35f..7bf27eed9d 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -77,6 +77,8 @@ struct _virNetDaemon {
 bool execRestart;

 unsigned int autoShutdownTimeout;
+int autoShutdownTimerID;
+bool autoShutdownTimerActive;
 size_t autoShutdownInhibitions;
 int autoShutdownInhibitFd;
 };
@@ -153,6 +155,8 @@ virNetDaemonNew(void)
 if (virEventRegisterDefaultImpl() < 0)
 goto error;

+dmn->autoShutdownTimerID = -1;
+
 #ifndef WIN32
 memset(_action, 0, sizeof(sig_action));
 sig_action.sa_handler = SIG_IGN;
@@ -403,6 +407,65 @@ virNetDaemonIsPrivileged(virNetDaemon *dmn)
 }


+static void
+virNetDaemonAutoShutdownTimer(int timerid G_GNUC_UNUSED,
+  void *opaque)
+{
+virNetDaemon *dmn = opaque;
+VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
+
+if (!dmn->autoShutdownInhibitions) {
+VIR_DEBUG("Automatic shutdown triggered");
+dmn->quit = true;
+}
+}
+
+
+static int
+virNetDaemonShutdownTimerRegister(virNetDaemon *dmn)
+{
+if (dmn->autoShutdownTimeout == 0)
+return 0;
+
+if ((dmn->autoShutdownTimerID = virEventAddTimeout(-1,
+   
virNetDaemonAutoShutdownTimer,
+   dmn, NULL)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Failed to register shutdown timeout"));
+return -1;
+}
+
+return 0;
+}
+
+
+static void
+virNetDaemonShutdownTimerUpdate(virNetDaemon *dmn)
+{
+if (dmn->autoShutdownTimeout == 0)
+return;
+
+/* A shutdown timeout is specified, so check
+ * if any drivers have active state, if not
+ * shutdown after timeout seconds
+ */
+if (dmn->autoShutdownTimerActive) {
+if (virNetDaemonHasClients(dmn)) {
+VIR_DEBUG("Deactivating shutdown timer %d", 
dmn->autoShutdownTimerID);
+virEventUpdateTimeout(dmn->autoShutdownTimerID, -1);
+dmn->autoShutdownTimerActive = false;
+}
+} else {
+if (!virNetDaemonHasClients(dmn)) {
+VIR_DEBUG("Activating shutdown timer %d", 
dmn->autoShutdownTimerID);
+virEventUpdateTimeout(dmn->autoShutdownTimerID,
+  dmn->autoShutdownTimeout * 1000);
+dmn->autoShutdownTimerActive = true;
+}
+}
+}
+
+
 void
 virNetDaemonAutoShutdown(virNetDaemon *dmn,
  unsigned int timeout)
@@ -657,19 +720,6 @@ virNetDaemonAddSignalHandler(virNetDaemon *dmn 
G_GNUC_UNUSED,
 #endif /* WIN32 */


-static void
-virNetDaemonAutoShutdownTimer(int timerid G_GNUC_UNUSED,
-  void *opaque)
-{
-virNetDaemon *dmn = opaque;
-VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
-
-if (!dmn->autoShutdownInhibitions) {
-VIR_DEBUG("Automatic shutdown triggered");
-dmn->quit = true;
-}
-}
-
 static int
 daemonServerUpdateServices(void *payload,
const char *key G_GNUC_UNUSED,
@@ -743,11 +793,10 @@ virNetDaemonFinishTimer(int timerid G_GNUC_UNUSED,
 dmn->finished = true;
 }

+
 void
 virNetDaemonRun(virNetDaemon *dmn)
 {
-int timerid = -1;
-bool timerActive = false;
 virThread shutdownThread;

 virObjectLock(dmn);
@@ -763,14 +812,8 @@ virNetDaemonRun(virNetDaemon *dmn)
 dmn->finished = false;
 dmn->graceful = false;

-if (dmn->autoShutdownTimeout &&
-(timerid = virEventAddTimeout(-1,
-  virNetDaemonAutoShutdownTimer,
-  dmn, NULL)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Failed to register shutdown timeout"));
+if (virNetDaemonShutdownTimerRegister(dmn) < 0)
 goto cleanup;
-}

 /* We are accepting connections now. Notify systemd
  * so it can start dependent services. */
@@ -778,26 +821,7 @@ virNetDaemonRun(virNetDaemon *dmn)

 VIR_DEBUG("dmn=%p quit=%d", dmn, dmn->quit);
 while (!dmn->finished) {
-/* A shutdown timeout is specified, so check
- * if any drivers have active state, if not
- * shutdown after timeout seconds
- */
-if (dmn->autoShutdownTimeout) {
-if (timerActive) {
-if (virNetDaemonHasClients(dmn)) {
-VIR_DEBUG("Deactivating shutdown timer %d", timerid);
-

[PATCH 3/6] scripts: apibuild: Improve error when API is missing from symbol file

[Peter Krempa]

Improve:

 KeyError: 'virAdmConnectSetDaemonTimeout'

to

 Exception: Missing symbol file entry for 'virAdmConnectSetDaemonTimeout'

Signed-off-by: Peter Krempa 
---
 scripts/apibuild.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index 23a66734ac..c232b4e2c8 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2405,6 +2405,8 @@ class docBuilder:

 # NB: this is consumed by a regex in 'getAPIFilenames' in hvsupport.pl
 if id.type == "function":
+if name not in self.versions:
+raise Exception("Missing symbol file entry for '%s'" % name)
 ver = self.versions[name]
 if ver is None:
 raise Exception("Missing version for '%s'" % name)
-- 
2.36.1

[PATCH 0/6] Improve setting of logging with 'virt-admin' when daemon uses auto-shutdown

[Peter Krempa]

Allow setting of the timeout so that users can bypass it when they are
setting runtime-logging.

Update the docs to mention the caveat and suggest the bypass.

Peter Krempa (6):
  virnetdaemon: Extract autoShutdownTimer operations from
virNetDaemonRun
  virNetDaemonAutoShutdown: Allow live update of shutdown timeout
  scripts: apibuild: Improve error when API is missing from symbol file
  admin: Introduce virAdmConnectSetDaemonTimeout
  virt-admin: Introduce 'daemon-timeout'
  kbase: debuglogs: Add a note about auto-shutdown of daemons

 docs/kbase/debuglogs.rst|  20 +
 docs/manpages/virt-admin.rst|  12 +++
 include/libvirt/libvirt-admin.h |   4 +
 scripts/apibuild.py |   2 +
 src/admin/admin_protocol.x  |  12 ++-
 src/admin/admin_server_dispatch.c   |  12 +++
 src/admin/libvirt-admin.c   |  34 
 src/admin/libvirt_admin_public.syms |   5 ++
 src/admin_protocol-structs  |   5 ++
 src/locking/lock_daemon.c   |   5 +-
 src/logging/log_daemon.c|   5 +-
 src/remote/remote_daemon.c  |   4 +-
 src/rpc/virnetdaemon.c  | 127 ++--
 src/rpc/virnetdaemon.h  |   4 +-
 tools/virt-admin.c  |  45 ++
 15 files changed, 240 insertions(+), 56 deletions(-)

-- 
2.36.1

Re: [PATCH] spec: Xen arches have changed on Fedora 36+

[Erik Skultety]

On Mon, Jun 13, 2022 at 09:16:09AM -0400, Cole Robinson wrote:
> On 6/13/22 3:03 AM, Erik Skultety wrote:
> > On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote:
> >> Latest fedora 36+ xen builds have dropped i686 and armv7hl builds.
> >>
> >> Signed-off-by: Cole Robinson 
> >> ---
> > Reviewed-by: Erik Skultety 
> > 
> 
> Pushed, but I neglected to run the test suite before submitting, and
> spec indentation was wrong :/ I pushed a trivial fix
> 
> commit aabace2aa53b4e792f94a442fe03d3c596350494
> Author: Cole Robinson 
> Date:   Mon Jun 13 09:09:35 2022 -0400
> 
> spec: Fix indentation
> 
> - Cole
> 

Weird, I ran the rpmbuild with your patch in the morning hoping that any syntax
problems would be revealed and it finished just fine.

Erik

Re: [PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array

[Michal Prívozník]

On 6/13/22 15:18, Michal Privoznik wrote:
> When creating a TAP interface we can end up with multiple FDs,
> each representing one queue. However, these FDs must be
> relabelled as they are then passed to QEMU. In case of
> qemuBuildInterfaceConnect() we allocate the array for the FDs and
> then let function corresponding to the  type to fill
> the array with FDs. When any of the functions meets an error,
> it's also responsible for closing previously opened FDs. However,
> the functions take a shortcut: iterate through each member of the
> array and close it (if it's non-negative). This assumes that the
> array is initialized to negative values, which use to be the case
> before rewrite in v8.4.0-rc1~170 but after it it's no longer the
> case. Subsequently, "random" FDs are closed (okay, not that
> random since the array is allocated via g_new0(), but hey - FD 0
> is still valid FD and might be valuable, actually).
> 
> Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
> Signed-off-by: Michal Privoznik 

After some discussion in BZ I'm going to append:

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075383#c18

Michal

[PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()

[Michal Privoznik]

For some types of SRIOV interfaces we create a temporary file
where the state of the interface is saved before we start
modifying it. The file is used then to restore the original
configuration when the interface is no longer associated with any
guest. For writing the file virFileWriteStr() is used. However,
it's given wrong argument: the last argument is supposed to be
mode to create the file with but virNetDevSaveNetConfig() passes
open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be
writable and readable by root only (0600). Therefore, pass that
mode instead of gibberish.

Signed-off-by: Michal Privoznik 
---
 src/util/virnetdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 93f836cd92..6c6f6dee42 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1977,7 +1977,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
 if (!(fileStr = virJSONValueToString(configJSON, true)))
 return -1;
 
-if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
+if (virFileWriteStr(filePath, fileStr, 0600) < 0) {
 virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
   "for device = %s, vf = %d"), linkdev, 
vf);
 return -1;
-- 
2.35.1

[PATCH 0/2] Two related fixes

[Michal Privoznik]

Except not really. Only the first one is strictly related to that type
of . The other one fixes a regression introduced in 8.4.0
and it just so happens that I'm able to reproduce it 100% on my
(SRIOV-less) machine.

Michal Prívozník (2):
  virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
  qemuBuildInterfaceConnect: Initialize @tapfd array

 src/qemu/qemu_command.c | 2 ++
 src/util/virnetdev.c| 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.35.1

[PATCH 2/2] qemuBuildInterfaceConnect: Initialize @tapfd array

[Michal Privoznik]

When creating a TAP interface we can end up with multiple FDs,
each representing one queue. However, these FDs must be
relabelled as they are then passed to QEMU. In case of
qemuBuildInterfaceConnect() we allocate the array for the FDs and
then let function corresponding to the  type to fill
the array with FDs. When any of the functions meets an error,
it's also responsible for closing previously opened FDs. However,
the functions take a shortcut: iterate through each member of the
array and close it (if it's non-negative). This assumes that the
array is initialized to negative values, which use to be the case
before rewrite in v8.4.0-rc1~170 but after it it's no longer the
case. Subsequently, "random" FDs are closed (okay, not that
random since the array is allocated via g_new0(), but hey - FD 0
is still valid FD and might be valuable, actually).

Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index be20053c0d..ecfe6020f3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm,
 size_t tapfdSize = net->driver.virtio.queues;
 g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
 
+memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd));
+
 if (tapfdSize == 0)
 tapfdSize = 1;
 
-- 
2.35.1

Re: [PATCH] spec: Xen arches have changed on Fedora 36+

[Cole Robinson]

On 6/13/22 3:03 AM, Erik Skultety wrote:
> On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote:
>> Latest fedora 36+ xen builds have dropped i686 and armv7hl builds.
>>
>> Signed-off-by: Cole Robinson 
>> ---
> Reviewed-by: Erik Skultety 
> 

Pushed, but I neglected to run the test suite before submitting, and
spec indentation was wrong :/ I pushed a trivial fix

commit aabace2aa53b4e792f94a442fe03d3c596350494
Author: Cole Robinson 
Date:   Mon Jun 13 09:09:35 2022 -0400

spec: Fix indentation

- Cole

Re: [PATCH] docs: kbase/launch_security_sev: QEMU 6.0+ sets iommu=on for us

[Cole Robinson]

On 6/13/22 2:56 AM, Erik Skultety wrote:
> On Sat, Jun 11, 2022 at 12:44:54PM -0400, Cole Robinson wrote:
>> Signed-off-by: Cole Robinson 
>> ---
>>  docs/kbase/launch_security_sev.rst | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/docs/kbase/launch_security_sev.rst 
>> b/docs/kbase/launch_security_sev.rst
>> index 3ebb01ad80..9f6330a1ca 100644
>> --- a/docs/kbase/launch_security_sev.rst
>> +++ b/docs/kbase/launch_security_sev.rst
>> @@ -295,6 +295,8 @@ In order to make virtio devices work, we need to use
>>   inside the given device XML element in order
>>  to enable DMA API in the virtio driver.
>>  
>> +QEMU 6.0 and later will `set this by default 
>> `__. For earlier 
>> QEMU versions, you will need to explicitly enable this in the device XML:
> 
> Do we need to link the specific commit in the kbase article? I think simply
> saying (in the same paragrap)
> 
> "... enable DMA API in the virtio driver. Starting with QEMU 6.0.0 QEMU will
> set this for us by default. For earlier versions though, you will need to
> explicitly enable this in the device XML as follows:"
> 
> Reviewed-by: Erik Skultety 

Thanks, I pushed with your suggested wording

- Cole

Re: [libvirt PATCH] Fix incorrect uses of g_clear_pointer() introduced in 8.1.0

[Ján Tomko]

On a Sunday in 2022, Mark Mielke wrote:

This is a partial revert of 87a43a907f0ad4897a28ad7c216bc70f37270b93.


I'll drop the trailing period before pushing.



The change to use g_clear_pointer() in more places was accidentally
applied to cases involving vir_g_source_unref().

In some cases, the ordering of g_source_destroy() and
vir_g_source_unref() was reversed, which resulted in the source being
marked as destroyed, after it is already unreferenced.


Oops, sorry I missed that during review.


This
use-after-free case might work in many cases, but with versions of
glibc older than 2.64.0 it may defer unref to run within the main


s/glibc/glib/


thread to avoid a race condition, which creates a large distance
between the g_source_unref() and g_source_destroy().

In some cases, the call to vir_g_source_unref() was replaced with a
second call to g_source_destroy(), leading to a memory leak or worse.

In our experience, the symptoms were that use of libvirt-python became
slower over time, with OpenStack nova-compute initially taking around
one second to periodically query the host PCI devices, and within an
hour it was taking over a minute to complete the same operation, until
it is was eventually running this query back-to-back, resulting in the
nova-compute process consuming 100% of one CPU thread, losing its
RabbitMQ connection frequently, and showing up as down to the control
plane.


Your patch is missing a sign-off
https://libvirt.org/hacking.html#developer-certificate-of-origin

Just replying to this e-mail with the Signed-off-by line is enough - no
need to resend the patch. I'll push it with the sign-off included after
the pipeline succeds: https://gitlab.com/janotomko/libvirt/-/pipelines/562139546


---
src/qemu/qemu_agent.c   |  3 ++-
src/qemu/qemu_monitor.c |  3 ++-
src/util/vireventglib.c | 12 
3 files changed, 12 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] spec: Xen arches have changed on Fedora 36+

[Erik Skultety]

On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote:
> Latest fedora 36+ xen builds have dropped i686 and armv7hl builds.
> 
> Signed-off-by: Cole Robinson 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH] docs: kbase/launch_security_sev: QEMU 6.0+ sets iommu=on for us

[Erik Skultety]

On Sat, Jun 11, 2022 at 12:44:54PM -0400, Cole Robinson wrote:
> Signed-off-by: Cole Robinson 
> ---
>  docs/kbase/launch_security_sev.rst | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/docs/kbase/launch_security_sev.rst 
> b/docs/kbase/launch_security_sev.rst
> index 3ebb01ad80..9f6330a1ca 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -295,6 +295,8 @@ In order to make virtio devices work, we need to use
>   inside the given device XML element in order
>  to enable DMA API in the virtio driver.
>  
> +QEMU 6.0 and later will `set this by default 
> `__. For earlier 
> QEMU versions, you will need to explicitly enable this in the device XML:

Do we need to link the specific commit in the kbase article? I think simply
saying (in the same paragrap)

"... enable DMA API in the virtio driver. Starting with QEMU 6.0.0 QEMU will
set this for us by default. For earlier versions though, you will need to
explicitly enable this in the device XML as follows:"

Reviewed-by: Erik Skultety 

Regards,
Erik