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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 21/09/16 19:55, John Ferlan wrote:
> 
> 
> On 08/18/2016 07:47 AM, Erik Skultety wrote:
>> Continuing with the refactor, in order to later split output parsing and output
> 
> s/Continuing with the refactor, i/I
> 
>> defining, introduce a new function which will create a new virLogOutput object
>> which parser will insert into a list with the list being eventually defined.
> 
> s/which/which the/
> 
>>
>> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virlog.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/util/virlog.h        |  6 ++++++
>>  3 files changed, 61 insertions(+), 1 deletion(-)
>>
> 
> When I first started reviewing I wondered what the deal with 'journalfd'
> was - why was it a global and not handled like the file fd.  Then
> eventually I read patch 19...
> 
> Would it be too painful to move patch 19 to in between 1 and 2?  It's
> not that important since in the long run things work out.  If you do
> decide to make that move, then of course the intervening patch 7 would
> need to pass journalfd..
> 

Sure, no problem at all.

>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 35200a3..b5cee5f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1854,6 +1854,7 @@ virLogLock;
>>  virLogMessage;
>>  virLogOutputFree;
>>  virLogOutputListFree;
>> +virLogOutputNew;
>>  virLogParseAndDefineFilters;
>>  virLogParseAndDefineOutputs;
>>  virLogParseDefaultPriority;
>> diff --git a/src/util/virlog.c b/src/util/virlog.c
>> index 3ada288..91c63a1 100644
>> --- a/src/util/virlog.c
>> +++ b/src/util/virlog.c
>> @@ -366,7 +366,6 @@ virLogOutputFree(virLogOutputPtr output)
>>          output->c(output->data);
>>      VIR_FREE(output->name);
>>      VIR_FREE(output);
>> -
>>  }
>>  
>>  
>> @@ -1551,3 +1550,57 @@ bool virLogProbablyLogMessage(const char *str)
>>          ret = true;
>>      return ret;
>>  }
>> +
>> +
>> +/**
>> + * virLogOutputNew:
>> + * @f: the function to call to output a message
>> + * @c: the function to call to close the output (or NULL)
>> + * @data: extra data passed as first arg to the function
>> + * @priority: minimal priority for this filter, use 0 for none
>> + * @dest: where to send output of this priority (see virLogDestination)
>> + * @name: optional name data associated with an output
>> + *
>> + * Allocates and returns a new log output object. The object has to be later
>> + * defined, so that the output will be taken into account when emitting a
>> + * message.
>> + *
>> + * Returns reference to a newly created object or NULL in case of failure.
>> + */
>> +virLogOutputPtr
>> +virLogOutputNew(virLogOutputFunc f,
>> +                virLogCloseFunc c,
>> +                void *data,
>> +                virLogPriority priority,
>> +                virLogDestination dest,
>> +                const char *name)
>> +{
>> +    virLogOutputPtr ret = NULL;
>> +    char *ndup = NULL;
>> +
>> +    if (!f)
>> +        return NULL;
> 
> I think instead of this, modify the prototype to be ATTRIBUTE_NONNULL(1)
> to ensure you're passed a function... [1]
> 
>> +
>> +    if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) {
>> +        if (!name)
>> +            return NULL;
> 
> The above two fail without a message which could percolate some day as a
> "failed for some unknown reason"
> 
>> +
>> +        if (VIR_STRDUP(ndup, name) < 0)
>> +            return NULL;
>> +    }
>> +
>> +    if (VIR_ALLOC_QUIET(ret) < 0) {
>> +        VIR_FREE(ndup);
>> +        return NULL;
>> +    }
> 
> The above two will fail with a OOM which is fine - just pointing out the
> difference...
> 
> So if you add a message for the first !name check that would be good.
> 
>> +
>> +    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.

Erik

> 
> (and if patch 19 moves before here, then "data" could be further checked
> to be non NULL (I think). In fact in this case, the ATTRIBUTE_NONNULL(2)
> and ATTRIBUTE_NONNULL(3) could be used instead of an error.
> 
>> +    ret->priority = priority;
>> +    ret->dest = dest;
>> +    ret->name = ndup;
>> +
>> +    return ret;
>> +}
>> diff --git a/src/util/virlog.h b/src/util/virlog.h
>> index de64f4c..fb32c41 100644
>> --- a/src/util/virlog.h
>> +++ b/src/util/virlog.h
>> @@ -226,5 +226,11 @@ void virLogVMessage(virLogSourcePtr source,
>>                      va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0);
>>  
>>  bool virLogProbablyLogMessage(const char *str);
>> +virLogOutputPtr virLogOutputNew(virLogOutputFunc f,
>> +                                virLogCloseFunc c,
>> +                                void *data,
>> +                                virLogPriority priority,
>> +                                virLogDestination dest,
>> +                                const char *name);
> 
> [1]
> s/);/)
>     ATTRIBUTE_NONNULL(1);
> 
> ACK with the adjustments,
> 
> John
>>  
>>  #endif
>>


Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]