On 12/16/2011 09:58 AM, shaohef@xxxxxxxxxxxxxxxxxx wrote:
> From: ShaoHe Feng <shaohef@xxxxxxxxxxxxxxxxxx>
>
Pretty light on the commit message.
>
> Signed-off-by: ShaoHe Feng <shaohef@xxxxxxxxxxxxxxxxxx>
> ---
> daemon/libvirtd.h | 10 +++
> daemon/remote.c | 172 +++++++++++++++++++++++++++++++++++++++++-
> src/remote/qemu_protocol.x | 29 +++++++-
> src/remote/remote_driver.c | 162 ++++++++++++++++++++++++++++++++++++++-
> src/remote/remote_protocol.x | 6 ++
> src/remote_protocol-structs | 5 +
> 6 files changed, 376 insertions(+), 8 deletions(-)
>
> diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h
> index c8d3ca2..fab7290 100644
> --- a/daemon/libvirtd.h
> +++ b/daemon/libvirtd.h
> @@ -38,6 +38,15 @@
> # endif
> # include "virnetserverprogram.h"
>
> +/* limit the number unknow event of an conncet can register */
s/unknow/unknown/
s/conncet/connection/
But based on my review of 1/4, this has several problems. They aren't
unknown events, so much as qemu events, which might be sent in parallel
to a libvirt event.
Is the goal to hard-code the number of event names we allow to be
registered in order to avoid a DoS where someone just registers an
indefinite stream of arbitrary event names to exhaust libvirtd's memory?
But since we don't limit the number of event registration ids you can
create, I think we're better off tracking this as a growable hash table,
rather than capping it to a hard limit in the number of names.
> +++ b/daemon/remote.c
> @@ -421,6 +421,53 @@ mem_error:
> return -1;
> }
>
> +static int remoteRelayDomainEventUnknown(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virDomainPtr dom,
> + const char *eventName, /* The JSON event name */
> + const char *eventArgs, /* The JSON string of args */
> + void *opaque)
> static int remoteRelayDomainEventControlError(virConnectPtr conn ATTRIBUTE_UNUSED,
> virDomainPtr dom,
> @@ -509,6 +556,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventControlError),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDiskChange),
> + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventUnknown),
> };
I think you want to separate things; since registering a qemu callback
should not interfere with libvirt events, this will not be in the table
of domainEventCallbacks. I think there's going to be a bit of work to
refactor this so that it is triggered in the right code paths for
libvirt-qemu.so, but doesn't impact libvirt.so.
> +++ b/src/remote/qemu_protocol.x
> @@ -47,6 +47,30 @@ struct qemu_domain_attach_ret {
> remote_nonnull_domain dom;
> };
>
> +struct qemu_domain_events_register_args {
> + remote_nonnull_string eventName;
> +};
> +
> +struct qemu_domain_events_deregister_args {
> + remote_nonnull_string eventName;
Shouldn't need the eventName on deregister.
> + int callbackID;
> +};
> +
> +struct qemu_domain_events_register_ret {
> + int callbackID;
> +};
I'd stick the related args and ret structs next to one another.
> +
> +struct qemu_domain_events_deregister_ret {
> + int callbackID;
> +};
Deregistration doesn't need a _ret.
> +
> +struct qemu_domain_events_unknown_event_msg {
> + remote_nonnull_domain dom;
> + remote_nonnull_string eventName;
> + remote_nonnull_string eventArgs;
> +};
> +
> +
> /* Define the program number, protocol version and procedure numbers here. */
> const QEMU_PROGRAM = 0x20008087;
> const QEMU_PROTOCOL_VERSION = 1;
> @@ -61,5 +85,8 @@ enum qemu_procedure {
> * are some exceptions to this rule, e.g. domainDestroy. Other APIs MAY
> * be marked as high priority. If in doubt, it's safe to choose low. */
> QEMU_PROC_MONITOR_COMMAND = 1, /* skipgen skipgen priority:low */
> - QEMU_PROC_DOMAIN_ATTACH = 2 /* autogen autogen priority:low */
> + QEMU_PROC_DOMAIN_ATTACH = 2, /* autogen autogen priority:low */
> + QEMU_PROC_DOMAIN_EVENTS_REGISTER = 3, /* skipgen skipgen priority:low */
> + QEMU_PROC_DOMAIN_EVENTS_DEREGISTER = 4, /* skipgen skipgen priority:low */
> + QEMU_PROC_DOMAIN_EVENTS_UNKNOWN_EVENT = 5 /* skipgen skipgen */
I still don't like the name UNKNOWN everywhere.
>
> @@ -4627,6 +4777,8 @@ static virDriver remote_driver = {
> .nodeGetFreeMemory = remoteNodeGetFreeMemory, /* 0.3.3 */
> .domainEventRegister = remoteDomainEventRegister, /* 0.5.0 */
> .domainEventDeregister = remoteDomainEventDeregister, /* 0.5.0 */
> + .qemuDomainQemuEventRegister = remoteDomainQemuEventRegister, /* 0.9.9 */
> + .qemuDomainQemuEventDeregister = remoteDomainQemuEventDeregister, /* 0.9.9 */
0.9.10, now. It looks like a lot of this will be copy and paste from
existing event codes, but I didn't review it closely.
> +++ b/src/remote/remote_protocol.x
> @@ -2049,6 +2049,12 @@ struct remote_domain_event_disk_change_msg {
> int reason;
> };
>
> +struct remote_domain_event_default_event_msg {
> + remote_nonnull_domain dom;
> + remote_nonnull_string eventName;
> + remote_nonnull_string eventArgs;
> +};
No. We should _not_ be sending any qemu-specific event over
remote_protocol.x. It should all be handled solely by qemu_protocol.x.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list