Re: [libvirt] [PATCH 6/8] admin: Introduce virAdmConnectSetLoggingOutputs
On 11/22/2016 11:29 AM, Erik Skultety wrote:
>> };
>> 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 "").
I wonder if I was reading this as "src ? src : virLogGetDefaultOutput()"
Been too long for me to recall for sure, but I do see the *src now and
sure that does what you want and I think what I wrote (so what was a I
thinking?)...
> 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.
Usage of the API isn't necessary, but it can be done - some prefer one
way over another.
>
> 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.
An empty string is fine and no need to check for NULL too, unless you
think setting up an empty outputs would be useful for some other
designation (but we can leave that to the future).
John
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)
>>