On Fri, Nov 11, 2016 at 5:13 AM, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
Presently domain events are emitted for the "agent lifecycle" which is a specialization on virtio-serial events specific to the channel named "org.qemu.guest_agent.0". This patch adds a generic event for "channel lifecycles", which emit state change events for all attached channels. --- daemon/remote.c | 42 +++++++++++++++++ examples/object-events/event-test.c | 57 ++++++++++++++++++++++++ include/libvirt/libvirt-domain.h | 41 +++++++++++++++++ src/conf/domain_event.c | 89 +++++++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 12 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 5 +++ src/qemu/qemu_process.c | 6 +++ src/remote/remote_driver.c | 32 +++++++++++++ src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 7 +++ tools/virsh-domain.c | 35 +++++++++++++++ 12 files changed, 344 insertions(+), 1 deletion(-)
[...]
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5f50660..d720132 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3965,6 +3965,46 @@ typedef void (*virConnectDomainEventAgentLifecycleCallback)(virConnectPtr conn, int reason, void *opaque);
+typedef enum { + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_CONNECTED = 1, /* channel connected */ + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_DISCONNECTED = 2, /* channel disconnected */ + +# ifdef VIR_ENUM_SENTINELS + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_LAST +# endif +} virConnectDomainEventChannelLifecycleState; + +typedef enum { + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_UNKNOWN = 0, /* unknown state change reason */ + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_DOMAIN_STARTED = 1, /* state changed due to domain start */
This reason is not used at all and I don't think it even makes sense to be reported for channel events. With guest agent it might make sense and also other possible reasons as guest agent error.
This reason is, in fact, being used in qemu_process.c:1925 (qemuProcessRefreshChannelVirtioState). The state is actually aligned with the virConnectDomainEventChannelLifecycleReason enum in order to reuse the existing `agentReason` for emitting this event. I can explicitly add a `channelReason` if that makes the code clearer/more readable.
With channels you mostly care about the connect and disconnect events. It might make sense to report client connect or disconnect in the future for certain backend types, but I don't think that qemu supports such report currently.
I agree that with chanels you generally care about `connected` and `disconnected`, but the real problem here is that there is no framing over the virtio-serial channel. As a result, it's important for developers of guest agents to know when a channel connection is being cycled or started for the first time. Remember that the reason for this patch in the first place is that I am creating an agent myself.
+ VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL = 2, /* channel state changed */ + +# ifdef VIR_ENUM_SENTINELS + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_LAST +# endif +} virConnectDomainEventChannelLifecycleReason; + +/** + * virConnectDomainEventChannelLifecycleCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @channelName: the name of the channel on which the event occurred + * @state: new state of the guest channel, one of virConnectDomainEventChannelLifecycleState + * @reason: reason for state change; one of virConnectDomainEventChannelLifecycleReason + * @opaque: application specified data + * + * This callback occurs when libvirt detects a change in the state of a guest + * serial channel.
You should note that the hypervisor needs to support channel state notifications, otherwise it won't work. (Qemu supporting this has been around for ages now, but we still support versions older than that. Also other hypervisors don't support this currently)
Agreed.
+ * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_CHANNEL_LIFECYCLE with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventChannelLifecycleCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *channelName, + int state, + int reason, + void *opaque);
/** * VIR_DOMAIN_EVENT_CALLBACK: @@ -4006,6 +4046,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION = 20, /* virConnectDomainEventMigrationIterationCallback */ VIR_DOMAIN_EVENT_ID_JOB_COMPLETED = 21, /* virConnectDomainEventJobCompletedCallback */ VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED = 22, /* virConnectDomainEventDeviceRemovalFailedCallback */ + VIR_DOMAIN_EVENT_ID_CHANNEL_LIFECYCLE = 23, /* virConnectDomainEventChannelLifecycleCallback */
# ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_ID_LAST
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c8414..b464412 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4407,6 +4407,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainChrDeviceState newstate; virObjectEventPtr event = NULL; + virObjectEventPtr channelEvent = NULL; virDomainDeviceDef dev; qemuDomainObjPrivatePtr priv = vm->privateData; int rc; @@ -4482,6 +4483,10 @@ processSerialChangedEvent(virQEMUDriverPtr driver, qemuDomainEventQueue(driver, event); }
+ channelEvent = virDomainEventChannelLifecycleNewFromObj(vm, dev.data.chr->target.name, newstate, + VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL);
I don't think we should emit this event for the guest agent channel. It has a separate one and the channel is reserved for libvirt anyways, so client applications should use the guest agent event for this.
This has been answered in subsequent emails.
+ qemuDomainEventQueue(driver, channelEvent); + endjob: qemuDomainObjEndJob(driver, vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..31b5028 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c
[...]
@@ -1958,6 +1959,11 @@ qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver, agentReason))) qemuDomainEventQueue(driver, event);
+ + channelEvent = + virDomainEventChannelLifecycleNewFromObj(vm, chr->target.name, entry->state, agentReason); + qemuDomainEventQueue(driver, channelEvent);
Same comment as above.
+ chr->state = entry->state; } } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index a3cd7cd..c778c42 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c
[...]
@@ -538,6 +543,10 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventCallbackAgentLifecycle, sizeof(remote_domain_event_callback_agent_lifecycle_msg), (xdrproc_t)xdr_remote_domain_event_callback_agent_lifecycle_msg }, + { REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CHANNEL_LIFECYCLE, + remoteDomainBuildEventCallbackChannelLifecycle, + sizeof(remote_domain_event_callback_channel_lifecycle_msg), + (xdrproc_t)xdr_remote_domain_event_callback_channel_lifecycle_msg }, { REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED, remoteDomainBuildEventCallbackDeviceAdded, sizeof(remote_domain_event_callback_device_added_msg),
These are ordered in the way the events were added, and you are not putting yours at the end.
Oops, thanks for catching that. I thought I had caught all the ordering issues!
Other than the few details looks good.
Peter