Re: [libvirt] [PATCH v2 05/10] admin: Introduce virAdmConnectGetLoggingFilters
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(a)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