On 03/18/2010 09:16 AM, Stefan Berger wrote: > This patch adds the implementation of the public API for the network > filtering (ACL) extensions to libvirt.c . > > Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com&gt; Some nits (again, leaving the content review to those more knowledgeable about API additions): > +virRegisterNWFilterDriver(virNWFilterDriverPtr driver) > +{ > + if (virInitialize() < 0) > + return -1; > + > + if (driver == NULL) { > + virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); > + return(-1); Why the two different styles of returning -1? A quick grep shows that the former style (return -1) is used nearly 9x more frequently than the latter (return(-1)).

> + DEBUG("nwfilter driver %d %s returned %s", > + i, virNWFilterDriverTab[i]->name, > + res == VIR_DRV_OPEN_SUCCESS ? "SUCCESS" : > + (res == VIR_DRV_OPEN_DECLINED ? "DECLINED" : > + (res == VIR_DRV_OPEN_ERROR ? "ERROR" : "unknown

> + if (res == VIR_DRV_OPEN_ERROR) { > + if (STREQ(virNWFilterDriverTab[i]->name, "remote")) { > + virLibConnWarning (NULL, VIR_WAR_NO_NWFILTER, > + "Is the daemon running ?"); Do DEBUG messages need to be marked for translation? Even if not, the virLibConnWarning probably should be.

-- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM]