Re: [libvirt PATCH 3/4] Add virNetworkObj Get and Set Methods for Metadata

On 7/11/23 08:47, K Shiva Kiran wrote: > - Introduces virNetworkObjGetMetadata() and > virNetworkObjSetMetadata(). > - These functions implement common behaviour that can be reused by > network drivers that use the virNetworkObj struct. > - Also adds helper functions that resolve the live/persistent state of > the network before setting metadata. > > Signed-off-by: K Shiva Kiran<shiva_kr(a)riseup.net> > --- > src/conf/virnetworkobj.c | 325 +++++++++++++++++++++++++++++++++++++++ > src/conf/virnetworkobj.h | 17 ++ > 2 files changed, 342 insertions(+) > > diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c > index b8b86da06f..41d0a1d971 100644 > --- a/src/conf/virnetworkobj.c > +++ b/src/conf/virnetworkobj.c > @@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net, > > return 0; > } > + > + > +/** > + * virNetworkObjUpdateModificationImpact: > + * > + * @net: network object > + * @flags: flags to update the modification impact on > + * > + * Resolves virNetworkUpdateFlags in @flags so that they correctly > + * apply to the actual state of @net. @flags may be modified after call to this > + * function. > + * > + * Returns 0 on success if @flags point to a valid combination for @net or -1 on > + * error. > + */ > +static int > +virNetworkObjUpdateModificationImpact(virNetworkObj *net, > + unsigned int *flags) > +{ > + bool isActive = virNetworkObjIsActive(net); > + > + if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) == > + VIR_NETWORK_UPDATE_AFFECT_CURRENT) { > + if (isActive) > + *flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE; > + else > + *flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG; > + } > + > + if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("network is not running")); > + return -1; > + } > + > + if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("transient networks do not have any " > + "persistent config")); I'm going to mention it here, but it applies to the all of your patches, and all the new code in fact. We made an exemption for error messages and thus they don't need to be broken at 80 chars limit. In fact, they shouldn't. The reason is simple: easier grep as one doesn't have to try and guess how is the message broken into individual substrings. Of course, you will find plenty of "bad" examples throughout the code, but that's because it is an old code. Whenever we rewrite something or introduce new code this exception applies and the old code is, well, gradually fixed. > + return -1; > + } > + > + return 0; This code is basically the same as in networkUpdate(). The first part that sets _LIVE or _CONFIG is there verbatim, the second one is hidden under virNetworkObjConfigChangeSetup(). There's one extra step that the function does - it calls virNetworkObjSetDefTransient() but I don't think that's necessary. Either the network is active and virNetworkObjSetDefTransient() was already called, or is inactive and thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be removed. After this, virNetworkObjUpdateModificationImpact() could be exported (just like corresponding virDomain* sibling function is) so that it can be called from both src/conf/virnetworkobj.c and src/network/bridge_driver.c. Because I think we can get away with the networkUpdate() doing the following: networkUpdate() { ... virNetworkObjUpdateModificationImpact(); if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) { /* This is the check that possibly calls networkRemoveFirewallRules() and sets needFirewallRefresh = true */ } virNetworkObjUpdate(); ... } BTW: when cooking the patch, don't forget the same pattern is copied to our test driver (src/test/test_driver.c). Michal