Allow outbound-only RpcChannels, cleanup API.

. allow a channel to be set up for outbound operations only. This defers
creation of an RpcIn channel to until its needed, and doesn't enforce
that data for RpcIn operations is available when starting the channel.

This allows clients who only want to send outbound messages to more easily
use the channel. Before, because of the code enforcing the call to the
setup() function, these apps could run into issues like failing to properly
start the channel because of some conflict caused by the RpcIn channel name
(among other possible issues).

. make the private data in the RpcChannel structure private to the
lib/rpcChannel code.

This only exposes the bare minimum of the structure in the header file to
allow for new implementations of RpcChannel.

Signed-off-by: Marcelo Vanzin <mvanzin@vmware.com>

diff --git a/open-vm-tools/lib/include/vmware/tools/guestrpc.h b/open-vm-tools/lib/include/vmware/tools/guestrpc.h
index eae0a41eff5a6fe2225449ea706fbfa8a031d794..0b98c7600dd6088abe55164b6956fd5859e09463 100644 (file)
--- a/open-vm-tools/lib/include/vmware/tools/guestrpc.h
+++ b/open-vm-tools/lib/include/vmware/tools/guestrpc.h
@@ -116,6 +116,10 @@ typedef gboolean (*RpcChannelSendFn)(struct RpcChannel *,
                                      size_t dataLen,
                                      char **result,
                                      size_t *resultLen);
+typedef void (*RpcChannelSetupFn)(struct RpcChannel *chan,
+                                  GMainContext *mainCtx,
+                                  const char *appName,
+                                  gpointer appCtx);
 
 
 /**
@@ -135,18 +139,8 @@ typedef struct RpcChannel {
    RpcChannelStartFn       start;
    RpcChannelStopFn        stop;
    RpcChannelSendFn        send;
-   /* Private section: don't use the fields below directly. */
+   RpcChannelSetupFn       setup;
    RpcChannelShutdownFn    shutdown;
-   gchar                  *appName;
-   GHashTable             *rpcs;
-   GMainContext           *mainCtx;
-   GSource                *resetCheck;
-   gpointer                appCtx;
-   RpcChannelCallback      resetReg;
-   RpcChannelResetCb       resetCb;
-   gpointer                resetData;
-   gboolean                rpcError;
-   guint                   rpcErrorCount;
    gpointer                _private;
 } RpcChannel;
 
@@ -217,6 +211,9 @@ RpcChannel_BuildXdrCommand(const char *cmd,
                            char **result,
                            size_t *resultLen);
 
+RpcChannel *
+RpcChannel_Create(void);
+
 gboolean
 RpcChannel_Destroy(RpcChannel *chan);
 
