On 8/16/23 20:47, K Shiva Kiran wrote: > - Introduces virNetworkObjGetMetadata() and > virNetworkObjSetMetadata(). > - These functions implement common behaviour that can be reused by > network drivers. > - Introduces virNetworkObjUpdateModificationImpact() among other > helper functions that resolve the live/persistent state of > the network before setting metadata. > - Eliminates redundant call of virNetworkObjSetDefTransient() in > virNetworkConfigChangeSetup() among others. > - Substituted redundant logic in networkUpdate() with a call to > virNetworkObjUpdateModificationImpact(). > > Signed-off-by: K Shiva Kiran <shiva...@riseup.net> > --- > src/conf/virnetworkobj.c | 329 ++++++++++++++++++++++++++++++++++-- > src/conf/virnetworkobj.h | 21 +++ > src/libvirt_private.syms | 3 + > src/network/bridge_driver.c | 14 +- > src/test/test_driver.c | 16 +- > 5 files changed, 347 insertions(+), 36 deletions(-)
Again, way too much changes, disperse in semantics for one patch. You've introduced virNetworkObjUpdateModificationImpact(). Perfect! But it should have been one patch. Then you eliminate redundant call to virNetworkObjSetDefTransient()? Splendid, but again - it's a different change and has nothing to do with virNetworkObjSetDefTransient(). You implement new APIs? Sweet, but what do they have to do with the redundant call? Splitting patches per directory is not the same as "one semantic change per patch". Sometimes it is, e.g. in the series I've posted earlier today: https://listman.redhat.com/archives/libvir-list/2023-August/241416.html I know I might be asking too much, but try to put yourself into reviewers shoes. Libvirt's code base is not exactly the smallest and reviewing one change (and trying to think of all implications) is hard enough already. If changes are intertwined into one patch then it's needlessly harder. Michal
