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

Re: [libvirt] [PATCH REPOST 04/38] virlog: Export virLogOutputPtr through header



On 10/05/16 02:08, Cole Robinson wrote:
> On 05/04/2016 10:30 AM, Erik Skultety wrote:
>> It needs to be exported, since some caller might (for some reason) want to
>> create a logging output without calling the parser which does this. Also,
>> some methods will use virLogOutputPtr as data type for one of its arguments.
>> ---
>>  src/util/virlog.c | 2 --
>>  src/util/virlog.h | 3 +++
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virlog.c b/src/util/virlog.c
>> index 812e2cd..0be1701 100644
>> --- a/src/util/virlog.c
>> +++ b/src/util/virlog.c
>> @@ -106,8 +106,6 @@ struct _virLogOutput {
>>      virLogDestination dest;
>>      char *name;
>>  };
>> -typedef struct _virLogOutput virLogOutput;
>> -typedef virLogOutput *virLogOutputPtr;
>>  
>>  static virLogOutputPtr *virLogOutputs;
>>  static size_t virLogNbOutputs;
>> diff --git a/src/util/virlog.h b/src/util/virlog.h
>> index b5056f5..7706d22 100644
>> --- a/src/util/virlog.h
>> +++ b/src/util/virlog.h
>> @@ -130,6 +130,9 @@ struct _virLogMetadata {
>>  typedef struct _virLogMetadata virLogMetadata;
>>  typedef struct _virLogMetadata *virLogMetadataPtr;
>>  
>> +typedef struct _virLogOutput virLogOutput;
>> +typedef virLogOutput *virLogOutputPtr;
>> +
>>  /**
>>   * virLogOutputFunc:
>>   * @src: the source of the log message
>>
> 
> ACK, but IMO exporting it early in a separate patch without context makes it
> hard to follow the reasoning. Better would have been to export it with the
> first public function that needs it, looks like virLogDefineOutputs
> 
> - Cole
>

I tried to break all the changes into as many bits as possible, so that
it could be reviewed more easily and I did it with the best intentions,
but I have to admit that you're absolutely right and without any
context, this can be very hard to follow for a reviewer. Also, for the
sake of commit history, I think squashing this change into the commit
where I'm introducing virLogDefineOutputs might be a better choice than
pushing this as a standalone patch. Analogically, I'll squash 5/38 into
the commit which introduces virLogDefineFilters.

Erik


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