event: server RPC protocol tweaks for domain lifecycle events

This patch adds some new RPC call numbers, but for ease of review,
they sit idle until a later patch adds the client counterpart to
drive the new RPCs.  Also for ease of review, I limited this patch
to just the lifecycle event; although converting the remaining
15 domain events will be quite mechanical.  On the server side,
we have to have a function per RPC call, largely with duplicated
bodies (the key difference being that we store in our callback
opaque pointer whether events should be fired with old or new
style); meanwhile, a single function can drive multiple RPC
messages.  With a strategic choice of XDR struct layout, we can
make the event generation code for both styles fairly compact.

I debated about adding a tri-state witness variable per
connection (values 'unknown', 'legacy', 'modern').  It would start
as 'unknown', move to 'legacy' if any RPC call is made to a legacy
event call, and move to 'modern' if the feature probe is made;
then the event code could issue an error if the witness state is
incorrect (a legacy RPC call while in 'modern', a modern RPC call
while in 'unknown' or 'legacy', and a feature probe while in
'legacy' or 'modern').  But while it might prevent odd behavior
caused by protocol fuzzing, I don't see that it would prevent
any security holes, so I considered it bloat.

Note that sticking @acl markers on the new RPCs generates unused
functions in access/viraccessapicheck.c, because there is no new
API call that needs to use the new checks; however, having a
consistent .x file is worth the dead code.

* src/libvirt_internal.h (VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK):
New feature.
* src/remote/remote_protocol.x
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_REGISTER_ANY)
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_DEREGISTER_ANY)
(REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE): New RPCs.
* daemon/remote.c (daemonClientCallback): Add field.
(remoteDispatchConnectDomainEventCallbackRegisterAny)
(remoteDispatchConnectDomainEventCallbackDeregisterAny): New
functions.
(remoteDispatchConnectDomainEventRegisterAny)
(remoteDispatchConnectDomainEventDeregisterAny): Mark legacy use.
(remoteRelayDomainEventLifecycle): Change message based on legacy
or new use.
(remoteDispatchConnectSupportsFeature): Advertise new feature.
* src/remote_protocol-structs: Regenerate.
Signed-off-by: NEric Blake <eblake@redhat.com>

daemon/remote.c

@@ -79,6 +79,7 @@ struct daemonClientEventCallback {
     virNetServerClientPtr client;
     int eventID;
     int callbackID;
+    bool legacy;
 };
 
 static virDomainPtr get_nonnull_domain(virConnectPtr conn, remote_nonnull_domain domain);
@@ -199,8 +200,8 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
         !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
         return -1;
 
-    VIR_DEBUG("Relaying domain lifecycle event %d %d, callback %d",
-              event, detail, callback->callbackID);
+    VIR_DEBUG("Relaying domain lifecycle event %d %d, callback %d legacy %d",
+              event, detail, callback->callbackID, callback->legacy);
 
     /* build return data */
     memset(&data, 0, sizeof(data));
@@ -208,9 +209,20 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
     data.event = event;
     data.detail = detail;
 
-    remoteDispatchObjectEventSend(callback->client, remoteProgram,
-                                  REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
-                                  (xdrproc_t)xdr_remote_domain_event_lifecycle_msg, &data);
+    if (callback->legacy) {
+        remoteDispatchObjectEventSend(callback->client, remoteProgram,
+                                      REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
+                                      (xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
+                                      &data);
+    } else {
+        remote_domain_event_callback_lifecycle_msg msg = { callback->callbackID,
+                                                           data };
+
+        remoteDispatchObjectEventSend(callback->client, remoteProgram,
+                                      REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
+                                      (xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
+                                      &msg);
+    }
 
     return 0;
 }
@@ -3281,6 +3293,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
     callback->client = client;
     callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
     callback->callbackID = -1;
+    callback->legacy = true;
     ref = callback;
     if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
                            priv->ndomainEventCallbacks,
@@ -3469,6 +3482,12 @@ cleanup:
     return rv;
 }
 
+
+/* Due to back-compat reasons, two RPC calls map to the same libvirt
+ * API of virConnectDomainEventRegisterAny.  A client should only use
+ * the new call if they have probed
+ * VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
+ * and must not mix the two styles.  */
 static int
 remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
                                             virNetServerClientPtr client,
@@ -3490,9 +3509,13 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
 
     virMutexLock(&priv->lock);
 
