6:24 a.m.
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
6:24 a.m.
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
11:01 a.m.
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
2:54 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Mehdi Abaakouk
mail: sileht(a)sileht.net
irc: sileht
7:29 p.m.
New subject: [libvirt] [PATCH v2 0/1] qemu_domain: set default ovs vhostuser ifname
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
7:29 p.m.
New subject: [libvirt] [PATCH v2 1/1] qemu_domain: set default ovs vhostuser ifname
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
3:45 a.m.
New subject: [libvirt] [PATCH v2 1/1] qemu_domain: set default ovs vhostuser ifname
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
2895
days inactive
2908
days old
6 comments
2 participants
participants (2)
-
Mehdi Abaakouk -
Michal Privoznik