6:50 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org