On Fri, Nov 11, 2016 at 5:13 AM, Peter Krempa <pkrempa@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list