[libvirt] [PATCH 6/8] admin: Introduce virAdmConnectSetLoggingOutputs

[Erik Skultety]

> >  };
> > diff --git a/src/util/virlog.c b/src/util/virlog.c
> > index 4ac72dc..d04a461 100644
> > --- a/src/util/virlog.c
> > +++ b/src/util/virlog.c
> > @@ -1820,6 +1820,8 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters)
> >   * @outputs: string defining a (set of) output(s)
> >   *
> >   * Replaces the current set of defined outputs with a new set of outputs.
> > + * Should the set be empty, a default output is used according to the daemon's
> > + * runtime attributes.
> >   *
> >   * Returns 0 on success or -1 in case of an error.
> >   */
> > @@ -1828,12 +1830,13 @@ virLogSetOutputs(const char *src)
> >  {
> >      int ret = -1;
> >      int noutputs = 0;
> > +    const char *outputstr = *src ? src : virLogGetDefaultOutput();
> 
> Not sure this is right - virAdmConnectSetLoggingOutputs says 'outputs'
> must be NonNull.
> 
> I assume the generated remoteAdminConnectSetLoggingOutputs will end up
> calling adminConnectSetLoggingOutputs with a NonNull 'outputs'.
> 
> Thus this code gets called with NonNull outputs.
> 

So, what exactly are you proposing? Because what I understand from it now is
that I should probably allow passing NULL outputs in
virAdmConnectSetLoggingOutputs as a way of resetting the outputs to our defaults
(as an addition to "").

> Anyway, what you're really looking to do is check if the contents of
> 'outputs' is empty, then use some default value.  In this case, since
> this code "owns" virLogDefaultOutput is there really a reason to call
> the API?

True, I certainly don't have to make the call.

> 
> NB: even though virlog.h says outputs cannot be passed as NULL that's
> somewhat of a misnomer since all the compiler is checking is that the
> argument in the call stack isn't NULL - it doesn't check if the argument
> being passed in actually NULL.
> 

Yes, but this should be actual check for NULL should be the case when we're
handling user-passed values in which case as it is right now, there's no need,
user can't pass NULL and in all the other cases, our internal callers should
respect the ATTRIBUTE_NONNULL constraint. Of course, if you were suggesting to
actually allow users to pass NULL to the public API^^, then yes, this has to be
adjusted properly.

Thanks,
Erik

> John
> 
> >      virLogOutputPtr *outputs = NULL;
> >  
> >      if (virLogInitialize() < 0)
> >          return -1;
> >  
> > -    if ((noutputs = virLogParseOutputs(src, &outputs)) < 0)
> > +    if ((noutputs = virLogParseOutputs(outputstr, &outputs)) < 0)
> >          goto cleanup;
> >  
> >      if (virLogDefineOutputs(outputs, noutputs) < 0)
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161122/8a651f08/attachment-0001.sig>