[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/4] add a qemu-specific event register API supported in remote driver



On 12/16/2011 09:58 AM, shaohef linux vnet ibm com wrote:
> From: ShaoHe Feng <shaohef linux vnet ibm com>
> 

Pretty light on the commit message.

> 
> Signed-off-by: ShaoHe Feng <shaohef linux vnet ibm com>
> ---
>  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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]