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

Re: [libvirt] [PATCH 07/12] Introduce generic RPC server objects



On 03/18/2011 12:54 PM, Daniel P. Berrange wrote:
> To facilitate creation of new daemons providing XDR RPC services,
> pull alot of the libvirtd daemon code into a set of reusable
> objects.

Part 3.  Hopefully I finish before sending this time.

> +int virNetServerAddService(virNetServerPtr srv,
> +                           virNetServerServicePtr svc)

> +
> +int virNetServerSetTLSContext(virNetServerPtr srv,
> +                              virNetTLSContextPtr tls)
> +{
> +    srv->tls = tls;
> +    virNetTLSContextRef(tls);
> +    return 0;

No virNetServerLock(srv)?

> +static void virNetServerAutoShutdownTimer(int timerid ATTRIBUTE_UNUSED,
> +                                          void *opaque) {
> +    virNetServerPtr srv = opaque;
> +
> +    virNetServerLock(srv);
> +
> +    if (srv->autoShutdownFunc(srv, srv->autoShutdownOpaque)) {
> +        VIR_DEBUG0("Automatic shutdown triggered");
> +        srv->quit = 1;

Should srv->quit be bool instead of int?

> +void virNetServerRun(virNetServerPtr srv)
> +{
> +    int timerid = -1;
> +    int timerActive = 0;
> +    int i;
> +
> +    virNetServerLock(srv);
> +
> +    if (srv->autoShutdownTimeout &&
> +        (timerid = virEventAddTimeout(-1,
> +                                      virNetServerAutoShutdownTimer,
> +                                      srv, NULL)) < 0) {
> +        virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                    _("Failed to register shutdown timeout"));
> +        goto cleanup;
> +    }
> +
> +    while (!srv->quit) {
> +        /* A shutdown timeout is specified, so check
> +         * if any drivers have active state, if not
> +         * shutdown after timeout seconds
> +         */
> +        if (srv->autoShutdownTimeout) {
> +            if (timerActive) {
> +                if (srv->clients) {
> +                    VIR_DEBUG("Deactivating shutdown timer %d", timerid);
> +                    virEventUpdateTimeout(timerid, -1);
> +                    timerActive = 0;
> +                }
> +            } else {
> +                if (!srv->clients) {
> +                    VIR_DEBUG("Activating shutdown timer %d", timerid);
> +                    virEventUpdateTimeout(timerid,
> +                                          srv->autoShutdownTimeout * 1000);

Do we need to worry about overflow here (that is, should
srv->autoShutdownTimeout be validated to be less than INT_MAX/1000
earlier on)?

> +                    timerActive = 1;
> +                }
> +            }
> +        }
> +
> +        virNetServerUnlock(srv);
> +        if (virEventRunDefaultImpl() < 0) {
> +            virNetServerLock(srv);
> +            VIR_DEBUG0("Loop iteration error, exiting");
> +            break;
> +        }
> +        virNetServerLock(srv);
> +
> +    reprocess:
> +        for (i = 0 ; i < srv->nclients ; i++) {
> +            if (virNetServerClientWantClose(srv->clients[i]))
> +                virNetServerClientClose(srv->clients[i]);
> +            if (virNetServerClientIsClosed(srv->clients[i])) {
> +                virNetServerClientFree(srv->clients[i]);

Do we really want to hold the server lock while calling these two
client-related functions?  Or is this a recipe for deadlock?
> +++ b/src/rpc/virnetserver.h
> @@ -0,0 +1,80 @@
> +
> +# include <stdbool.h>

"internal.h" should guarantee this

> +
> +void virNetServerRef(virNetServerPtr srv);

ATTRIBUTE_NONNULL(1)

> +
> +bool virNetServerIsPrivileged(virNetServerPtr srv);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerAutoShutdown(virNetServerPtr srv,
> +                              unsigned int timeout,
> +                              virNetServerAutoShutdownFunc func,
> +                              void *opaque);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)

> +
> +typedef void (*virNetServerSignalFunc)(virNetServerPtr srv, siginfo_t *info, void *opaque);
> +
> +int virNetServerAddSignalHandler(virNetServerPtr srv,
> +                                 int signum,
> +                                 virNetServerSignalFunc func,
> +                                 void *opaque);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)

> +
> +int virNetServerAddService(virNetServerPtr srv,
> +                           virNetServerServicePtr svc);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +
> +int virNetServerAddProgram(virNetServerPtr srv,
> +                           virNetServerProgramPtr prog);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +
> +int virNetServerSetTLSContext(virNetServerPtr srv,
> +                              virNetTLSContextPtr tls);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +
> +void virNetServerUpdateServices(virNetServerPtr srv,
> +                                bool enabled);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerRun(virNetServerPtr srv);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerQuit(virNetServerPtr srv);

ATTRIBUTE_NONNULL(1)

> +++ b/src/rpc/virnetserverclient.c
> @@ -0,0 +1,938 @@

> +
> +#include <config.h>
> +
> +#if HAVE_SASL
> +# include <sasl/sasl.h>
> +#endif

Do we really need this, or did it get taken care of by hiding all SASL
details behind virNetSASL*

> +struct _virNetServerClient
> +{
> +    int refs;
> +    bool wantClose;
> +    virMutex lock;
> +    virNetSocketPtr sock;
> +    int auth;
> +    bool readonly;
> +    char *identity;
> +    virNetTLSContextPtr tlsCtxt;
> +    virNetTLSSessionPtr tls;
> +#if HAVE_SASL
> +    virNetSASLSessionPtr sasl;
> +#endif
> +
> +    /* Count of messages in the 'tx' queue,
> +     * and the server worker pool queue
> +     * ie RPC calls in progress. Does not count

s/ie/i.e./

> +     * async events which are not used for
> +     * throttling calculations */
> +    size_t nrequests;
> +    size_t nrequests_max;
> +    /* Zero or one messages being received. Zero if
> +     * nrequests >= max_clients and throttling */
> +    virNetMessagePtr rx;
> +    /* Zero or many messages waiting for transmit
> +     * back to client, including async events */
> +    virNetMessagePtr tx;
> +
> +    /* Filters to capture messages that would otherwise
> +     * end up on the 'dx' queue */

dx?  Did you mean tx?

> +
> +/*
> + * @server: a locked or unlocked server object
> + * @client: a locked client object
> + */
> +static int virNetServerClientRegisterEvent(virNetServerClientPtr client)

No @server argument; too much copy-n-paste?

> +
> +int virNetServerClientAddFilter(virNetServerClientPtr client,
> +                                virNetServerClientFilterFunc func,
> +                                void *opaque)

No docs?

> +
> +
> +void virNetServerClientRemoveFilter(virNetServerClientPtr client,
> +                                    int filterID)
> +{
> +    virNetServerClientFilterPtr tmp, prev;
> +    virNetServerClientLock(client);
> +
> +    prev = NULL;
> +    tmp = client->filters;
> +    while (tmp) {
> +        if (tmp->id == filterID) {
> +            if (prev)
> +                prev->next = tmp->next;
> +            else
> +                client->filters = tmp->next;
> +
> +            VIR_FREE(tmp);
> +            break;
> +        }
> +        tmp = tmp->next;
> +    }

Where does prev get set in that loop?

> +
> +
> +virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock,
> +                                            int auth,
> +                                            bool readonly,
> +                                            virNetTLSContextPtr tls)
> +{
> +    virNetServerClientPtr client;
> +
> +    VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, tls);
> +
> +    if (VIR_ALLOC(client) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    if (virMutexInit(&client->lock) < 0)
> +        goto error;
> +
> +    client->refs = 1;
> +    client->sock = sock;
> +    client->auth = auth;
> +    client->readonly = readonly;
> +    client->tlsCtxt = tls;
> +    client->nrequests_max = 10; /* XXX */

Any plans to change this, such as making it a caller-settable parameter
(which the caller can then load from a conf file)?  Does it really matter?

> +
> +    if (tls)
> +        virNetTLSContextRef(tls);
> +
> +    /* Prepare one for packet receive */
> +    if (!(client->rx = virNetMessageNew()))
> +        goto error;
> +    client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX;
> +    client->nrequests = 1;
> +
> +    VIR_DEBUG("client=%p refs=%d", client, client->refs);
> +
> +    return client;
> +
> +error:
> +    /* XXX ref counting is better than this */
> +    client->sock = NULL; /* Caller owns 'sock' upon failure */
> +    virNetServerClientFree(client);

Should we clean this up first?

> +
> +void virNetServerClientRef(virNetServerClientPtr client)
> +{
> +    virNetServerClientLock(client);
> +    client->refs++;
> +    VIR_DEBUG("client=%p refs=%d", client, client->refs);
> +    virNetServerClientUnlock(client);

virObject and atomic ref-counting seems like it might be nice to get in
first.

> +bool virNetServerClientGetReadonly(virNetServerClientPtr client)
> +{
> +    bool readonly;
> +    virNetServerClientLock(client);
> +    readonly = client->readonly;
> +    virNetServerClientUnlock(client);
> +    return readonly;

Do we need to lock in order to read data that should never be changed
after the client is first created?

> +int virNetServerClientGetFD(virNetServerClientPtr client)
> +{
> +    int fd = 0;

Why do you init this to 0,

> +    virNetServerClientLock(client);
> +    fd = virNetSocketGetFD(client->sock);
> +    virNetServerClientUnlock(client);
> +    return fd;

if it will always get overwritten, and since -1 is a safer init value
for an fd?

> +bool virNetServerClientIsSecure(virNetServerClientPtr client)
> +{
> +    bool secure = false;
> +    virNetServerClientLock(client);
> +    if (client->tls)
> +        secure = true;
> +#if HAVE_SASL
> +    if (client->sasl)

else if

> +        secure = true;
> +#endif
> +    if (virNetSocketIsLocal(client->sock))

else if (no need to spend time on calling virNetSocketIsLocal if we
already had our answer from client->tls or client->sasl)

> +void virNetServerClientSetPrivateData(virNetServerClientPtr client,
> +                                      void *opaque,
> +                                      virNetServerClientFreeFunc ff)
> +{
> +    virNetServerClientLock(client);
> +
> +    if (client->privateData &&
> +        client->privateDataFreeFunc)
> +        client->privateDataFreeFunc(client->privateData);

Should this be deferred, and the old function and data cached for
calling after locks are dropped, so that the callback can't deadlock
with other client-locked functions?

> +void virNetServerClientFree(virNetServerClientPtr client)
> +{
> +    if (!client)
> +        return;
> +
> +    virNetServerClientLock(client);
> +    VIR_DEBUG("client=%p refs=%d", client, client->refs);
> +
> +    client->refs--;
> +    if (client->refs > 0) {
> +        virNetServerClientUnlock(client);
> +        return;
> +    }
> +
> +    if (client->privateData &&
> +        client->privateDataFreeFunc)
> +        client->privateDataFreeFunc(client->privateData);

Likewise to calling this outside lock?

> +/*
> + *
> + * We don't free stuff here, merely disconnect the client's
> + * network socket & resources.
> + *
> + * Full free of the client is done later in a safe point
> + * where it can be guaranteed it is no longer in use
> + */
> +void virNetServerClientClose(virNetServerClientPtr client)
> +{
> +    virNetServerClientLock(client);
> +    VIR_DEBUG("client=%p refs=%d", client, client->refs);
> +    if (!client->sock) {
> +        virNetServerClientUnlock(client);
> +        return;
> +    }
> +
> +    /* Do now, even though we don't close the socket
> +     * until end, to ensure we don't get invoked
> +     * again due to tls shutdown */
> +    if (client->sock)
> +        virNetSocketRemoveIOCallback(client->sock);
> +
> +    if (client->tls) {
> +        virNetTLSSessionFree(client->tls);
> +        client->tls = NULL;
> +    }
> +    if (client->sock) {
> +        virNetSocketFree(client->sock);
> +        client->sock = NULL;
> +    }
> +
> +    while (client->rx) {
> +        virNetMessagePtr msg
> +            = virNetMessageQueueServe(&client->rx);
> +        virNetMessageFree(msg);
> +    }
> +    while (client->tx) {
> +        virNetMessagePtr msg
> +            = virNetMessageQueueServe(&client->tx);
> +        virNetMessageFree(msg);
> +    }

Should we flush tx before rx, in case the act of flushing tx results in
a couple more responses received?

> +bool virNetServerClientIsClosed(virNetServerClientPtr client)
> +{
> +    bool closed;
> +    virNetServerClientLock(client);
> +    closed = client->sock == NULL ? true : false;

I would have written closed = (client->sock == NULL) or even closed =
!!client->sock

> +
> +/*
> + * Read data until we get a complete message to process
> + */
> +static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> +{
> +readmore:
> +    if (virNetServerClientRead(client) < 0) {
> +        client->wantClose = true;
> +        return; /* Error */
> +    }
> +
> +    if (client->rx->bufferOffset < client->rx->bufferLength)
> +        return; /* Still not read enough */
> +
> +    /* Either done with length word header */
> +    if (client->rx->bufferLength == VIR_NET_MESSAGE_LEN_MAX) {
> +        if (virNetMessageDecodeLength(client->rx) < 0)
> +            return;
> +
> +        virNetServerClientUpdateEvent(client);
> +
> +        /* Try and read payload immediately instead of going back
> +           into poll() because chances are the data is already
> +           waiting for us */
> +        goto readmore;
> +    } else {
> +        /* Grab the completed message */
> +        virNetMessagePtr msg = virNetMessageQueueServe(&client->rx);
> +        virNetServerClientFilterPtr filter;
> +
> +        /* Decode the header so we can use it for routing decisions */
> +        if (virNetMessageDecodeHeader(msg) < 0) {
> +            virNetMessageFree(msg);
> +            client->wantClose = true;
> +            return;
> +        }
> +
> +        /* Maybe send off for queue against a filter */
> +        filter = client->filters;
> +        while (filter) {
> +            int ret = filter->func(client, msg, filter->opaque);
> +            if (ret < 0 || ret > 0) {

ret != 0

When do the filters return > 0?

> +                virNetMessageFree(msg);
> +                msg = NULL;
> +                if (ret < 0)
> +                    client->wantClose = true;
> +                break;
> +            }
> +
> +            filter = filter->next;
> +        }
> +
> +        /* Send off to for normal dispatch to workers */
> +        if (msg) {
> +            if (!client->dispatchFunc ||
> +                client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {

Should we log the case of client->dispatchFunc being NULL?

> +
> +static void
> +virNetServerClientDispatchHandshake(virNetServerClientPtr client)
> +{
> +    int ret;
> +    /* Continue the handshake. */
> +    ret = virNetTLSSessionHandshake(client->tls);
> +    if (ret == 0) {
> +        /* Finished.  Next step is to check the certificate. */
> +        if (virNetServerClientCheckAccess(client) < 0)
> +            client->wantClose = true;
> +        else
> +            virNetServerClientUpdateEvent(client);
> +    } else if (ret > 0) {
> +        /* Carry on waiting for more handshake. Update
> +           the events just in case handshake data flow
> +           direction has changed */
> +        virNetServerClientUpdateEvent (client);

s/ (/(/

> +++ b/src/rpc/virnetserverclient.h
> @@ -0,0 +1,106 @@

> +
> +virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock,
> +                                            int auth,
> +                                            bool readonly,
> +                                            virNetTLSContextPtr tls);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4)

> +
> +int virNetServerClientAddFilter(virNetServerClientPtr client,
> +                                virNetServerClientFilterFunc func,
> +                                void *opaque);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK

> +
> +void virNetServerClientRemoveFilter(virNetServerClientPtr client,
> +                                    int filterID);

ATTRIBUTE_NONNULL(1)

> +
> +int virNetServerClientGetAuth(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +bool virNetServerClientGetReadonly(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +bool virNetServerClientHasTLSSession(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +int virNetServerClientGetTLSKeySize(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +#ifdef HAVE_SASL
> +void virNetServerClientSetSASLSession(virNetServerClientPtr client,
> +                                      virNetSASLSessionPtr sasl);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +#endif
> +
> +int virNetServerClientGetFD(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +bool virNetServerClientIsSecure(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +int virNetServerClientSetIdentity(virNetServerClientPtr client,
> +                                  const char *identity);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +const char *virNetServerClientGetIdentity(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +int virNetServerClientGetLocalIdentity(virNetServerClientPtr client,
> +                                       uid_t *uid, pid_t *pid);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)

> +
> +void virNetServerClientRef(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +typedef void (*virNetServerClientFreeFunc)(void *data);
> +
> +void virNetServerClientSetPrivateData(virNetServerClientPtr client,
> +                                      void *opaque,
> +                                      virNetServerClientFreeFunc ff);

ATTRIBUTE_NONNULL(1)

> +void *virNetServerClientGetPrivateData(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerClientSetDispatcher(virNetServerClientPtr client,
> +                                     virNetServerClientDispatchFunc func,
> +                                     void *opaque);

ATTRIBUTE_NONNULL(1)

> +void virNetServerClientClose(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +bool virNetServerClientIsClosed(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +void virNetServerClientMarkClose(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +bool virNetServerClientWantClose(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +
> +int virNetServerClientInit(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK

> +
> +const char *virNetServerClientLocalAddrString(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK

> +const char *virNetServerClientRemoteAddrString(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK

> +
> +int virNetServerClientSendMessage(virNetServerClientPtr client,
> +                                  virNetMessagePtr msg);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +
> +bool virNetServerClientNeedAuth(virNetServerClientPtr client);

ATTRIBUTE_NONNULL(1)

> +++ b/src/rpc/virnetserverprogram.c

> +static int
> +virNetServerProgramDispatchCall(virNetServerProgramPtr prog,
> +                                virNetServerPtr server,
> +                                virNetServerClientPtr client,
> +                                virNetMessagePtr msg)
> +{

> +    /*
> +     * When the RPC handler is called:
> +     *
> +     *  - Server object is unlocked
> +     *  - Client object is unlocked
> +     *
> +     * Without locking, it is safe to use:
> +     *
> +     *   'args and 'ret'

s/'args/'args'/

> +++ b/src/rpc/virnetserverprogram.h
> @@ -0,0 +1,107 @@

> +# define __VIR_NET_PROGRAM_H__
> +
> +# include <stdbool.h>

Provided by "internal.h"

> +
> +struct _virNetServerProgramProc {
> +    virNetServerProgramDispatchFunc func;
> +    size_t arg_len;
> +    xdrproc_t arg_filter;
> +    size_t ret_len;
> +    xdrproc_t ret_filter;
> +    bool needAuth;
> +};
> +
> +virNetServerProgramPtr virNetServerProgramNew(unsigned program,
> +                                              unsigned version,
> +                                              virNetServerProgramProcPtr procs,
> +                                              size_t nprocs);

ATTRIBUTE_NONNULL(3)

> +
> +int virNetServerProgramGetID(virNetServerProgramPtr prog);

ATTRIBUTE_NONNULL(1)

> +int virNetServerProgramGetVersion(virNetServerProgramPtr prog);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerProgramRef(virNetServerProgramPtr prog);

ATTRIBUTE_NONNULL(1)

> +
> +int virNetServerProgramMatches(virNetServerProgramPtr prog,
> +                               virNetMessagePtr msg);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +
> +int virNetServerProgramDispatch(virNetServerProgramPtr prog,
> +                                virNetServerPtr server,
> +                                virNetServerClientPtr client,
> +                                virNetMessagePtr msg);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4)

> +
> +int virNetServerProgramSendReplyError(virNetServerProgramPtr prog,
> +                                      virNetServerClientPtr client,
> +                                      virNetMessagePtr msg,
> +                                      virNetMessageErrorPtr rerr,
> +                                      virNetMessageHeaderPtr req);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)

> +
> +int virNetServerProgramSendStreamError(virNetServerProgramPtr prog,
> +                                       virNetServerClientPtr client,
> +                                       virNetMessagePtr msg,
> +                                       virNetMessageErrorPtr rerr,
> +                                       int procedure,
> +                                       int serial);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4)

> +
> +int virNetServerProgramSendStreamData(virNetServerProgramPtr prog,
> +                                      virNetServerClientPtr client,
> +                                      virNetMessagePtr msg,
> +                                      int procedure,
> +                                      int serial,
> +                                      const char *data,
> +                                      size_t len);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(6)

> +
> +++ b/src/rpc/virnetserverservice.c
> +
> +static void virNetServerServiceAccept(virNetSocketPtr sock,
> +                                      int events ATTRIBUTE_UNUSED,
> +                                      void *opaque)
> +{
> +    virNetServerServicePtr svc = opaque;
> +    virNetServerClientPtr client = NULL;
> +    virNetSocketPtr clientsock = NULL;
> +
> +    if (virNetSocketAccept(sock, &clientsock) < 0)
> +        goto error;
> +
> +    if (!clientsock) /* Connection already went away */
> +        goto cleanup;

Why not just return;?

> +
> +    if (!(client = virNetServerClientNew(clientsock,
> +                                         svc->auth,
> +                                         svc->readonly,
> +                                         svc->tls)))
> +        goto error;
> +
> +    if (!svc->dispatchFunc)
> +        goto error;
> +
> +    if (svc->dispatchFunc(svc, client, svc->dispatchOpaque) < 0)
> +        virNetServerClientClose(client);
> +
> +    virNetServerClientFree(client);
> +
> +cleanup:
> +    return;

and avoid an extra label?

> +++ b/src/rpc/virnetserverservice.h
> @@ -0,0 +1,65 @@

> +
> +virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename,
> +                                                 const char *service,
> +                                                 int auth,
> +                                                 bool readonly,
> +                                                 virNetTLSContextPtr tls);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)

> +virNetServerServicePtr virNetServerServiceNewUNIX(const char *path,
> +                                                  mode_t mask,
> +                                                  gid_t grp,
> +                                                  int auth,
> +                                                  bool readonly,
> +                                                  virNetTLSContextPtr tls);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(6)

> +
> +int virNetServerServiceGetAuth(virNetServerServicePtr svc);

ATTRIBUTE_NONNULL(1)

> +bool virNetServerServiceIsReadonly(virNetServerServicePtr svc);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerServiceRef(virNetServerServicePtr svc);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerServiceSetDispatcher(virNetServerServicePtr svc,
> +                                      virNetServerServiceDispatchFunc func,
> +                                      void *opaque);

ATTRIBUTE_NONNULL(1)

> +
> +void virNetServerServiceFree(virNetServerServicePtr svc);
> +
> +void virNetServerServiceToggle(virNetServerServicePtr svc,
> +                               bool enabled);

ATTRIBUTE_NONNULL(1)

Phew.  I finished reviewing this.  I think I found several points worth
fixing (both here and in the first two messages reviewing the same
mail), and you'll probably have other changes when rebasing to latest.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
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]