@@ -246,7 +243,7 @@ RpcChannel_UnregisterCallback(RpcChannel *chan,
 
 
 RpcChannel *
-RpcChannel_NewBackdoorChannel(GMainContext *mainCtx);
+BackdoorChannel_New(void);
 
 G_END_DECLS
 

diff --git a/open-vm-tools/lib/rpcChannel/bdoorChannel.c b/open-vm-tools/lib/rpcChannel/bdoorChannel.c
index 57772075445fed53a9cdba135e3f5b65a338bc7b..b9f3b154e8bdeb0f88cdb33ef7af848935264f16 100644 (file)
--- a/open-vm-tools/lib/rpcChannel/bdoorChannel.c
+++ b/open-vm-tools/lib/rpcChannel/bdoorChannel.c
@@ -33,6 +33,7 @@
 #define RPCIN_MAX_DELAY    10
 
 typedef struct BackdoorChannel {
+   GMainContext  *mainCtx;
    GStaticMutex   outLock;
    RpcIn         *in;
    RpcOut        *out;
@@ -41,6 +42,28 @@ typedef struct BackdoorChannel {
 } BackdoorChannel;
 
 
+/**
+ * Initializes internal state for the inbound channel.
+ *
+ * @param[in]  chan     The RPC channel instance.
+ * @param[in]  ctx      Main application context.
+ * @param[in]  appName  Unused.
+ * @param[in]  appCtx   Unused.
+ */
+
+static void
+RpcInSetup(RpcChannel *chan,
+           GMainContext *ctx,
+           const char *appName,
+           gpointer appCtx)
+{
+   BackdoorChannel *bdoor = chan->_private;
+   bdoor->mainCtx = g_main_context_ref(ctx);
+   bdoor->in = RpcIn_Construct(ctx, RpcChannel_Dispatch, chan);
+   ASSERT(bdoor->in != NULL);
+}
+
+
 /**
  * Starts the RpcIn loop and the RpcOut channel.
  *
@@ -52,21 +75,22 @@ typedef struct BackdoorChannel {
 static gboolean
 RpcInStart(RpcChannel *chan)
 {
-   gboolean ret;
+   gboolean ret = TRUE;
    BackdoorChannel *bdoor = chan->_private;
 
-   ASSERT(chan->appName != NULL);
-   ASSERT(!bdoor->inStarted);
+   ASSERT(bdoor->in == NULL || !bdoor->inStarted);
    ASSERT(!bdoor->outStarted);
 
-   ret = RpcIn_start(bdoor->in, RPCIN_MAX_DELAY, RpcChannel_Error, chan);
+   if (bdoor->in != NULL) {
+      ret = RpcIn_start(bdoor->in, RPCIN_MAX_DELAY, RpcChannel_Error, chan);
+   }
    if (ret) {
       ret = RpcOut_start(bdoor->out);
       if (!ret) {
          RpcIn_stop(bdoor->in);
       }
    }
-   bdoor->inStarted = TRUE;
+   bdoor->inStarted = (bdoor->in != NULL);
    bdoor->outStarted = TRUE;
    return ret;
 }
@@ -88,7 +112,6 @@ RpcInStop(RpcChannel *chan)
 {
    BackdoorChannel *bdoor = chan->_private;
 
-   ASSERT(chan->appName != NULL);
    g_static_mutex_lock(&bdoor->outLock);
    if (bdoor->out != NULL) {
       if (bdoor->outStarted) {
@@ -124,9 +147,14 @@ RpcInShutdown(RpcChannel *chan)
 {
    BackdoorChannel *bdoor = chan->_private;
    RpcInStop(chan);
-   RpcIn_Destruct(bdoor->in);
+   if (bdoor->in != NULL) {
+      RpcIn_Destruct(bdoor->in);
+   }
    RpcOut_Destruct(bdoor->out);
    g_static_mutex_free(&bdoor->outLock);
+   if (bdoor->mainCtx != NULL) {
+      g_main_context_unref(bdoor->mainCtx);
+   }
    g_free(bdoor);
 }
 
@@ -155,14 +183,11 @@ RpcInSend(RpcChannel *chan,
    size_t replyLen;
    BackdoorChannel *bdoor = chan->_private;
 
-   ASSERT(chan->appName != NULL);
-
    g_static_mutex_lock(&bdoor->outLock);
    if (!bdoor->outStarted) {
       goto exit;
    }
 
-
    ret = RpcOut_send(bdoor->out, data, dataLen, &reply, &replyLen);
 
    /*
@@ -222,24 +247,19 @@ exit:
 
 
 /**
- * Creates a new RpcIn channel.
- *
- * @param[in]  mainCtx     The app's main context.
+ * Creates a new RpcChannel channel that uses the backdoor for communication.
  *
  * @return A new channel instance (never NULL).
  */
 
 RpcChannel *
-RpcChannel_NewBackdoorChannel(GMainContext *mainCtx)
+BackdoorChannel_New(void)
 {
    RpcChannel *ret;
    BackdoorChannel *bdoor;
 
-   ret = g_malloc0(sizeof *ret);
-
-   bdoor = g_malloc(sizeof *bdoor);
-   bdoor->in = RpcIn_Construct(mainCtx, RpcChannel_Dispatch, ret);
-   ASSERT(bdoor->in != NULL);
+   ret = RpcChannel_Create();
+   bdoor = g_malloc0(sizeof *bdoor);
 
    g_static_mutex_init(&bdoor->outLock);
    bdoor->out = RpcOut_Construct();
@@ -251,6 +271,7 @@ RpcChannel_NewBackdoorChannel(GMainContext *mainCtx)
    ret->start = RpcInStart;
    ret->stop = RpcInStop;
    ret->send = RpcInSend;
+   ret->setup = RpcInSetup;
    ret->shutdown = RpcInShutdown;
    ret->_private = bdoor;
 

diff --git a/open-vm-tools/lib/rpcChannel/rpcChannel.c b/open-vm-tools/lib/rpcChannel/rpcChannel.c
index 3cc0c4d5aff4ee51c21af5724d76b6fe0126837a..1f1c6c779a65cdcc704f11257283f5f037ee235f 100644 (file)
--- a/open-vm-tools/lib/rpcChannel/rpcChannel.c
+++ b/open-vm-tools/lib/rpcChannel/rpcChannel.c
@@ -31,6 +31,22 @@
 #include "vmxrpc.h"
 #include "xdrutil.h"
 
+/** Internal state of a channel. */
+typedef struct RpcChannelInt {
+   RpcChannel              impl;
+   gchar                  *appName;
+   GHashTable             *rpcs;
+   GMainContext           *mainCtx;
+   GSource                *resetCheck;
+   gpointer                appCtx;
+   RpcChannelCallback      resetReg;
+   RpcChannelResetCb       resetCb;
+   gpointer                resetData;
+   gboolean                rpcError;
+   guint                   rpcErrorCount;
+} RpcChannelInt;
+
+
 /** Max number of times to attempt a channel restart. */
 #define RPCIN_MAX_RESTARTS 60
 
@@ -68,13 +84,13 @@ RpcChannelPing(RpcInData *data)
 static gboolean
 RpcChannelRestart(gpointer _chan)
 {
-   RpcChannel *chan = _chan;
+   RpcChannelInt *chan = _chan;
 
-   chan->stop(chan);
-   if (!chan->start(chan)) {
+   RpcChannel_Stop(&chan->impl);
+   if (!RpcChannel_Start(&chan->impl)) {
       g_warning("Channel restart failed [%d]\n", chan->rpcErrorCount);
       if (chan->resetCb != NULL) {
-         chan->resetCb(chan, FALSE, chan->resetData);
+         chan->resetCb(&chan->impl, FALSE, chan->resetData);
       }
    } else {
       chan->rpcError = FALSE;
@@ -97,7 +113,7 @@ static gboolean
 RpcChannelCheckReset(gpointer _chan)
 {
    static int channelTimeoutAttempts = RPCIN_MAX_RESTARTS;
-   RpcChannel *chan = _chan;
+   RpcChannelInt *chan = _chan;
 
    /* Check the channel state. */
    if (chan->rpcError) {
@@ -107,7 +123,7 @@ RpcChannelCheckReset(gpointer _chan)
          g_warning("Failed to reset channel after %u attempts\n",
                    chan->rpcErrorCount - 1);
          if (chan->resetCb != NULL) {
-            chan->resetCb(chan, FALSE, chan->resetData);
+            chan->resetCb(&chan->impl, FALSE, chan->resetData);
          }
          goto exit;
       }
@@ -126,7 +142,7 @@ RpcChannelCheckReset(gpointer _chan)
    chan->rpcErrorCount = 0;
 
    if (chan->resetCb != NULL) {
-      chan->resetCb(chan, TRUE, chan->resetData);
+      chan->resetCb(&chan->impl, TRUE, chan->resetData);
    }
 
 exit:
@@ -148,7 +164,7 @@ static gboolean
 RpcChannelReset(RpcInData *data)
 {
    gchar *msg;
-   RpcChannel *chan = data->clientData;
+   RpcChannelInt *chan = data->clientData;
 
    if (chan->resetCheck == NULL) {
       chan->resetCheck = g_idle_source_new();
@@ -300,6 +316,26 @@ exit:
 }
 
 
+/**
+ * Creates a new RpcChannel without any implementation.
+ *
+ * This is mainly for use of code that is implementing a custom RpcChannel.
+ * Such implementations should provide their own "constructor"-type function
+ * which should then call this function to get an RpcChannel instance. They
+ * should then fill in the function pointers that provide the implementation
+ * for the channel before making the channel available to the callers.
+ *
+ * @return A new RpcChannel instance.
+ */
+
+RpcChannel *
+RpcChannel_Create(void)
+{
+   RpcChannelInt *chan = g_new0(RpcChannelInt, 1);
+   return &chan->impl;
+}
+
+
 /**
  * Dispatches the given RPC to the registered handler. This mimics the behavior
  * of the RpcIn library (but is not tied to that particular implementation of
@@ -318,7 +354,7 @@ RpcChannel_Dispatch(RpcInData *data)
    size_t nameLen;
    Bool status;
    RpcChannelCallback *rpc = NULL;
-   RpcChannel *chan = data->clientData;
+   RpcChannelInt *chan = data->clientData;
 
    name = StrUtil_GetNextToken(&index, data->args, " ");
    if (name == NULL) {
@@ -370,37 +406,40 @@ gboolean
 RpcChannel_Destroy(RpcChannel *chan)
 {
    size_t i;
+   RpcChannelInt *cdata = (RpcChannelInt *) chan;
 
-   if (chan->shutdown != NULL) {
-      chan->shutdown(chan);
+   if (cdata->impl.shutdown != NULL) {
+      cdata->impl.shutdown(chan);
    }
 
-   RpcChannel_UnregisterCallback(chan, &chan->resetReg);
+   RpcChannel_UnregisterCallback(chan, &cdata->resetReg);
    for (i = 0; i < ARRAYSIZE(gRpcHandlers); i++) {
       RpcChannel_UnregisterCallback(chan, &gRpcHandlers[i]);
    }
 
-   if (chan->rpcs != NULL) {
-      g_hash_table_destroy(chan->rpcs);
-      chan->rpcs = NULL;
+   if (cdata->rpcs != NULL) {
+      g_hash_table_destroy(cdata->rpcs);
+      cdata->rpcs = NULL;
    }
 
-   chan->resetCb = NULL;
-   chan->resetData = NULL;
-   chan->appCtx = NULL;
+   cdata->resetCb = NULL;
+   cdata->resetData = NULL;
+   cdata->appCtx = NULL;
 
-   g_free(chan->appName);
-   chan->appName = NULL;
+   g_free(cdata->appName);
+   cdata->appName = NULL;
 
-   g_main_context_unref(chan->mainCtx);
-   chan->mainCtx = NULL;
+   if (cdata->mainCtx != NULL) {
+      g_main_context_unref(cdata->mainCtx);
+      cdata->mainCtx = NULL;
+   }
 
-   if (chan->resetCheck != NULL) {
-      g_source_destroy(chan->resetCheck);
-      chan->resetCheck = NULL;
+   if (cdata->resetCheck != NULL) {
+      g_source_destroy(cdata->resetCheck);
+      cdata->resetCheck = NULL;
    }
 
-   g_free(chan);
+   g_free(cdata);
    return TRUE;
 }
 
@@ -417,7 +456,7 @@ void
 RpcChannel_Error(void *_chan,
                  char const *status)
 {
-   RpcChannel *chan = _chan;
+   RpcChannelInt *chan = _chan;
    chan->rpcError = TRUE;
    g_warning("Error in the RPC receive loop: %s.\n", status);
 
@@ -430,8 +469,11 @@ RpcChannel_Error(void *_chan,
 
 
 /**
- * Initializes the RPC channel for use. This function must be called before
- * starting the channel.
+ * Initializes the RPC channel for inbound operations.
+ *
+ * This function must be called before starting the channel if the application
+ * wants to receive messages on the channel. Applications don't need to call it
+ * if only using the outbound functionality.
  *
  * @param[in]  chan        The RPC channel.
  * @param[in]  appName     TCLO application name.
@@ -450,25 +492,29 @@ RpcChannel_Setup(RpcChannel *chan,
                  gpointer resetData)
 {
    size_t i;
+   RpcChannelInt *cdata = (RpcChannelInt *) chan;
 
-   chan->appName = g_strdup(appName);
-   chan->appCtx = appCtx;
-   chan->mainCtx = g_main_context_ref(mainCtx);
-   chan->resetCb = resetCb;
-   chan->resetData = resetData;
+   cdata->appName = g_strdup(appName);
+   cdata->appCtx = appCtx;
+   cdata->mainCtx = g_main_context_ref(mainCtx);
+   cdata->resetCb = resetCb;
+   cdata->resetData = resetData;
 
-   chan->resetReg.name = "reset";
-   chan->resetReg.callback = RpcChannelReset;
-   chan->resetReg.clientData = chan;
+   cdata->resetReg.name = "reset";
+   cdata->resetReg.callback = RpcChannelReset;
+   cdata->resetReg.clientData = chan;
 
    /* Register the callbacks handled by the rpcChannel library. */
-   RpcChannel_RegisterCallback(chan, &chan->resetReg);
+   RpcChannel_RegisterCallback(chan, &cdata->resetReg);
 
    for (i = 0; i < ARRAYSIZE(gRpcHandlers); i++) {
       RpcChannel_RegisterCallback(chan, &gRpcHandlers[i]);
    }
-}
 
+   if (cdata->impl.setup != NULL) {
+      cdata->impl.setup(&cdata->impl, mainCtx, appName, appCtx);
+   }
+}
 
 
 /**
@@ -508,16 +554,17 @@ void
 RpcChannel_RegisterCallback(RpcChannel *chan,
                             RpcChannelCallback *rpc)
 {
+   RpcChannelInt *cdata = (RpcChannelInt *) chan;
    ASSERT(rpc->name != NULL && strlen(rpc->name) > 0);
    ASSERT(rpc->callback);
    ASSERT(rpc->xdrIn == NULL || rpc->xdrInSize > 0);
-   if (chan->rpcs == NULL) {
-      chan->rpcs = g_hash_table_new(g_str_hash, g_str_equal);
+   if (cdata->rpcs == NULL) {
+      cdata->rpcs = g_hash_table_new(g_str_hash, g_str_equal);
    }
-   if (g_hash_table_lookup(chan->rpcs, rpc->name) != NULL) {
+   if (g_hash_table_lookup(cdata->rpcs, rpc->name) != NULL) {
       g_error("Trying to overwrite existing RPC registration for %s!\n", rpc->name);
    }
-   g_hash_table_insert(chan->rpcs, (gpointer) rpc->name, rpc);
+   g_hash_table_insert(cdata->rpcs, (gpointer) rpc->name, rpc);
 }
 
 
@@ -533,8 +580,9 @@ void
 RpcChannel_UnregisterCallback(RpcChannel *chan,
                               RpcChannelCallback *rpc)
 {
-   if (chan->rpcs != NULL) {
-      g_hash_table_remove(chan->rpcs, rpc->name);
+   RpcChannelInt *cdata = (RpcChannelInt *) chan;
+   if (cdata->rpcs != NULL) {
+      g_hash_table_remove(cdata->rpcs, rpc->name);
    }
 }
 

diff --git a/open-vm-tools/services/vmtoolsd/toolsRpc.c b/open-vm-tools/services/vmtoolsd/toolsRpc.c
index c583d0b204367e28ded1a42dc195a5ec1d9c9669..9cdca15fb17f58dd489831936c96ca818532e7a2 100644 (file)
--- a/open-vm-tools/services/vmtoolsd/toolsRpc.c
+++ b/open-vm-tools/services/vmtoolsd/toolsRpc.c
@@ -244,7 +244,7 @@ ToolsCore_InitRpc(ToolsServiceState *state)
                    state->name);
          state->ctx.rpc = NULL;
       } else {
-         state->ctx.rpc = RpcChannel_NewBackdoorChannel(mainCtx);
+         state->ctx.rpc = BackdoorChannel_New();
       }
       app = ToolsCore_GetTcloName(state);
       ASSERT(app != NULL);

diff --git a/open-vm-tools/tests/vmrpcdbg/debugChannel.c b/open-vm-tools/tests/vmrpcdbg/debugChannel.c
index a099736cc05af7e24592e69dbee95fe11055a617..3251dfe43439bc9197933a86900e87b64a03e877 100644 (file)
--- a/open-vm-tools/tests/vmrpcdbg/debugChannel.c
+++ b/open-vm-tools/tests/vmrpcdbg/debugChannel.c
@@ -37,6 +37,7 @@
 #include "vmware/tools/utils.h"
 
 typedef struct DbgChannelData {
+   ToolsAppCtx      *ctx;
    gboolean          hasLibRef;
    RpcDebugPlugin   *plugin;
    GSource          *msgTimer;
@@ -68,7 +69,7 @@ RpcDebugDispatch(gpointer _chan)
    memset(&rpcdata, 0, sizeof rpcdata);
 
    if (!plugin->sendFn(&rpcdata)) {
-      RpcDebug_DecRef(chan->appCtx);
+      RpcDebug_DecRef(cdata->ctx);
       cdata->hasLibRef = FALSE;
       return FALSE;
    } else if (rpcdata.message == NULL) {
@@ -80,7 +81,7 @@ RpcDebugDispatch(gpointer _chan)
    }
 
    data.clientData = chan;
-   data.appCtx = chan->appCtx;
+   data.appCtx = cdata->ctx;
    data.args = rpcdata.message;
    data.argsSize = rpcdata.messageLen;
 
@@ -100,8 +101,8 @@ RpcDebugDispatch(gpointer _chan)
    }
 
    if (!ret) {
-      VMTOOLSAPP_ERROR((ToolsAppCtx *) chan->appCtx, 1);
-      RpcDebug_DecRef(chan->appCtx);
+      VMTOOLSAPP_ERROR(cdata->ctx, 1);
+      RpcDebug_DecRef(cdata->ctx);
       cdata->hasLibRef = FALSE;
       return FALSE;
    }
@@ -124,11 +125,11 @@ RpcDebugStart(RpcChannel *chan)
 {
    DbgChannelData *data = chan->_private;
 
-   ASSERT(chan->appName != NULL);
+   ASSERT(data->ctx != NULL);
    ASSERT(data->msgTimer == NULL);
 
    data->msgTimer = g_timeout_source_new(100);
-   VMTOOLSAPP_ATTACH_SOURCE((ToolsAppCtx *)chan->appCtx,
+   VMTOOLSAPP_ATTACH_SOURCE(data->ctx,
                             data->msgTimer,
                             RpcDebugDispatch,
                             chan,
@@ -179,12 +180,13 @@ RpcDebugSend(RpcChannel *chan,
 {
    char *copy;
    gpointer xdrdata = NULL;
-   RpcDebugPlugin *plugin = ((DbgChannelData *)chan->_private)->plugin;
+   DbgChannelData *cdata = chan->_private;
+   RpcDebugPlugin *plugin = cdata->plugin;
    RpcDebugRecvMapping *mapping = NULL;
    RpcDebugRecvFn recvFn = NULL;
    gboolean ret = TRUE;
 
-   ASSERT(chan->appName != NULL);
+   ASSERT(cdata->ctx != NULL);
 
    /* Be paranoid. Like the VMX, NULL-terminate the incoming data. */
    copy = g_malloc(dataLen + 1);
@@ -256,7 +258,27 @@ exit:
 
 
 /**
- * Does nothing.
+ * Intiializes internal state for the inbound channel.
+ *
+ * @param[in]  chan     The RPC channel instance.
+ * @param[in]  ctx      Unused.
+ * @param[in]  appName  Unused.
+ * @param[in]  appCtx   A ToolsAppCtx instance.
+ */
+
+static void
+RpcDebugSetup(RpcChannel *chan,
+              GMainContext *ctx,
+              const char *appName,
+              gpointer appCtx)
+{
+   DbgChannelData *cdata = chan->_private;
+   cdata->ctx = appCtx;
+}
+
+
+/**
+ * Cleans up the internal channel state.
  *
  * @param[in]  chan     The RPC channel instance.
  */
@@ -265,9 +287,9 @@ static void
 RpcDebugShutdown(RpcChannel *chan)
 {
    DbgChannelData *cdata = chan->_private;
-   ASSERT(chan->appName != NULL);
+   ASSERT(cdata->ctx != NULL);
    if (cdata->hasLibRef) {
-      RpcDebug_DecRef(chan->appCtx);
+      RpcDebug_DecRef(cdata->ctx);
    }
    g_free(chan->_private);
 }
@@ -293,11 +315,11 @@ RpcDebug_NewDebugChannel(ToolsAppCtx *ctx,
    RpcChannel *ret;
 
    ASSERT(data != NULL);
-
-   ret = g_malloc0(sizeof *ret);
+   ret = RpcChannel_Create();
    ret->start = RpcDebugStart;
    ret->stop = RpcDebugStop;
    ret->send = RpcDebugSend;
+   ret->setup = RpcDebugSetup;
    ret->shutdown = RpcDebugShutdown;
 
    cdata = g_malloc0(sizeof *cdata);