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 ebl...@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
