On 5/21/26 10:39, Michal Prívozník wrote:
On 5/20/26 16:14, Lucas Kornicki wrote:
On 5/20/26 12:41, Michal Prívozník wrote:
On 5/19/26 15:11, Lucas Kornicki wrote:
Emit the channel lifecycle event on VSERPORT_CHANGE
and when refreshing virtio state.
On "org.qemu.guest_agent.0" channel state change both
agent and channel lifecycle events are emitted in that order.
Signed-off-by: Lucas Kornicki<[email protected]>
---
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_process.c | 28 ++++++++++++++++++++++------
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index eda1f42054..d260c1cc74 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3906,6 +3906,14 @@ processSerialChangedEvent(virQEMUDriver *driver,
virObjectEventStateQueue(driver->domainEventState, event);
}
+ /* we deliberately allow for goto endjob to skip generic event emission
+ * to ensure identical semantics for "org.qemu.guest_agent.0" */
+ event = virDomainEventChannelLifecycleNewFromObj(vm,
I'd rather avoid reusing the same variable. What can be utilized is the
fact that agent event is created and queued in a different code block,
i.e. in there new variable can be declared (say agentEvent) leaving this
'event' var free for this use. Or use an array, like you do in the hunk
below.
Personally I'm fine with either, but it was Peter's suggestion to reuse
the event variable.
This should have been marked as v2. Sorry for the commotion
Ah, it was to avoid long lines. I hear what he says, but at the same
time I think only a handful of variables should be reused (maybe some
int indexes if there are two loops). But I can live with this.
+ dev.data.chr->target.name,
+ newstate,
+
VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL);
+ virObjectEventStateQueue(driver->domainEventState, event);
+
endjob:
virDomainObjEndJob(vm);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 83f5ebb19c..38c1fa05ce 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2253,7 +2253,7 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriver
*driver,
size_t i;
int agentReason = VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL;
qemuMonitorChardevInfo *entry;
- virObjectEvent *event = NULL;
+ virObjectEvent *events[2];
I'd rather have these initialized. Dangling pointers are a problem
waiting to happen.
I've decided to drop the initialization of `events` given how they're
used and given that `entry` above is not initialized either.
True and this is a good example of how our coding standard evolves. I
mean, we had one way of writing things, but then evolved to slightly
different one (in this specific case influenced by introduction of
automatic memory freeing - g_autofree) but never went through the code
and fixed every single occurrence of the old one.
g_autofree char *id = NULL;
if (booted)
@@ -2271,11 +2271,27 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriver
*driver,
!entry->state)
continue;
- if (entry->state != VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT &&
- STREQ_NULLABLE(chr->target.name, "org.qemu.guest_agent.0") &&
- (event = virDomainEventAgentLifecycleNewFromObj(vm,
entry->state,
- agentReason)))
- virObjectEventStateQueue(driver->domainEventState, event);
+ if (entry->state != VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT) {
+ events[0] = virDomainEventChannelLifecycleNewFromObj(vm,
+
chr->target.name,
+
entry->state,
+
agentReason);
+ if (STREQ_NULLABLE(chr->target.name,
"org.qemu.guest_agent.0")) {
+ events[1] = virDomainEventAgentLifecycleNewFromObj(vm,
+
entry->state,
+
agentReason);
+ } else {
+ events[1] = NULL;
+ }
+
+ /* emit agent then channel when emitting both events */
+ if (events[0] && events[1]) {
+ virObjectEventStateQueue(driver->domainEventState,
events[1]);
+ virObjectEventStateQueue(driver->domainEventState,
events[0]);
+ } else if (events[0]) {
+ virObjectEventStateQueue(driver->domainEventState,
events[0]);
+ }
No need to check for NULL, virObjectEventStateQueue() is a NOP when
event is NULL.
So you'd want to drop the `if (events[0])` and leave just the `else`? Or
drop the `else` altogether?
I see no harm in keeping it this way, but I'll defer to your judgement.
I'd just keep queuing both events and drop else branch. Even in the
first hunk (or pre-existing code) there's no NULL check. It's simply:
event = virDomainEventXXXNewFromObj(...);
virObjectEventStateQueue(..., event);
And all of virDomainEventXXXNewFromObj() can fail and return NULL.
Michal
Ah I think I remember why I've done it this way.
If we just queue events[1] and events[0] there is a possibility that we
only end up sending the agent event if construction of channel event fails.
I don't think this is much of a practical concern as if NewFromObj
fails, it will likely fail for both events. At least with explicit
control flow it will be evident what the intention was.
If we don't care about this sort of consistency we can rely on NOP
queuing behavior. Thoughts?
Do you want me to send in a v3, or will you apply the changes when merging?
