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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110623/6f9f745f/attachment-0001.sig>