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

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.

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