[libvirt] [PATCH] domain_conf: set ifname for ovs vhostuser ifname
Hi, This is the followup of '[PATCH v3 4/4] domain_conf: autodetect vhostuser ifname'. I have rewritten a better version that shouldn't have the same concern as before. I have isolated the code that detects the ifname in util/virnetdevopenvswitch.c in a new helper function virNetDevOpenvswitchGetVhostuserIfname(). Unlike the previous version, it returns a ifname only if the vhostuser interface is handled by openswitch. So, if the vhostuser is not handled by openvswitch, the ifname stay NULL. Now, virDomainNetDefParseXML() only calls this function, it does nothing else now. Cheers, Mehdi Abaakouk (1): domain_conf: set ifname for ovs vhostuser ifname src/conf/domain_conf.c | 5 +++++ src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 3 +++ 4 files changed, 50 insertions(+) -- 2.10.2
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/conf/domain_conf.c | 5 +++++ src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 3 +++ 4 files changed, 50 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7715cad..178208e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -55,6 +55,7 @@ #include "virtpm.h" #include "virstring.h" #include "virnetdev.h" +#include "virnetdevopenvswitch.h" #include "virhostdev.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -9582,6 +9583,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX; def->data.vhostuser->data.nix.path = vhostuser_path; + + if (!ifname) + ifname = virNetDevOpenvswitchGetVhostuserIfname(vhostuser_path); + vhostuser_path = NULL; if (STREQ(vhostuser_mode, "server")) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 867acdb..1b3d8d4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2047,6 +2047,7 @@ virNetDevMidonetUnbindPort; # util/virnetdevopenvswitch.h virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; +virNetDevOpenvswitchGetVhostuserIfname; virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index f003b3b..94b730b 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -377,3 +377,44 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchVhostuserGetIfname: + * @path: the path of the unix socket + * + * Retreives the ovs ifname from vhostuser unix socket path + * + * Returns a new string which must be deallocated by caller or NULL + */ +char * +virNetDevOpenvswitchGetVhostuserIfname(const char *path) +{ + virCommandPtr cmd = NULL; + char *ifname = NULL; + char **tokens = NULL; + size_t ntokens = 0; + + /* 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 ((tokens = virStringSplitCount(path, "/", 0, &ntokens))) { + if (VIR_STRDUP(ifname, tokens[ntokens - 1]) < 0) + goto cleanup; + } + if (ifname != NULL) { + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get", "Interface", + ifname, "name", NULL); + if (virCommandRun(cmd, NULL) < 0) { + /* it's not a openvswitch vhostuser interface */ + VIR_FREE(ifname); + } + } + + cleanup: + virStringListFreeCount(tokens, ntokens); + return ifname; + +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 0f9e1df..fe86e28 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -52,4 +52,7 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +char *virNetDevOpenvswitchGetVhostuserIfname(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */ -- 2.10.2
On 09.12.2016 13:24, 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/conf/domain_conf.c | 5 +++++ src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 3 +++ 4 files changed, 50 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7715cad..178208e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -55,6 +55,7 @@ #include "virtpm.h" #include "virstring.h" #include "virnetdev.h" +#include "virnetdevopenvswitch.h" #include "virhostdev.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -9582,6 +9583,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX; def->data.vhostuser->data.nix.path = vhostuser_path; + + if (!ifname) + ifname = virNetDevOpenvswitchGetVhostuserIfname(vhostuser_path);
This should go into post parse callback. I mean, I know that we still do fill out some defaults in parse phase, but we shouldn't really. Once domain XML is parsed, post parse callbacks are called and it their jobs to fill in missing pieces (or chose some sane defaults). qemuDomainDefPostParse() looks like a good candidate.
+ vhostuser_path = NULL;
if (STREQ(vhostuser_mode, "server")) { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 867acdb..1b3d8d4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2047,6 +2047,7 @@ virNetDevMidonetUnbindPort; # util/virnetdevopenvswitch.h virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; +virNetDevOpenvswitchGetVhostuserIfname; virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index f003b3b..94b730b 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -377,3 +377,44 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchVhostuserGetIfname: + * @path: the path of the unix socket + * + * Retreives the ovs ifname from vhostuser unix socket path + * + * Returns a new string which must be deallocated by caller or NULL + */ +char * +virNetDevOpenvswitchGetVhostuserIfname(const char *path) +{ + virCommandPtr cmd = NULL; + char *ifname = NULL; + char **tokens = NULL; + size_t ntokens = 0; + + /* 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 ((tokens = virStringSplitCount(path, "/", 0, &ntokens))) { + if (VIR_STRDUP(ifname, tokens[ntokens - 1]) < 0) + goto cleanup; + } + if (ifname != NULL) { + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get", "Interface", + ifname, "name", NULL); + if (virCommandRun(cmd, NULL) < 0) { + /* it's not a openvswitch vhostuser interface */ + VIR_FREE(ifname); + } + } + + cleanup: + virStringListFreeCount(tokens, ntokens); + return ifname;
The @cmd needs to be freed.
+ +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 0f9e1df..fe86e28 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -52,4 +52,7 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+char *virNetDevOpenvswitchGetVhostuserIfname(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */
Michal
Any comments on this ? On Fri, Dec 09, 2016 at 01:24:52PM +0100, Mehdi Abaakouk wrote:
Hi,
This is the followup of '[PATCH v3 4/4] domain_conf: autodetect vhostuser ifname'.
I have rewritten a better version that shouldn't have the same concern as before.
I have isolated the code that detects the ifname in util/virnetdevopenvswitch.c in a new helper function virNetDevOpenvswitchGetVhostuserIfname(). Unlike the previous version, it returns a ifname only if the vhostuser interface is handled by openswitch. So, if the vhostuser is not handled by openvswitch, the ifname stay NULL.
Now, virDomainNetDefParseXML() only calls this function, it does nothing else now.
Cheers,
Mehdi Abaakouk (1): domain_conf: set ifname for ovs vhostuser ifname
src/conf/domain_conf.c | 5 +++++ src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 3 +++ 4 files changed, 50 insertions(+)
-- 2.10.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
Hi, I have fixed the two remarks. ifname is now set in qemuDomainDeviceDefPostParse(). Regards, Mehdi Abaakouk (1): qemu_domain: set default ovs vhostuser ifname 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(-) -- 2.11.0
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) + goto cleanup; + } } /* set default disk types and drivers */ diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index f003b3b13..41933ce28 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -377,3 +377,45 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchVhostuserGetIfname: + * @path: the path of the unix socket + * + * Retreives the ovs ifname from vhostuser unix socket path + * + * Returns a new string which must be deallocated by caller or NULL + */ +char * +virNetDevOpenvswitchGetVhostuserIfname(const char *path) +{ + virCommandPtr cmd = NULL; + char *ifname = NULL; + char **tokens = NULL; + size_t ntokens = 0; + + /* 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 ((tokens = virStringSplitCount(path, "/", 0, &ntokens))) { + if (VIR_STRDUP(ifname, tokens[ntokens - 1]) < 0) + goto cleanup; + } + if (ifname != NULL) { + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get", "Interface", + ifname, "name", NULL); + if (virCommandRun(cmd, NULL) < 0) { + /* it's not a openvswitch vhostuser interface */ + VIR_FREE(ifname); + } + } + + cleanup: + virStringListFreeCount(tokens, ntokens); + virCommandFree(cmd); + return ifname; + +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 0f9e1dfa6..fe86e286f 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -52,4 +52,7 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +char *virNetDevOpenvswitchGetVhostuserIfname(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */ -- 2.11.0
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
participants (2)
-
Mehdi Abaakouk -
Michal Privoznik