[libvirt] [PATCH 06/13] Remote driver & daemon impl of new event API

[Daniel Veillard]

On Fri, Mar 19, 2010 at 03:38:54PM +0000, Daniel P. Berrange wrote:
> This wires up the remote driver to handle the new events APIs.
> The public API allows an application to request a callback filters
> events to a specific domain object, and register multiple callbacks
> for the same event type. On the wire there are two strategies for
> this
> 
>  - Register multiple callbacks with the remote daemon, each
>    with filtering as needed
>  - Register only one callback per event type, with no filtering
> 
> Both approaches have potential inefficiency. In the first scheme,
> the same event gets sent over the wire many times if multiple
> callbacks are registered. With the second scheme, unneccessary
> events get sent over the wire if a per-domain filter is set on
> the client. The second scheme is far easier to implement though,
> so this patch takes that approach.

  I think we should try to optimize for the new API, and for monitoring
tools I would assume they are gonna monitor all running domains, so we
are likely to end up with as many registration with each a domain
filter. In such a situation the second scheme sounds better since events
are sent only once over the wire, and they would be no waste since all
doamins are likely to be monitored. So assuming I understood correctly
the second approach is also the one mich makes the most sense to me.

[...]
> diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h
> index d30fcd7..d292681 100644
> --- a/daemon/libvirtd.h
> +++ b/daemon/libvirtd.h
> @@ -177,7 +177,7 @@ struct qemud_client {
>      int watch;
>      unsigned int readonly :1;
>      unsigned int closing :1;
> -    unsigned int domain_events_registered :1;
> +    int domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LAST];

I can't remember if we explicitely initialized the first value in the
enum.


> @@ -4844,21 +4862,26 @@ remoteDispatchDomainEventsDeregister (struct qemud_server *server ATTRIBUTE_UNUS
>  {
>      CHECK_CONN(client);
>  
> -    if (virConnectDomainEventDeregister(conn, remoteRelayDomainEvent) < 0) {
> -        remoteDispatchConnError(rerr, conn);
> +    if (client->domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LIFECYCLE] == -1) {
> +        remoteDispatchFormatError(rerr, _("domain event %d not registered"), VIR_DOMAIN_EVENT_ID_LIFECYCLE);
>          return -1;
>      }
>  
> -    if (ret)
> -        ret->cb_registered = 0;
> +    if (virConnectDomainEventDeregisterAny(conn,
> +                                           client->domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LIFECYCLE]) < 0) {
> +        remoteDispatchConnError(rerr, conn);
> +        return -1;
> +    }
>  
> -    client->domain_events_registered = 0;
> +    client->domainEventCallbackID[VIR_DOMAIN_EVENT_ID_LIFECYCLE] = -1;
>      return 0;
>  }

[...]
> -static int remoteDomainEventDeregister (virConnectPtr conn,
> -                                        virConnectDomainEventCallback callback)
> +static int remoteDomainEventDeregister(virConnectPtr conn,
> +                                       virConnectDomainEventCallback callback)
>  {
>      struct private_data *priv = conn->privateData;
>      int rv = -1;
> @@ -6839,14 +6838,14 @@ static int remoteDomainEventDeregister (virConnectPtr conn,
>              error (conn, VIR_ERR_RPC, _("removing cb from list"));
>              goto done;
>          }
> +    }
>  
> -        if ( priv->callbackList->count == 0 ) {
> -            /* Tell the server when we are the last callback deregistering */
> -            if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER,
> -                      (xdrproc_t) xdr_void, (char *) NULL,
> -                      (xdrproc_t) xdr_void, (char *) NULL) == -1)
> -                goto done;
> -        }
> +    if (virDomainEventCallbackListCountID(conn, priv->callbackList, VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 0) {
> +        /* Tell the server when we are the last callback deregistering */
> +        if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER,
> +                  (xdrproc_t) xdr_void, (char *) NULL,
> +                  (xdrproc_t) xdr_void, (char *) NULL) == -1)
> +            goto done;
>      }

  okay, that's where we could the number of local client registered
  before forwarding a deregisting request remotely.

[...]
> +static int remoteDomainEventDeregisterAny(virConnectPtr conn,
> +                                          int callbackID)
> +{
> +    struct private_data *priv = conn->privateData;
> +    int rv = -1;
> +    remote_domain_events_deregister_any_args args;
> +    int eventID;
> +
> +    remoteDriverLock(priv);
> +
> +    if ((eventID = virDomainEventCallbackListEventID(conn, priv->callbackList, callbackID)) < 0) {
> +        errorf (conn, VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
> +        goto done;
> +    }
> +
> +    if (priv->domainEventDispatching) {
> +        if (virDomainEventCallbackListMarkDeleteID(conn, priv->callbackList,
> +                                                   callbackID) < 0) {
> +            error (conn, VIR_ERR_RPC, _("marking cb for deletion"));
> +            goto done;
> +        }
> +    } else {
> +        if (virDomainEventCallbackListRemoveID(conn, priv->callbackList,
> +                                               callbackID) < 0) {
> +            error (conn, VIR_ERR_RPC, _("removing cb from list"));
> +            goto done;
> +        }
> +    }
> +
> +    /* If that was the last callback for this eventID, we need to disable
> +     * events on the server */
> +    if (virDomainEventCallbackListCountID(conn, priv->callbackList, eventID) == 0) {
> +        args.eventID = eventID;
> +
> +        if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER_ANY,
> +                  (xdrproc_t) xdr_remote_domain_events_deregister_any_args, (char *) &args,
> +                  (xdrproc_t) xdr_void, (char *) NULL) == -1)
> +            goto done;
> +    }

  and here too.

[...]
>  typedef remote_nonnull_string *remote_string;
> -# define REMOTE_DOMAIN_ID_LIST_MAX 16384
> -# define REMOTE_DOMAIN_NAME_LIST_MAX 1024
> -# define REMOTE_CPUMAP_MAX 256

  hitting the indentation of generated headers here too, will probably
  need a rebase

ACK,

  looks fine!

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/