-    if (args->eventID >= VIR_DOMAIN_EVENT_ID_LAST ||
+    /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
+     * new domain events added after this point should only use the
+     * modern callback style of RPC.  */
+    if (args->eventID > VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED ||
         args->eventID < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"), args->eventID);
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"),
+                       args->eventID);
         goto cleanup;
     }
 
@@ -3507,6 +3530,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     callback->client = client;
     callback->eventID = args->eventID;
     callback->callbackID = -1;
+    callback->legacy = true;
     ref = callback;
     if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
                            priv->ndomainEventCallbacks,
@@ -3538,6 +3562,85 @@ cleanup:
 }
 
 
+static int
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+                                                    virNetServerClientPtr client,
+                                                    virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                                    virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+                                                    remote_connect_domain_event_callback_register_any_args *args,
+                                                    remote_connect_domain_event_callback_register_any_ret *ret)
+{
+    int callbackID;
+    int rv = -1;
+    daemonClientEventCallbackPtr callback = NULL;
+    daemonClientEventCallbackPtr ref;
+    struct daemonClientPrivate *priv =
+        virNetServerClientGetPrivateData(client);
+    virDomainPtr dom = NULL;
+
+    if (!priv->conn) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+        goto cleanup;
+    }
+
+    virMutexLock(&priv->lock);
+
+    if (args->dom &&
+        !(dom = get_nonnull_domain(priv->conn, *args->dom)))
+        goto cleanup;
+
+    /* FIXME: support all domain events */
+    if (args->eventID != VIR_DOMAIN_EVENT_ID_LIFECYCLE) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"),
+                       args->eventID);
+        goto cleanup;
+    }
+
+    /* If we call register first, we could append a complete callback
+     * to our array, but on OOM append failure, we'd have to then hope
+     * deregister works to undo our register.  So instead we append an
+     * incomplete callback to our array, then register, then fix up
+     * our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
+     * success, we use 'ref' to save a copy of the pointer.  */
+    if (VIR_ALLOC(callback) < 0)
+        goto cleanup;
+    callback->client = client;
+    callback->eventID = args->eventID;
+    callback->callbackID = -1;
+    ref = callback;
+    if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
+                           priv->ndomainEventCallbacks,
+                           callback) < 0)
+        goto cleanup;
+
+    if ((callbackID = virConnectDomainEventRegisterAny(priv->conn,
+                                                       dom,
+                                                       args->eventID,
+                                                       domainEventCallbacks[args->eventID],
+                                                       ref,
+                                                       remoteEventCallbackFree)) < 0) {
+        VIR_SHRINK_N(priv->domainEventCallbacks,
+                     priv->ndomainEventCallbacks, 1);
+        callback = ref;
+        goto cleanup;
+    }
+
+    ref->callbackID = callbackID;
+    ret->callbackID = callbackID;
+
+    rv = 0;
+
+cleanup:
+    VIR_FREE(callback);
+    if (rv < 0)
+        virNetMessageSaveError(rerr);
+    if (dom)
+        virDomainFree(dom);
+    virMutexUnlock(&priv->lock);
+    return rv;
+}
+
+
 static int
 remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
                                               virNetServerClientPtr client,
@@ -3558,9 +3661,13 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server ATTRIBUTE_U
 
     virMutexLock(&priv->lock);
 
-    if (args->eventID >= VIR_DOMAIN_EVENT_ID_LAST ||
+    /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
+     * new domain events added after this point should only use the
+     * modern callback style of RPC.  */
+    if (args->eventID > VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED ||
         args->eventID < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"), args->eventID);
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"),
+                       args->eventID);
         goto cleanup;
     }
 
@@ -3591,6 +3698,53 @@ cleanup:
     return rv;
 }
 
