[libvirt] [PATCH v2 1/1] qemu_domain: set default ovs vhostuser ifname

[Michal Privoznik]

On 21.12.2016 02:29, Mehdi Abaakouk wrote:
> This change adds a new helper function that
> return a ifname from the vhostuser unix-socket path but only
> if the interface is managed by openvswitch
> 
> This new function is issue to autodetect the ifname for openvswitch
> managed vhostuser interface.
> ---
>  src/libvirt_private.syms        |  1 +
>  src/qemu/qemu_domain.c          | 21 +++++++++++++++------
>  src/util/virnetdevopenvswitch.c | 42 +++++++++++++++++++++++++++++++++++++++++
>  src/util/virnetdevopenvswitch.h |  3 +++
>  4 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2d23e462d..9e4e8f83f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2057,6 +2057,7 @@ virNetDevMidonetUnbindPort;
>  # util/virnetdevopenvswitch.h
>  virNetDevOpenvswitchAddPort;
>  virNetDevOpenvswitchGetMigrateData;
> +virNetDevOpenvswitchGetVhostuserIfname;
>  virNetDevOpenvswitchInterfaceStats;
>  virNetDevOpenvswitchRemovePort;
>  virNetDevOpenvswitchSetMigrateData;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index acfa69589..7ae487821 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -41,6 +41,7 @@
>  #include "domain_addr.h"
>  #include "domain_event.h"
>  #include "virtime.h"
> +#include "virnetdevopenvswitch.h"
>  #include "virstoragefile.h"
>  #include "virstring.h"
>  #include "virthreadjob.h"
> @@ -3028,12 +3029,20 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>                                            def->emulator);
>      }
>  
> -    if (dev->type == VIR_DOMAIN_DEVICE_NET &&
> -        dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -        !dev->data.net->model) {
> -        if (VIR_STRDUP(dev->data.net->model,
> -                       qemuDomainDefaultNetModel(def, qemuCaps)) < 0)
> -            goto cleanup;
> +    if (dev->type == VIR_DOMAIN_DEVICE_NET) {
> +        if (dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +            !dev->data.net->model) {
> +            if (VIR_STRDUP(dev->data.net->model,
> +                           qemuDomainDefaultNetModel(def, qemuCaps)) < 0)
> +                goto cleanup;
> +        }
> +        if (dev->data.net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> +            !dev->data.net->ifname) {
> +            if (VIR_STRDUP(dev->data.net->ifname,
> +                virNetDevOpenvswitchGetVhostuserIfname(
> +                    dev->data.net->data.vhostuser->data.nix.path)) < 0)

Almost.

Firstly, virNetDevOpenvswitchGetVhostuserIfname() already returns an
allocated string. There is no need to duplicate it again and letting
this copy leak.
Secondly, this breaks tests (make check).
Thirdly, I think we want virNetDevOpenvswitchVhostuserGetIfname() to be
as quiet as possible. I mean, error out on OOM error but do not produce
any error if the ovs-vsctl command fails (e.g. it's not found, the ovs
bridge is not running). Which brings me to another point - we will
probably need to mock this function so that we get predictable test
results. We certainly do not want our test suite to actually ask
openvswitch anything.

Let my work in the changes and post the patch again.

Michal