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

[Daniel P. Berrange]

On Thu, Jun 23, 2011 at 05:50:27PM -0600, Eric Blake wrote:
> 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?

I expect libtool discards it for some reason

> > +
> > +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();

I find the explicit conditional a little bit more
immediately obvious to read.

> > +
> > +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?

I guess not, but I guess I didn't want to special
case this method

> 
> > +
> > +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),

Ah good idea.

> > +
> > +    /* 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'?

No, messages are on the 'rx' queue while data is
being read. When a complete message has been read
the message is moved from 'rx' to 'dx' queue for
processing by a worker. The filter causes the
message to go elsewhere, instead of the 'dx' queue.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|