Re: [libvirt] [PATCH 07/12] Introduce generic RPC server objects
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)