[libvirt] [PATCH REPOST 20/38] virlog: Split output parsing and output defining to separate operations

[Erik Skultety]

On 10/05/16 03:11, Cole Robinson wrote:
> On 05/04/2016 10:30 AM, Erik Skultety wrote:
>> Everything has been prepared to successfully split parsing and defining logic
>> to separate operations.
>> ---
>>  src/util/virlog.c  | 160 +++++++++++++++++++++++++++++------------------------
>>  src/util/virlog.h  |  15 +++--
>>  tests/testutils.c  |  19 ++++++-
>>  tests/virlogtest.c |   5 +-
>>  4 files changed, 114 insertions(+), 85 deletions(-)
> 
> Hmm. I see what you were going for with this patch layout: line up all the
> pieces so you can internally convert everything in one fell swoop with patch
> #20 and #21. However that makes it difficult to review IMO, most of the
> critical change is bottled up in these few patches that depend on the previous
> 20 patches of context.
> 
> If instead you had (just an idea) renamed all the poorly named functions to
> more descriptive names (like virLogParse* functions to virLogParseAndSet*) up
> front, you could then split out virLogParse* and virLogSet* functions
> completely internally to virlog.c and transparent to the current users, the
> patches would be more individually self contained. Then you could convert all
> the current users to the new functions, and drop the old poorly named ones.
> 

Yeah, thanks for the advice, I'll try to do a better job next time.

> I realize reworking that will be a pain so I'll try and review as is (more
> tomorrow), but I'll jump around a bit. Also when posting the next round, I
> suggest just splitting out the logging rework first, then once all of that is
> committed, post a second series with the API bits.
> 
> - Cole
> 

No problem with splitting it into 2 batches, makes perfect sense.

Erik