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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list