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

[Michal Prívozník]

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 at 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