On 03/18/2011 12:54 PM, 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 objects.
Continuing my review...
+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;
Should this be a bool parameter passed in rather than an explicit geteuid() call up front? I know that elsewhere in the code (libvirtd.c:main), there is a comment stating: /* Beyond this point, nothing should rely on using * getuid/geteuid() == 0, for privilege level checks. * It must all use the flag 'server->privileged' * which is also passed into all libvirt stateful * drivers */ Also, I'm not a fan of (cond) ? true : false when cond is already type bool; the same code is produced for the shorter and still legible: srv->privileged = (geteuid() == 0);
+ +void virNetServerRef(virNetServerPtr srv) +{ + virNetServerLock(srv); + srv->refs++; + VIR_DEBUG("srv=%p refs=%d", srv, srv->refs); + virNetServerUnlock(srv);
It will be interesting to see if Hu's virObject proposal can ease this, but I suspect that your patch will go in first, at which point this is fine.
+bool virNetServerIsPrivileged(virNetServerPtr srv) +{ + bool priv; + virNetServerLock(srv); + priv = srv->privileged; + virNetServerUnlock(srv); + return priv;
Is srv->privileged ever modified after creation? If not, then locking may be overkill. Is it worth documenting which fields of virNetServerPtr are read-only once constructed?
+int virNetServerAddService(virNetServerPtr srv, + virNetServerServicePtr svc) +{
Aargh, out of time again (I've got to quit saving the review of this to the end of my day). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org