Re: [libvirt PATCH v2 5/7] Add virNetworkObj Get and Set Methods for Metadata
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_kr(a)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