On 06/22/2011 09:33 AM, 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/ I think this is the point where I haven't really reviewed your series in the past.
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.
+++ 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?
+struct _virNetServerJob { + virNetServerClientPtr client; + virNetMessagePtr msg; +}; + +struct _virNetServer { + int refs;
Whatever happened to the virObject patches, to ease reference counting? But what you have is fine for now.
+ +static void virNetServerFatalSignal(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED, + void* context ATTRIBUTE_UNUSED)
Style nit in the spacing of pointers: siginfo_t *siginfo void *context
+{ + 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/
+ +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),
+ virReportSystemError(errno, "%s", + _("Unable to create signal pipe")); + return -1; + } + + if (virSetNonBlock(fds[0]) < 0 || + virSetNonBlock(fds[1]) < 0 || + virSetCloseExec(fds[0]) < 0 || + virSetCloseExec(fds[1]) < 0) { + virReportSystemError(errno, "%s", + _("Failed to setup pipe flags")); + goto error; + }
then you can drop this entire if clause.
+++ b/src/rpc/virnetserver.h @@ -0,0 +1,80 @@
+ +#ifndef __VIR_NET_SERVER_H__ +# define __VIR_NET_SERVER_H__ + +# include <stdbool.h>
Is this one necessary?
+ + /* 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'?
+ +bool virNetServerClientIsClosed(virNetServerClientPtr client) +{ + bool closed; + virNetServerClientLock(client); + closed = client->sock == NULL ? true : false;
Shorter as closed = !client->sock
+++ b/src/rpc/virnetserverprogram.h @@ -0,0 +1,107 @@
+ +#ifndef __VIR_NET_PROGRAM_H__ +# define __VIR_NET_PROGRAM_H__ + +# include <stdbool.h>
Needed? ACK with nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org