Re: [PATCH 1/7] conf: clarify what is returned for actual bandwidth and vlan

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 21.02.2014 14:58, Laine Stump wrote:
> In practice, if a virDomainNetDef has a virDomainActualNetDef
> allocated, the ActualNetDef will *always* contain the bandwidth and
> vlan data from the NetDef (unless there was also a portgroup involved
> - see networkAllocateActualDevice()).
>
> However, virDomainNetGetActual(Bandwidth|Vlan)() were coded to make it
> appear as if it might be possible to have a valid bandwidth/vlan in
> the NetDef, but a NULL in the ActualNetDef. Believing this un-truth
> could lead to writing unnecessarily defensive code when dealing with
> the virDomainGetActual*() functions, so this patch makes it more
> obvious:
>
>     If there is an ActualNetDef, it will always have a copy of the
>     various appropriate bits from its parent NetDef, and the
>     virDomainGetActual* function will *always* return the data from the
>     ActualNetDef, not from the NetDef.
>
> The reason for this effective-NOP patch is that a subsequent patch to
> change virDomainNetDefFormat will rely on the above rule.
> ---
>   src/conf/domain_conf.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 064a40e..755066c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18638,8 +18638,11 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface)
>   virNetDevBandwidthPtr
>   virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
>   {
> +    /* if there is an ActualNetDef, *always* return
> +     * its bandwidth rather than the NetDef's bandwidth.
> +     */
>       if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> -        iface->data.network.actual && iface->data.network.actual->bandwidth) {
> +        iface->data.network.actual) {
>           return iface->data.network.actual->bandwidth;
>       }
>       return iface->bandwidth;
> @@ -18648,12 +18651,17 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
>   virNetDevVlanPtr
>   virDomainNetGetActualVlan(virDomainNetDefPtr iface)
>   {
> +    virNetDevVlanPtr vlan = &iface->vlan;
> +
> +    /* if there is an ActualNetDef, *always* return
> +     * its vlan rather than the NetDef's vlan.
> +     */
>       if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> -        iface->data.network.actual &&
> -        iface->data.network.actual->vlan.nTags > 0)
> -        return &iface->data.network.actual->vlan;
> -    if (iface->vlan.nTags > 0)
> -        return &iface->vlan;
> +        iface->data.network.actual)
> +        vlan = &iface->data.network.actual->vlan;
> +
> +    if (vlan->nTags > 0)
> +        return vlan;
>       return 0;

When you're at this, can you s/0/NULL/? We are returning a pointer after 
all.

>   }

ACK

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list