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

On 06/22/2011 09:33 AM, Daniel P. Berrange wrote: > +++ b/src/Makefile.am > @@ -1187,7 +1187,7 @@ else > > +libvirt_net_rpc_server_la_SOURCES = \ > + rpc/virnetserverprogram.h rpc/virnetserverprogram.c \ > + rpc/virnetserverservice.h rpc/virnetserverservice.c \ > + rpc/virnetserverclient.h rpc/virnetserverclient.c \ > + rpc/virnetserver.h rpc/virnetserver.c > +libvirt_net_rpc_server_la_CFLAGS = \ > + $(AM_CFLAGS) > +libvirt_net_rpc_server_la_LDFLAGS = \ > + $(AM_LDFLAGS) \ > + $(CYGWIN_EXTRA_LDFLAGS) \ > + $(MINGW_EXTRA_LDFLAGS)l Umm, wonder why that spurious l isn't causing us grief?

> + > +virNetServerPtr virNetServerNew(size_t min_workers, > + size_t max_workers, > + size_t max_clients, > + virNetServerClientInitHook clientInitHook) > +{ > + virNetServerPtr srv; > + struct sigaction sig_action; > + > + if (VIR_ALLOC(srv) < 0) { > + virReportOOMError(); > + return NULL; > + } > + > + srv->refs = 1; > + > + if (!(srv->workers = virThreadPoolNew(min_workers, max_workers, > + virNetServerHandleJob, > + srv))) > + goto error; > + > + srv->nclients_max = max_clients; > + srv->sigwrite = srv->sigread = -1; > + srv->clientInitHook = clientInitHook; > + srv->privileged = geteuid() == 0 ? true : false; I'd have gone with the shorter: src->privileged = !geteuid();

> + > +bool virNetServerIsPrivileged(virNetServerPtr srv) > +{ > + bool priv; > + virNetServerLock(srv); > + priv = srv->privileged; > + virNetServerUnlock(srv); > + return priv; Does this really need to obtain a lock, since srv->privileged is never changed after construction?

> + > +static int virNetServerSignalSetup(virNetServerPtr srv) > +{ > + int fds[2]; > + > + if (srv->sigwrite != -1) > + return 0; > + > + if (pipe(fds) < 0) { If you use pipe2(fds, O_CLOEXEC|O_NONBLOCK),

> + > + /* Count of messages in the 'tx' queue, > + * and the server worker pool queue > + * ie RPC calls in progress. Does not count > + * 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 */ > + virNetServerClientFilterPtr filters; 'dx' queue? Did you mean 'rx'?