[libvirt] [PATCH v2 02/20] virlog: Introduce virLogOutputNew

[John Ferlan]

[...]
>>> +
>>> +    ret->logInitMessage = true;
>>> +    ret->f = f;
>>> +    ret->c = c;
>>> +    ret->data = data;
>>
>> From future patches I see this can be a file or syslog fd.
>>
>> Anyway, because you're relying on the "->c" to be the free function for
>> ->data, perhaps there should be a check above that causes an error if
>> "data" was passed as non-NULL, but ->c is NULL; otherwise, in some
>> future world someone may begin to leak data unexpectedly.
> 
> I think having non-NULL data with NULL free callback in general is a
> valid use-case especially if the data is void * and you store integers
> in it. Anyway, the problem here are stderr and syslog where you have
> NULL in the close callback and a file descriptor in @data for the former
> case, which you really do not want to close anyway, and a valid close
> callback with NULL @data (syslog keeps the file descriptor private) for
> the latter. While I can imagine having a dummy close callback for stderr
> which would just return instantly (however I'd rather avoid that), I
> really don't want to pass an arbitrary pointer to @data for syslog-based
> output just to satisfy ATTRIBUTE_NONNULL(3), even though it would be
> ignored by the syslog close callback.
> 
> Let me know if I misunderstood your thoughts, I'll continue fixing the
> rest of the patches in the meantime.
> 

Yeah - I think I realized this later too (patch 7)... I guess I was
"over"thinking the fd/journalfd usage where we want someone to "forget"
somehow to free the resource we're about to "steal" and store.

So ignore the whole ATTRIBUTE_NONNULL(3)...

John