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@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