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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list