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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list