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

[Eric Blake]

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 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/20110322/8c8023dd/attachment-0001.sig>