6:51 a.m.
On Fri, Nov 11, 2016 at 5:13 AM, Peter Krempa <pkrempa(a)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