[libvirt] [PATCH v2 05/10] admin: Introduce virAdmConnectGetLoggingFilters

[Erik Skultety]

On Fri, Dec 09, 2016 at 06:59:57AM -0500, John Ferlan wrote:
> 
> 
> On 11/25/2016 08:12 AM, Erik Skultety wrote:
> > Enable libvirt users to query logging filter settings.
> > 
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  daemon/admin.c                  | 47 +++++++++++++++++++++++++++++++++++++++++
> >  include/libvirt/libvirt-admin.h |  4 ++++
> >  src/admin/admin_protocol.x      | 16 +++++++++++++-
> >  src/admin/admin_remote.c        | 35 ++++++++++++++++++++++++++++++
> >  src/admin_protocol-structs      |  8 +++++++
> >  src/libvirt-admin.c             | 41 +++++++++++++++++++++++++++++++++++
> >  src/libvirt_admin_private.syms  |  2 ++
> >  src/libvirt_admin_public.syms   |  1 +
> >  8 files changed, 153 insertions(+), 1 deletion(-)
> > 
> > diff --git a/daemon/admin.c b/daemon/admin.c
> > index 4fa3851..f3bc1de 100644
> > --- a/daemon/admin.c
> > +++ b/daemon/admin.c
> > @@ -399,6 +399,22 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags)
> >      return virLogGetNbOutputs();
> >  }
> >  
> > +/* Returns the number of defined filters or -1 in case of an error */
> > +static int
> > +adminConnectGetLoggingFilters(char **filters, unsigned int flags)
> > +{
> > +    char *tmp = NULL;
> > +    int ret = 0;
> > +
> > +    virCheckFlags(0, -1);
> > +
> > +    if ((ret = virLogGetNbFilters()) > 0 && !(tmp = virLogGetFilters()))
> > +        return -1;
> > +
> > +    *filters = tmp;
> > +    return ret;
> > +}
> > +
> >  static int
> >  adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED,
> >                                        virNetServerClientPtr client ATTRIBUTE_UNUSED,
> > @@ -420,4 +436,35 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED,
> >  
> >      return 0;
> >  }
> > +
> > +static int
> > +adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED,
> > +                                      virNetServerClientPtr client ATTRIBUTE_UNUSED,
> > +                                      virNetMessagePtr msg ATTRIBUTE_UNUSED,
> > +                                      virNetMessageErrorPtr rerr,
> > +                                      admin_connect_get_logging_filters_args *args,
> > +                                      admin_connect_get_logging_filters_ret *ret)
> > +{
> > +    char *filters = NULL;
> > +    int nfilters = 0;
> > +
> > +    if ((nfilters = adminConnectGetLoggingFilters(&filters, args->flags)) < 0) {
> > +        virNetMessageSaveError(rerr);
> > +        return -1;
> > +    }
> > +
> > +    if (nfilters == 0) {
> > +        ret->filters = NULL;
> > +    } else {
> > +        char **ret_filters = NULL;
> > +        if (VIR_ALLOC(ret_filters) < 0)
> > +            return -1;
> > +
> > +        *ret_filters = filters;
> > +        ret->filters = ret_filters;
> 
> So while this works - why the extra effort to allocate an array of 1
> item to stuff the returned filter into it?
> 
> While I understand outputs has a default and thus doesn't return NULL -
> the adminConnectGetLoggingFilters will return :
> 
> -1 on failure
> 0 w/ NULL filters
> # w/ non-NULL filters
> 
> So I believe you could still just VIR_STEAL_PTR(ret->filters, filters)
> as it doesn't distinguish if 'b' is NULL or not - it just assigns it.

I almost forgot to reply to this patch. Unfortunately, VIR_STEAL_PTR is not
enough and that's because of the way XDR stores pointers - string that actually
can be NULL is defined as char **, so you have to allocate the 1 member array
because what virNetServerProgramDispatchCall will then do is call xdr_free()
which will try to free both ret->filters and *ret->filters - it's just how XDR
defines it. I remember wanting to do it as you suggest (the intuitive way) but
then the daemon either crashed or I got an RPC error so I looked at the XDR spec
and found out that to be able to pass both NULL and string data in the same data
type, double pointer is needed.

Erik