[PATCH 1/2] virnetdevopenvswitch: Get names for dpdkvhostuserclient too

[Daniel Henrique Barboza]

On 11/11/20 5:38 AM, Michal Privoznik wrote:
> There are two type of vhostuser ports:

s/type/types

> 
>    dpdkvhostuser - OVS creates the socket and QEMU connects to it
>    dpdkvhostuserclient - QEMU creates the socket and OVS connects to it
> 
> But of course ovs-vsctl syntax for fetching ifname is different.
> So far, we've implemented the former. The lack of implementation
> for the latter means that we are not detecting the interface name
> and thus not reporting it in domain XML, or failing to get
> interface statistics.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>   src/qemu/qemu_command.c         |  1 +
>   src/qemu/qemu_hotplug.c         |  1 +
>   src/util/virnetdevopenvswitch.c | 60 +++++++++++++++++++++++----------
>   src/util/virnetdevopenvswitch.h |  1 +
>   tests/qemuxml2argvmock.c        |  1 +
>   5 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b0c2a5efb5..0e803145d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8004,6 +8004,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>               goto cleanup;
>   
>           if (virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path,
> +                                                   net->data.vhostuser->data.nix.listen,
>                                                      &net->ifname) < 0)
>               goto cleanup;
>   
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 81bbe178a9..8c22075be1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1305,6 +1305,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>               goto cleanup;
>   
>           if (virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path,
> +                                                   net->data.vhostuser->data.nix.listen,
>                                                      &net->ifname) < 0)
>               goto cleanup;
>   
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index a7b6af594d..d809152f33 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -471,9 +471,20 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
>   /**
>    * virNetDevOpenvswitchVhostuserGetIfname:
>    * @path: the path of the unix socket
> + * @server: true if OVS creates the @path
>    * @ifname: the retrieved name of the interface
>    *
> - * Retrieves the ovs ifname from vhostuser unix socket path.
> + * Retrieves the OVS ifname from vhostuser UNIX socket path.
> + * There are two types of vhostuser ports which differ in client/server
> + * role:
> + *
> + * dpdkvhostuser - OVS creates the socket and QEMU connects to it
> + *                 (@server = true)
> + * dpdkvhostuserclient - QEMU creates the socket and OVS connects to it
> + *                       (@server = false)
> + *
> + * Since the way of retrieving ifname is different in these two cases,
> + * caller must set @server according to the interface definition.
>    *
>    * Returns: 1 if interface is an openvswitch interface,
>    *          0 if it is not, but no other error occurred,
> @@ -481,33 +492,46 @@ virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
>    */
>   int
>   virNetDevOpenvswitchGetVhostuserIfname(const char *path,
> +                                       bool server,
>                                          char **ifname)
>   {
> -    const char *tmpIfname = NULL;
> +    g_autoptr(virCommand) cmd = NULL;
>       int status;
> -    g_autoptr(virCommand) cmd = NULL;
>   
> -    /* Openvswitch vhostuser path are hardcoded to
> -     * /<runstatedir>/openvswitch/<ifname>
> -     * for example: /var/run/openvswitch/dpdkvhostuser0
> -     *
> -     * so we pick the filename and check it's a openvswitch interface
> -     */
> -    if (!path ||
> -        !(tmpIfname = strrchr(path, '/')))
> -        return 0;
> -
> -    tmpIfname++;
>       cmd = virCommandNew(OVS_VSCTL);
>       virNetDevOpenvswitchAddTimeout(cmd);
> -    virCommandAddArgList(cmd, "get", "Interface", tmpIfname, "name", NULL);
> -    if (virCommandRun(cmd, &status) < 0 ||
> -        status) {
> +
> +    if (server) {
> +        virCommandAddArgList(cmd, "--no-headings", "--columns=name", "find",
> +                             "Interface", NULL);
> +        virCommandAddArgPair(cmd, "options:vhost-server-path", "path");
> +    } else {
> +        const char *tmpIfname = NULL;
> +
> +        /* Openvswitch vhostuser path are hardcoded to

Either 'path is hardcoded to' or 'paths are hardcoded to'.

> +         * /<runstatedir>/openvswitch/<ifname>
> +         * for example: /var/run/openvswitch/dpdkvhostuser0
> +         *
> +         * so we pick the filename and check it's a openvswitch interface

s/a openvswitch/an openvswitch


ps: I am aware that those were existing typos and you just moved them, but might as
well fix it while you're at it :)


Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>



> +         */
> +        if (!path ||
> +            !(tmpIfname = strrchr(path, '/'))) {
> +            return 0;
> +        }
> +
> +        tmpIfname++;
> +        virCommandAddArgList(cmd, "get", "Interface", tmpIfname, "name", NULL);
> +    }
> +
> +    virCommandSetOutputBuffer(cmd, ifname);
> +    if (virCommandRun(cmd, &status) < 0)
> +        return -1;
> +
> +    if (status != 0) {
>           /* it's not a openvswitch vhostuser interface. */
>           return 0;
>       }
>   
> -    *ifname = g_strdup(tmpIfname);
>       return 1;
>   }
>   
> diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
> index c9ea592058..5cd1d22ae3 100644
> --- a/src/util/virnetdevopenvswitch.h
> +++ b/src/util/virnetdevopenvswitch.h
> @@ -61,6 +61,7 @@ int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
>       ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
>   
>   int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
> +                                           bool server,
>                                              char **ifname)
>       ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;
>   
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 2cdbe7e356..6900232b33 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -217,6 +217,7 @@ virCommandPassFD(virCommandPtr cmd,
>   
>   int
>   virNetDevOpenvswitchGetVhostuserIfname(const char *path G_GNUC_UNUSED,
> +                                       bool server G_GNUC_UNUSED,
>                                          char **ifname)
>   {
>       *ifname = g_strdup("vhost-user0");
>