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
s/alot/a lot/
objects.
* virNetServer: A server contains one or more services which accept incoming clients. It maintains the list of active clients. It has a list of RPC programs which can be used by clients. When clients produce a complete RPC message, the server passes this onto the corresponding program for handling, and queues any response back with the client.
* virNetServerClient: Encapsulates a single client connection. All I/O for the client is handled, reading & writing RPC messages.
* virNetServerProgram: Handles processing and dispatch of RPC method calls for a single RPC (program,version). Multiple programs can be registered with the server.
* virNetServerService: Encapsulates socket(s) listening for new connections. Each service listens on a single host/port, but may have multiple sockets if on a dual IPv4/6 host.
Each new daemon now merely has to define the list of RPC procedures & their handlers. It does not need to deal with any network related functionality at all.
We're now into the realm of this series where I've never before provided review comments, so it may take me a bit longer...
+++ b/src/rpc/virnetserver.c + +static void virNetServerHandleJob(void *jobOpaque, void *opaque) +{
Several control flow bugs. I'm prefixing them with numbers (all lines prefixed with 1 are in the same flow):
+ virNetServerPtr srv = opaque; + virNetServerJobPtr job = jobOpaque; + virNetServerProgramPtr prog = NULL; + size_t i; + + virNetServerClientRef(job->client); + + virNetServerLock(srv); + VIR_DEBUG("server=%p client=%p message=%p", + srv, job->client, job->msg); + + for (i = 0 ; i < srv->nprograms ; i++) { + if (virNetServerProgramMatches(srv->programs[i], job->msg)) { + prog = srv->programs[i]; + break; + } + } + + if (!prog) { + VIR_DEBUG("Cannot find program %d version %d", + job->msg->header.prog, + job->msg->header.vers); + goto error; + } + + virNetServerProgramRef(prog); + virNetServerUnlock(srv);
1. unlock...
+ + if (virNetServerProgramDispatch(prog, + srv, + job->client, + job->msg) < 0) + goto error;
1. error...
+ + virNetServerLock(srv); + virNetServerProgramFree(prog);
2. prog freed while server lock held
+ virNetServerUnlock(srv); + virNetServerClientFree(job->client); + + VIR_FREE(job); + return; + +error: + virNetServerUnlock(srv);
1. unlock. Oops - double-unlock is a recipe for disaster.
+ virNetServerProgramFree(prog);
2. prog freed while server lock released. Which should it be?
+ virNetMessageFree(job->msg); + virNetServerClientClose(job->client); + virNetServerClientFree(job->client); + VIR_FREE(job); +} + +
+ +static void virNetServerFatalSignal(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED, + void* context ATTRIBUTE_UNUSED) +{ + struct sigaction sig_action; + int origerrno; + + origerrno = errno; + virLogEmergencyDumpAll(sig); + + /* + * If the signal is fatal, avoid looping over this handler + * by desactivating it
s/desactivating/deactivating/
+ */ +#ifdef SIGUSR2 + if (sig != SIGUSR2) { +#endif + sig_action.sa_handler = SIG_IGN; + sigaction(sig, &sig_action, NULL);
Also need to sigemptyset(&sig_action.sa_mask), in order to keep valgrind happy (see commit fd21ecfd).
+virNetServerPtr virNetServerNew(size_t min_workers, + size_t max_workers, + size_t max_clients, + virNetServerClientInitHook clientInitHook)
Ran out of time to finish this review today. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org