+
+static int
+remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+                                                      virNetServerClientPtr client,
+                                                      virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                                      virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+                                                      remote_connect_domain_event_callback_deregister_any_args *args)
+{
+    int rv = -1;
+    size_t i;
+    struct daemonClientPrivate *priv =
+        virNetServerClientGetPrivateData(client);
+
+    if (!priv->conn) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+        goto cleanup;
+    }
+
+    virMutexLock(&priv->lock);
+
+    for (i = 0; i < priv->ndomainEventCallbacks; i++) {
+        if (priv->domainEventCallbacks[i]->callbackID == args->callbackID)
+            break;
+    }
+    if (i == priv->ndomainEventCallbacks) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("domain event callback %d not registered"),
+                       args->callbackID);
+        goto cleanup;
+    }
+
+    if (virConnectDomainEventDeregisterAny(priv->conn, args->callbackID) < 0)
+        goto cleanup;
+
+    VIR_DELETE_ELEMENT(priv->domainEventCallbacks, i,
+                       priv->ndomainEventCallbacks);
+
+    rv = 0;
+
+cleanup:
+    if (rv < 0)
+        virNetMessageSaveError(rerr);
+    virMutexUnlock(&priv->lock);
+    return rv;
+}
+
+
 static int
 qemuDispatchDomainMonitorCommand(virNetServerPtr server ATTRIBUTE_UNUSED,
                                  virNetServerClientPtr client ATTRIBUTE_UNUSED,
@@ -3911,6 +4065,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
 
     switch (args->feature) {
     case VIR_DRV_FEATURE_FD_PASSING:
+    case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
         supported = 1;
         break;
 

src/libvirt_internal.h

 /*
  * libvirt_internal.h: internally exported APIs, not for public use
  *
- * Copyright (C) 2006-2013 Red Hat, Inc.
+ * Copyright (C) 2006-2014 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -115,6 +115,11 @@ enum {
      * Support for migration parameters.
      */
     VIR_DRV_FEATURE_MIGRATION_PARAMS = 13,
+
+    /*
+     * Support for server-side event filtering via callback ids in events.
+     */
+    VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK = 14,
 };
 
 

src/remote/remote_protocol.x

@@ -1972,6 +1972,10 @@ struct remote_domain_event_lifecycle_msg {
     int event;
     int detail;
 };
+struct remote_domain_event_callback_lifecycle_msg {
+    int callbackID;
+    remote_domain_event_lifecycle_msg msg;
+};
 
 
 struct remote_connect_domain_xml_from_native_args {
@@ -2248,6 +2252,19 @@ struct remote_connect_domain_event_deregister_any_args {
     int eventID;
 };
 
+struct remote_connect_domain_event_callback_register_any_args {
+    int eventID;
+    remote_domain dom;
+};
+
+struct remote_connect_domain_event_callback_register_any_ret {
+    int callbackID;
+};
+
+struct remote_connect_domain_event_callback_deregister_any_args {
+    int callbackID;
+};
+
 struct remote_domain_event_reboot_msg {
     remote_nonnull_domain dom;
 };
@@ -5068,5 +5085,26 @@ enum remote_procedure {
      * @generate: both
      * @acl: none
      */
-    REMOTE_PROC_NETWORK_EVENT_LIFECYCLE = 315
+    REMOTE_PROC_NETWORK_EVENT_LIFECYCLE = 315,
+
+    /**
+     * @generate: none
+     * @priority: high
+     * @acl: connect:search_domains
+     * @aclfilter: domain:getattr
+     */
+    REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_REGISTER_ANY = 316,
+
+    /**
+     * @generate: none
+     * @priority: high
+     * @acl: connect:read
+     */
+    REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_DEREGISTER_ANY = 317,
+
+    /**
+     * @generate: both
+     * @acl: none
+     */
+    REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE = 318
 };

src/remote_protocol-structs

@@ -1491,6 +1491,10 @@ struct remote_domain_event_lifecycle_msg {
         int                        event;
         int                        detail;
 };
+struct remote_domain_event_callback_lifecycle_msg {
+        int                        callbackID;
+        remote_domain_event_lifecycle_msg msg;
+};
 struct remote_connect_domain_xml_from_native_args {
         remote_nonnull_string      nativeFormat;
         remote_nonnull_string      nativeConfig;
@@ -1707,6 +1711,16 @@ struct remote_connect_domain_event_register_any_args {
 struct remote_connect_domain_event_deregister_any_args {
         int                        eventID;
 };
+struct remote_connect_domain_event_callback_register_any_args {
+        int                        eventID;
+        remote_domain              dom;
+};
+struct remote_connect_domain_event_callback_register_any_ret {
+        int                        callbackID;
+};
+struct remote_connect_domain_event_callback_deregister_any_args {
+        int                        callbackID;
+};
 struct remote_domain_event_reboot_msg {
         remote_nonnull_domain      dom;
 };
@@ -2660,4 +2674,7 @@ enum remote_procedure {
         REMOTE_PROC_CONNECT_NETWORK_EVENT_REGISTER_ANY = 313,
         REMOTE_PROC_CONNECT_NETWORK_EVENT_DEREGISTER_ANY = 314,
         REMOTE_PROC_NETWORK_EVENT_LIFECYCLE = 315,
+        REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_REGISTER_ANY = 316,
+        REMOTE_PROC_CONNECT_DOMAIN_EVENT_CALLBACK_DEREGISTER_ANY = 317,
+        REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE = 318,
 };