When starting a VM we must assign unique IDs for fdsets we add via
'-add-fd'. For now it was done by using the index of the filedescriptor
passed to the virCommand. That approach is not very flexible, because
you need to have already passed the 'fd' to virCommand before generating
the fdset path, and also won't nicely work with fdsets containing two or
more fds.

This patch introduces a counter into the private data of a qemu domain
so that we can allocate unique ids without relying on virCommand.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
 src/qemu/qemu_domain.h |  4 ++++
 2 files changed, 33 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 64be39d49f..d0abf76e4a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv)
 }


+/**
+ * qemuDomainFDSetIdNew:
+ * @priv: qemu VM private data object.
+ *
+ * Generate a new unique id for a fdset. Note that this is necessary only for
+ * startup. When running qemu auto-assigns id for added fdset.
+ */
+unsigned int
+qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv)
+{
+    return ++priv->fdsetindex;
+}
+
+
+/**
+ * qemuDomainFDSetIdReset:
+ * @priv: qemu VM private data object.
+ *
+ * Resets the data for the fdset ID generator.
+ */
+static void
+qemuDomainFDSetIdReset(qemuDomainObjPrivate *priv)
+{
+    priv->fdsetindex = 0;
+}
+
+
 static void
 qemuDomainSecretInfoClear(qemuDomainSecretInfo *secinfo,
                           bool keepAlias)
@@ -1683,6 +1710,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivate *priv)
     /* reset node name allocator */
     qemuDomainStorageIdReset(priv);

+    qemuDomainFDSetIdReset(priv);
+
     priv->dbusDaemonRunning = false;

     if (priv->dbusVMStateIds)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7ec337e9ae..b7d0f5d2d5 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -207,6 +207,9 @@ struct _qemuDomainObjPrivate {
     /* counter for generating node names for qemu disks */
     unsigned long long nodenameindex;

+    /* counter for generating IDs of fdsets - only relevant during startup */
+    unsigned int fdsetindex;
+
     /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the
      * RESUME event handler to use it */
     virDomainRunningReason runningReason;
@@ -970,6 +973,7 @@ char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivate *priv);
 bool qemuDomainDefHasManagedPR(virDomainObj *vm);

 unsigned int qemuDomainStorageIdNew(qemuDomainObjPrivate *priv);
+unsigned int qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv);

 virDomainEventResumedDetailType
 qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason);
-- 
2.34.1

On a Wednesday in 2022, Peter Krempa wrote:
>When starting a VM we must assign unique IDs for fdsets we add via
>'-add-fd'. For now it was done by using the index of the filedescriptor
>passed to the virCommand. That approach is not very flexible, because
>you need to have already passed the 'fd' to virCommand before generating
>the fdset path, and also won't nicely work with fdsets containing two or
>more fds.
>
>This patch introduces a counter into the private data of a qemu domain
>so that we can allocate unique ids without relying on virCommand.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h |  4 ++++
> 2 files changed, 33 insertions(+)
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 64be39d49f..d0abf76e4a 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv)
> }
>
>
>+/**
>+ * qemuDomainFDSetIdNew:
>+ * @priv: qemu VM private data object.
>+ *
>+ * Generate a new unique id for a fdset. Note that this is necessary only for
>+ * startup. When running qemu auto-assigns id for added fdset.
>+ */
>+unsigned int
>+qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv)

The spelling of 'ID' would look nicer.

>+{
>+    return ++priv->fdsetindex;

Is there a reason why we shouldn't allow '0' as a valid fdset index?

It would reduce test churn.

Jano

>+}
>+
>+

On Thu, Feb 10, 2022 at 18:53:43 +0100, Ján Tomko wrote:
> On a Wednesday in 2022, Peter Krempa wrote:
> > When starting a VM we must assign unique IDs for fdsets we add via
> > '-add-fd'. For now it was done by using the index of the filedescriptor
> > passed to the virCommand. That approach is not very flexible, because
> > you need to have already passed the 'fd' to virCommand before generating
> > the fdset path, and also won't nicely work with fdsets containing two or
> > more fds.
> > 
> > This patch introduces a counter into the private data of a qemu domain
> > so that we can allocate unique ids without relying on virCommand.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
> > src/qemu/qemu_domain.h |  4 ++++
> > 2 files changed, 33 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 64be39d49f..d0abf76e4a 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv)
> > }
> > 
> > 
> > +/**
> > + * qemuDomainFDSetIdNew:
> > + * @priv: qemu VM private data object.
> > + *
> > + * Generate a new unique id for a fdset. Note that this is necessary only for
> > + * startup. When running qemu auto-assigns id for added fdset.
> > + */
> > +unsigned int
> > +qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv)
> 
> The spelling of 'ID' would look nicer.

Hmm, okay, I'll thus also fix
qemuDomainStorageIdNew/qemuDomainStorageIdReset separately.

> 
> > +{
> > +    return ++priv->fdsetindex;
> 
> Is there a reason why we shouldn't allow '0' as a valid fdset index?

Not really. FDset ID '0' is valid in qemu. Starting from 1 has the nice
side effect of seeing that this counter was used, but that's not very
helpful and we have few tests adding multiple fdsets where it's possible
to see too, so I'll change it.

On a Wednesday in 2022, Peter Krempa wrote:
>When starting a VM we must assign unique IDs for fdsets we add via
>'-add-fd'. For now it was done by using the index of the filedescriptor
>passed to the virCommand. That approach is not very flexible, because
>you need to have already passed the 'fd' to virCommand before generating
>the fdset path, and also won't nicely work with fdsets containing two or
>more fds.
>
>This patch introduces a counter into the private data of a qemu domain
>so that we can allocate unique ids without relying on virCommand.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h |  4 ++++
> 2 files changed, 33 insertions(+)

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano