Eric Blake <eblake@redhat.com> wrote on 03/18/2010 03:25:25 PM:

> 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@us.ibm.com>
>
> 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)).

Parts have been recycled from the storage driver and that's likely where that comes from.

>
> > +        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 status")));
> > +        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.

Correct. I will fix the error message.

 Thanks and regards,
   Stefan



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