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