[libvirt] [PATCH 0/3] support VLan for virtual network based on 8021q kernel module
Support VLan for virtual network based on 8021q kernel module other than ovs backend. Insert vlan-device into the datapath of the network traffic: (outside of host) <--> physical_interface <--> vlan-dev(with VLAN-Tag) <--> bridge <--> guests Then VLAN-Tag will be applied to the guest's network traffic. The VLan network acts as real layer-2 switch which provides 'access' port to guest. 1. Create VLan network by setting mode='vlan' on the forward element, for example: ... <network> <name>vlan10</name> <forward mode="vlan" dev="eth0"/> <vlan> <tag id="10"/> </vlan> </network> ... The mode attribute of <forward> should be 'vlan' (this patch added). The dev attribute of <forward> specifies a physical interface which forwards traffice between this VLan-network and outside. The id attribue of the vlan tag indicates VLAN-Tag. Both vlan element and tag element should be unique in this xml. A guest connects to this VLan network by setting its xml like this: <interface type='network'> <source network='vlan10'/> </interface> 2. We can enable dhcp for VLan network according to the current way, for example: ... <network> ... <ip address="192.168.126.2" netmask="255.255.255.0"> <dhcp> <range start="192.168.126.120" end="192.168.126.129"/> </dhcp> </ip> </network> ... 3. This can help to build Cross-Host VLan network for guests. We can simplify the work to implement vlan-net of management app (e.g. OpenStack). 1) Distribute the xml of VLan network to all hosts, then 'virsh net-create ...' on each host locally. 2) Makesure the outside switch's port linked to the physical interface (specified by the dev of <forward>) is 'trunk' mode. 3) For each VLan network, ONLY one host can 'net-create' network with ip and dhcp element to avoid dhcp conflict. Shi Lei (3): add functions: load(verify) 8021q module, create/destroy vlan-dev support new forward mode of vlan for virtual network fix other functions to add VIR_NETWORK_FORWARD_VLAN configure.ac | 5 +- src/conf/domain_conf.c | 1 + src/conf/network_conf.c | 12 ++- src/conf/network_conf.h | 1 + src/conf/virnetworkobj.c | 1 + src/libvirt_private.syms | 4 + src/network/bridge_driver.c | 80 ++++++++++++++++-- src/qemu/qemu_process.c | 1 + src/util/virnetdev.c | 195 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 14 ++++ 10 files changed, 301 insertions(+), 13 deletions(-) -- 2.7.4
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- configure.ac | 5 +- src/libvirt_private.syms | 4 + src/util/virnetdev.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 14 ++++ 4 files changed, 216 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 59d2d09..141d5b2 100644 --- a/configure.ac +++ b/configure.ac @@ -769,8 +769,9 @@ then fi AM_CONDITIONAL([WITH_NODE_DEVICES], [test "$with_nodedev" = "yes"]) -dnl GET_VLAN_VID_CMD is required for virNetDevGetVLanID -AC_CHECK_DECLS([GET_VLAN_VID_CMD], [], [], [[#include <linux/if_vlan.h>]]) +dnl GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD is required +AC_CHECK_DECLS([GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD], + [], [], [[#include <linux/if_vlan.h>]]) # Check for Linux vs. BSD ifreq members AC_CHECK_MEMBERS([struct ifreq.ifr_newname, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e30490..a61ba02 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2266,7 +2266,9 @@ virModuleLoad; # util/virnetdev.h virNetDevAddMulti; +virNetDevCreateVLanDev; virNetDevDelMulti; +virNetDevDestroyVLanDev; virNetDevExists; virNetDevFeatureTypeFromString; virNetDevFeatureTypeToString; @@ -2287,10 +2289,12 @@ virNetDevGetRxFilter; virNetDevGetVirtualFunctionIndex; virNetDevGetVirtualFunctionInfo; virNetDevGetVirtualFunctions; +virNetDevGetVLanDevName; virNetDevGetVLanID; virNetDevIfStateTypeFromString; virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; +virNetDevLoad8021Q; virNetDevPFGetVF; virNetDevReadNetConfig; virNetDevRunEthernetScript; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c20022f..50a81d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,6 +44,7 @@ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_UNIX +# include "virkmod.h" #elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL) # define VIR_NETDEV_FAMILY AF_LOCAL #else @@ -1048,6 +1049,200 @@ int virNetDevGetVLanID(const char *ifname ATTRIBUTE_UNUSED, #endif /* ! SIOCGIFVLAN */ +#if defined(HAVE_STRUCT_IFREQ) + +# define MODULE_8021Q "8021q" +# define PROC_NET_VLAN_CONFIG "/proc/net/vlan/config" + +static int +validate8021Q(void) +{ + int fd; + if ((fd = open(PROC_NET_VLAN_CONFIG, O_RDONLY)) < 0) + return -1; + VIR_FORCE_CLOSE(fd); + return 0; +} + +static int +getVlanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) +{ + if (!vdname || vdname_size < IFNAMSIZ) + return -1; + if (snprintf(vdname, IFNAMSIZ, "%s.%d", ifname, vlanid) >= IFNAMSIZ) + return -1; + return 0; +} + +static int +controlVlanDev(unsigned int cmd, + const char *ifname, unsigned int vlanid) +{ + int fd; + struct vlan_ioctl_args if_request; + memset(&if_request, 0, sizeof(struct vlan_ioctl_args)); + if_request.cmd = cmd; + + if (cmd == ADD_VLAN_CMD) { + strcpy(if_request.device1, ifname); + if_request.u.VID = vlanid; + } else if (cmd == DEL_VLAN_CMD) { + char vdname[IFNAMSIZ]; + if (getVlanDevName(ifname, vlanid, vdname, sizeof(vdname)) < 0) + return -1; + strcpy(if_request.device1, vdname); + } else { + virReportSystemError(ENOSYS, + _("unsupport cmd: %d"), cmd); + return -1; + } + + if ((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("unable to open control socket")); + return -1; + } + + if (ioctl(fd, SIOCSIFVLAN, &if_request) < 0) { + virReportSystemError(errno, "%s", + _("control VLAN device error")); + VIR_FORCE_CLOSE(fd); + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + +/** + * virNetDevLoad8021Q: + * + * Load 8021q module (since kernel v2.6) + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevLoad8021Q(void) +{ + if (validate8021Q() < 0) { + char *errbuf = NULL; + if ((errbuf = virKModLoad(MODULE_8021Q, false))) { + VIR_FREE(errbuf); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to load 8021q module")); + return -1; + } + if (validate8021Q() < 0) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot load 8021q module")); + return -1; + } + } + return 0; +} + +/** + * virNetDevCreateVLanDev: + * @ifname: name of interface we will create vlan-device on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Create vlan-device which based on 8021q module. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) +{ + if (controlVlanDev(ADD_VLAN_CMD, ifname, vlanid) < 0) + return -1; + return getVlanDevName(ifname, vlanid, vdname, vdname_size); +} + +/** + * virNetDevDestroyVLanDev: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * + * Destroy vlan-device whick has created by virNetDevCreateVLanDev. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) +{ + if (controlVlanDev(DEL_VLAN_CMD, ifname, vlanid) < 0) + return -1; + return 0; +} + +/** + * virNetDevGetVLanDevName: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Get the name of the vlan-device + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) +{ + return getVlanDevName(ifname, vlanid, vdname, vdname_size); +} + +#else /* !HAVE_STRUCT_IFREQ */ + +int +virNetDevLoad8021Q(void) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to load 8021q module on this platform")); + return -1; +} + +int +virNetDevCreateVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char *vdname[] ATTRIBUTE_UNUSED, + size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to create vlan-dev on this platform")); + return -1; +} + +int +virNetDevDestroyVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform")); + return -1; +} + +int +virNetDevGetVLanDevName(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char vdname[] ATTRIBUTE_UNUSED, + size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform")); + return -1; +} + +#endif /* HAVE_STRUCT_IFREQ */ + + /** * virNetDevValidateConfig: * @ifname: Name of the interface diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 71eaf45..40fb6ee 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -206,6 +206,20 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) int virNetDevGetVLanID(const char *ifname, int *vlanid) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevLoad8021Q(void) + ATTRIBUTE_RETURN_CHECK; + +int virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + +int virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +int virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + int virNetDevGetMaster(const char *ifname, char **master) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -- 2.7.4
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- configure.ac | 5 +- src/libvirt_private.syms | 4 + src/util/virnetdev.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 14 ++++ 4 files changed, 216 insertions(+), 2 deletions(-)
Not exactly in my wheelhouse of knowledge and I suspect when Laine gets back from vacation he'd want to look as well, but ... let's see what feedback I can provide. I'll probably make a mess, but hopefully its understandable. First off kudos for actually providing patches first time out that compile and pass make check syntax-check!
diff --git a/configure.ac b/configure.ac index 59d2d09..141d5b2 100644 --- a/configure.ac +++ b/configure.ac @@ -769,8 +769,9 @@ then fi AM_CONDITIONAL([WITH_NODE_DEVICES], [test "$with_nodedev" = "yes"])
-dnl GET_VLAN_VID_CMD is required for virNetDevGetVLanID
Losing this comment for a more generic one probably isn't good.
-AC_CHECK_DECLS([GET_VLAN_VID_CMD], [], [], [[#include <linux/if_vlan.h>]]) +dnl GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD is required
Rather than combining into one and losing the virNetDevGetVLanID why not add separate comments "ADD_VLAN_CMD is required for virNetDevCreateVLanDev" and "DEL_VLAN_CMD is required for virNetDevDestroyVLanDev".
+AC_CHECK_DECLS([GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD], + [], [], [[#include <linux/if_vlan.h>]])
# Check for Linux vs. BSD ifreq members AC_CHECK_MEMBERS([struct ifreq.ifr_newname, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e30490..a61ba02 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2266,7 +2266,9 @@ virModuleLoad;
# util/virnetdev.h virNetDevAddMulti; +virNetDevCreateVLanDev; virNetDevDelMulti; +virNetDevDestroyVLanDev; virNetDevExists; virNetDevFeatureTypeFromString; virNetDevFeatureTypeToString; @@ -2287,10 +2289,12 @@ virNetDevGetRxFilter; virNetDevGetVirtualFunctionIndex; virNetDevGetVirtualFunctionInfo; virNetDevGetVirtualFunctions; +virNetDevGetVLanDevName; virNetDevGetVLanID; virNetDevIfStateTypeFromString; virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; +virNetDevLoad8021Q; virNetDevPFGetVF; virNetDevReadNetConfig; virNetDevRunEthernetScript; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c20022f..50a81d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,6 +44,7 @@ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_UNIX +# include "virkmod.h" #elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL) # define VIR_NETDEV_FAMILY AF_LOCAL #else @@ -1048,6 +1049,200 @@ int virNetDevGetVLanID(const char *ifname ATTRIBUTE_UNUSED, #endif /* ! SIOCGIFVLAN */
+#if defined(HAVE_STRUCT_IFREQ) + +# define MODULE_8021Q "8021q" +# define PROC_NET_VLAN_CONFIG "/proc/net/vlan/config" + +static int +validate8021Q(void)
I think this could be replaced in the callers with a: virFileExists(PROC_NET_VLAN_CONFIG) since all you seem to care is that the file got created. The open here is not doing much other than proving that this process could open the file. There's nothing be read AFAICT.
+{ + int fd; + if ((fd = open(PROC_NET_VLAN_CONFIG, O_RDONLY)) < 0) + return -1; + VIR_FORCE_CLOSE(fd); + return 0; +} +
Adding new functions should keep 2 empty lines between each.
+static int +getVlanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size)
One line per argument please. Why is it 'char vdname[]' and not 'char **'?
+{
I think a : if (!ifname) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Interface name not provided")); return -1; } should be added.
+ if (!vdname || vdname_size < IFNAMSIZ) + return -1;
So if this fails eventually a caller is going to want to know why. Adding a virReportError here about invalid arguments would be OK, but perhaps unnecessary. The @vdname is an output that if not provided is just wrong coding practices. I don't believe vdname_size is relevant can be removed.
+ if (snprintf(vdname, IFNAMSIZ, "%s.%d", ifname, vlanid) >= IFNAMSIZ)
use "return virAsprintf(vdname, "%s.%d", ifname, vlandid);" it'll magically do the allocation and return - not really caring about IFNAMSIZ "stuff". See virNetDevSysfsFile for the example. The virNetDevGetVLanDevName should be moved here and then the subsequent callers can use it rather than this helper by name.
+ return -1; + return 0; +} + +static int +controlVlanDev(unsigned int cmd, + const char *ifname, unsigned int vlanid)
One line per argument.
+{
int ret = -1;
+ int fd; + struct vlan_ioctl_args if_request; + memset(&if_request, 0, sizeof(struct vlan_ioctl_args)); + if_request.cmd = cmd; +
Again, add the : if (!ifname) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Interface name not provided")); return -1; }
+ if (cmd == ADD_VLAN_CMD) { + strcpy(if_request.device1, ifname); + if_request.u.VID = vlanid; + } else if (cmd == DEL_VLAN_CMD) { + char vdname[IFNAMSIZ];
char *vdname = NULL;
+ if (getVlanDevName(ifname, vlanid, vdname, sizeof(vdname)) < 0)
if (virNetDevGetVLanDevName(ifname, vlanid, &vdname) < 0)
+ return -1; + strcpy(if_request.device1, vdname);
Hmmm... I know I said the length of the created name doesn't matter, but this strcpy would seem to be suspicious... let's see, I see "vlan_ioctl_args" as a char device1[24]; with 24 being a very purely magic number with no apparent rhyme or reason. In any case, if vdname is > 24 chars, then there is a whole load of problems. How do I say awful design of vlan_ioctl_args politely? /-| Anyway this will need to be handled somehow - perhaps a : /* vlan_ioctl_args field has 'device1[24]' so we need to be * sure we don't over flow */ if (strlen(vdname) > 24) { virReportError(VIR_ERR_INTERNAL_ERROR, _("generated vdname='%s' too large"), vdname); VIR_FREE(vdname); return -1; } BTW: Similarly for ADD if @ifname > 24 chars, that strcpy is going to be bad.
+ } else { + virReportSystemError(ENOSYS, + _("unsupport cmd: %d"), cmd);
virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported command option: %d"), cmd);
+ return -1; + } + + if ((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("unable to open control socket"));
s/to open/to open VLAN/
+ return -1; + } + + if (ioctl(fd, SIOCSIFVLAN, &if_request) < 0) { + virReportSystemError(errno, "%s", + _("control VLAN device error")); + VIR_FORCE_CLOSE(fd); + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + +/** + * virNetDevLoad8021Q: + * + * Load 8021q module (since kernel v2.6) + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevLoad8021Q(void) +{ + if (validate8021Q() < 0) {
if (!virFileExists(PROC_NET_VLAN_CONFIG)) { or if (virFileExists(PROC_NET_VLAN_CONFIG)) return 0; for one less level of indents.
+ char *errbuf = NULL;
Looks like you can now use: VIR_AUTOFREE(char *) errbuf = NULL;
+ if ((errbuf = virKModLoad(MODULE_8021Q, false))) { + VIR_FREE(errbuf); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to load 8021q module")); + return -1; + } + if (validate8021Q() < 0) {
if (!virFileExists(PROC_NET_VLAN_CONFIG)) { Hopefully there's no "strange delay" in generating that file that would require some sort synchronization effort here...
+ virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot load 8021q module")); + return -1; + } + } + return 0; +} + +/** + * virNetDevCreateVLanDev: + * @ifname: name of interface we will create vlan-device on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Create vlan-device which based on 8021q module. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size)
One line per arg ... char **vdname remove vdname_size
+{ + if (controlVlanDev(ADD_VLAN_CMD, ifname, vlanid) < 0) + return -1; + return getVlanDevName(ifname, vlanid, vdname, vdname_size);
Note my suggested new usage... virNetDevDestroyVLanDev(ifname, vlanid, vdname) However, if this fails, then we probably should delete the VLAN; however, we'd need to have a vdname which if we aren't going to put together here then there's little chance that the control delete code would be able to either. It's a conundrum.
+} + +/** + * virNetDevDestroyVLanDev: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * + * Destroy vlan-device whick has created by virNetDevCreateVLanDev. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) +{ + if (controlVlanDev(DEL_VLAN_CMD, ifname, vlanid) < 0)
return controlVlanDev(DEL_VLAN_CMD, ifname, vlanid);
+ return -1; + return 0; +} + +/** + * virNetDevGetVLanDevName: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Get the name of the vlan-device + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) +{ + return getVlanDevName(ifname, vlanid, vdname, vdname_size); +}
This should move to where getVlanDevName is and then getVlanDevName can be removed having this do the magic as noted before.
+ +#else /* !HAVE_STRUCT_IFREQ */ + +int +virNetDevLoad8021Q(void) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to load 8021q module on this platform")); + return -1; +} + +int +virNetDevCreateVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char *vdname[] ATTRIBUTE_UNUSED,
This doesn't match .h prototype
+ size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to create vlan-dev on this platform")); + return -1; +} + +int +virNetDevDestroyVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform")); + return -1; +} + +int +virNetDevGetVLanDevName(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char vdname[] ATTRIBUTE_UNUSED, + size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform"));
Unable to get vlan-dev name on this platform
+ return -1; +}
Whatever order/changes are made for the other side of the #if, will need to be made in this #else before posting again.
+ +#endif /* HAVE_STRUCT_IFREQ */ + + /** * virNetDevValidateConfig: * @ifname: Name of the interface diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 71eaf45..40fb6ee 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -206,6 +206,20 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) int virNetDevGetVLanID(const char *ifname, int *vlanid) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virNetDevLoad8021Q(void) + ATTRIBUTE_RETURN_CHECK; + +int virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + +int virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +int virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
Rather than ATTRIBUTE_NONNULL, let's just add checks in the code for ifname == NULL, then virReportError. As for vdname, those should be char **vdname anyway and the vdname_size is irrelevant. FWIW: The ATTRIBUTE_NONNULL's only work if NULL is passed as an argument. If the argument being passed is NULL, then some bad stuff can happen. Furthermore, if someone adds a check in the .c module for something marked this way, then Coverity gets upset. John
+ int virNetDevGetMaster(const char *ifname, char **master) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
Sent: Wednesday, July 18, 2018 at 12:12 AM From: "John Ferlan" <jferlan@redhat.com> To: "Shi Lei" <shilei.massclouds@gmx.com>, libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/3] add functions: load 8021q module, create/destroy vlan-dev
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- configure.ac | 5 +- src/libvirt_private.syms | 4 + src/util/virnetdev.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 14 ++++ 4 files changed, 216 insertions(+), 2 deletions(-)
Not exactly in my wheelhouse of knowledge and I suspect when Laine gets back from vacation he'd want to look as well, but ... let's see what feedback I can provide. I'll probably make a mess, but hopefully its understandable.
First off kudos for actually providing patches first time out that compile and pass make check syntax-check!
Thanks for your reviewing, John! I have thought that my patches have been ignored ...
diff --git a/configure.ac b/configure.ac index 59d2d09..141d5b2 100644 --- a/configure.ac +++ b/configure.ac @@ -769,8 +769,9 @@ then fi AM_CONDITIONAL([WITH_NODE_DEVICES], [test "$with_nodedev" = "yes"])
-dnl GET_VLAN_VID_CMD is required for virNetDevGetVLanID
Losing this comment for a more generic one probably isn't good.
OK
-AC_CHECK_DECLS([GET_VLAN_VID_CMD], [], [], [[#include <linux/if_vlan.h>]]) +dnl GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD is required
Rather than combining into one and losing the virNetDevGetVLanID why not add separate comments "ADD_VLAN_CMD is required for virNetDevCreateVLanDev" and "DEL_VLAN_CMD is required for virNetDevDestroyVLanDev".
OK. I'll fix it.
+AC_CHECK_DECLS([GET_VLAN_VID_CMD, ADD_VLAN_CMD, DEL_VLAN_CMD], + [], [], [[#include <linux/if_vlan.h>]])
# Check for Linux vs. BSD ifreq members AC_CHECK_MEMBERS([struct ifreq.ifr_newname, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e30490..a61ba02 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2266,7 +2266,9 @@ virModuleLoad;
# util/virnetdev.h virNetDevAddMulti; +virNetDevCreateVLanDev; virNetDevDelMulti; +virNetDevDestroyVLanDev; virNetDevExists; virNetDevFeatureTypeFromString; virNetDevFeatureTypeToString; @@ -2287,10 +2289,12 @@ virNetDevGetRxFilter; virNetDevGetVirtualFunctionIndex; virNetDevGetVirtualFunctionInfo; virNetDevGetVirtualFunctions; +virNetDevGetVLanDevName; virNetDevGetVLanID; virNetDevIfStateTypeFromString; virNetDevIfStateTypeToString; virNetDevIsVirtualFunction; +virNetDevLoad8021Q; virNetDevPFGetVF; virNetDevReadNetConfig; virNetDevRunEthernetScript; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c20022f..50a81d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -44,6 +44,7 @@ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_UNIX +# include "virkmod.h" #elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL) # define VIR_NETDEV_FAMILY AF_LOCAL #else @@ -1048,6 +1049,200 @@ int virNetDevGetVLanID(const char *ifname ATTRIBUTE_UNUSED, #endif /* ! SIOCGIFVLAN */
+#if defined(HAVE_STRUCT_IFREQ) + +# define MODULE_8021Q "8021q" +# define PROC_NET_VLAN_CONFIG "/proc/net/vlan/config" + +static int +validate8021Q(void)
I think this could be replaced in the callers with a:
virFileExists(PROC_NET_VLAN_CONFIG)
since all you seem to care is that the file got created. The open here is not doing much other than proving that this process could open the file. There's nothing be read AFAICT.
Yes. It's better to use virFileExists here. I didn't know it before.
+{ + int fd; + if ((fd = open(PROC_NET_VLAN_CONFIG, O_RDONLY)) < 0) + return -1; + VIR_FORCE_CLOSE(fd); + return 0; +} +
Adding new functions should keep 2 empty lines between each.
OK.
+static int +getVlanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size)
One line per argument please.
OK.
Why is it 'char vdname[]' and not 'char **'?
OK. I'll change this. Generally I prefer to use stack variable if I know the limit of its size.
+{
I think a :
if (!ifname) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Interface name not provided")); return -1; }
should be added.
I'll add it.
+ if (!vdname || vdname_size < IFNAMSIZ) + return -1;
So if this fails eventually a caller is going to want to know why. Adding a virReportError here about invalid arguments would be OK, but perhaps unnecessary. The @vdname is an output that if not provided is just wrong coding practices.
I don't believe vdname_size is relevant can be removed.
These two lines will be removed in patch v2.
+ if (snprintf(vdname, IFNAMSIZ, "%s.%d", ifname, vlanid) >= IFNAMSIZ)
use "return virAsprintf(vdname, "%s.%d", ifname, vlandid);"
it'll magically do the allocation and return - not really caring about IFNAMSIZ "stuff". See virNetDevSysfsFile for the example.
OK. It's better.
The virNetDevGetVLanDevName should be moved here and then the subsequent callers can use it rather than this helper by name.
OK.
+ return -1; + return 0; +} + +static int +controlVlanDev(unsigned int cmd, + const char *ifname, unsigned int vlanid)
One line per argument.
OK
+{
int ret = -1;
Why need ret?
+ int fd; + struct vlan_ioctl_args if_request; + memset(&if_request, 0, sizeof(struct vlan_ioctl_args)); + if_request.cmd = cmd; +
Again, add the :
if (!ifname) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Interface name not provided")); return -1; }
OK.
+ if (cmd == ADD_VLAN_CMD) { + strcpy(if_request.device1, ifname); + if_request.u.VID = vlanid; + } else if (cmd == DEL_VLAN_CMD) { + char vdname[IFNAMSIZ];
char *vdname = NULL;
+ if (getVlanDevName(ifname, vlanid, vdname, sizeof(vdname)) < 0)
if (virNetDevGetVLanDevName(ifname, vlanid, &vdname) < 0)
+ return -1; + strcpy(if_request.device1, vdname);
Hmmm... I know I said the length of the created name doesn't matter, but this strcpy would seem to be suspicious...
let's see, I see "vlan_ioctl_args" as a char device1[24];
with 24 being a very purely magic number with no apparent rhyme or reason. In any case, if vdname is > 24 chars, then there is a whole load of problems. How do I say awful design of vlan_ioctl_args politely? /-|
Anyway this will need to be handled somehow - perhaps a :
/* vlan_ioctl_args field has 'device1[24]' so we need to be * sure we don't over flow */ if (strlen(vdname) > 24) { virReportError(VIR_ERR_INTERNAL_ERROR, _("generated vdname='%s' too large"), vdname); VIR_FREE(vdname); return -1; }
BTW: Similarly for ADD if @ifname > 24 chars, that strcpy is going to be bad.
OK. I'll add this to make sure. But I think that the length of interface name on linux can't be larger than 15 because IFNAMSIZ is 16(includes '\0'). Both vdname and ifname are interface's names.
+ } else { + virReportSystemError(ENOSYS, + _("unsupport cmd: %d"), cmd);
virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported command option: %d"), cmd);
OK
+ return -1; + } + + if ((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", + _("unable to open control socket"));
s/to open/to open VLAN/
OK
+ return -1; + } + + if (ioctl(fd, SIOCSIFVLAN, &if_request) < 0) { + virReportSystemError(errno, "%s", + _("control VLAN device error")); + VIR_FORCE_CLOSE(fd); + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + +/** + * virNetDevLoad8021Q: + * + * Load 8021q module (since kernel v2.6) + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevLoad8021Q(void) +{ + if (validate8021Q() < 0) {
if (!virFileExists(PROC_NET_VLAN_CONFIG)) {
or
if (virFileExists(PROC_NET_VLAN_CONFIG)) return 0;
for one less level of indents.
OK
+ char *errbuf = NULL;
Looks like you can now use:
VIR_AUTOFREE(char *) errbuf = NULL;
I'll try it ...
+ if ((errbuf = virKModLoad(MODULE_8021Q, false))) { + VIR_FREE(errbuf); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to load 8021q module")); + return -1; + } + if (validate8021Q() < 0) {
if (!virFileExists(PROC_NET_VLAN_CONFIG)) {
OK.
Hopefully there's no "strange delay" in generating that file that would require some sort synchronization effort here...
Maybe need extra code to do this. I'll think about it.
+ virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot load 8021q module")); + return -1; + } + } + return 0; +} + +/** + * virNetDevCreateVLanDev: + * @ifname: name of interface we will create vlan-device on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Create vlan-device which based on 8021q module. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size)
One line per arg ...
OK
char **vdname
remove vdname_size
OK
+{ + if (controlVlanDev(ADD_VLAN_CMD, ifname, vlanid) < 0) + return -1; + return getVlanDevName(ifname, vlanid, vdname, vdname_size);
Note my suggested new usage...
virNetDevDestroyVLanDev(ifname, vlanid, vdname)
However, if this fails, then we probably should delete the VLAN; however, we'd need to have a vdname which if we aren't going to put together here then there's little chance that the control delete code would be able to either. It's a conundrum.
I prepare to add this parameter vdname for virNetDevDestroyVLanDev.
+} + +/** + * virNetDevDestroyVLanDev: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * + * Destroy vlan-device whick has created by virNetDevCreateVLanDev. + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) +{ + if (controlVlanDev(DEL_VLAN_CMD, ifname, vlanid) < 0)
return controlVlanDev(DEL_VLAN_CMD, ifname, vlanid);
OK
+ return -1; + return 0; +} + +/** + * virNetDevGetVLanDevName: + * @ifname: name of interface vlan-device depends on + * @vlanid: VLAN ID + * @vdname: used to return the name of vlan-device + * (it should be pre-defined on stack and its type is char[]) + * @vdname_size: size of the vlan-device's name (do not less than IFNAMSIZ) + * + * Get the name of the vlan-device + * + * Returns 0 on success, -1 on failure + */ +int +virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) +{ + return getVlanDevName(ifname, vlanid, vdname, vdname_size); +}
This should move to where getVlanDevName is and then getVlanDevName can be removed having this do the magic as noted before.
OK.
+ +#else /* !HAVE_STRUCT_IFREQ */ + +int +virNetDevLoad8021Q(void) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to load 8021q module on this platform")); + return -1; +} + +int +virNetDevCreateVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char *vdname[] ATTRIBUTE_UNUSED,
This doesn't match .h prototype
It's my foolish mistake!
+ size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to create vlan-dev on this platform")); + return -1; +} + +int +virNetDevDestroyVLanDev(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform")); + return -1; +} + +int +virNetDevGetVLanDevName(const char *ifname ATTRIBUTE_UNUSED, + unsigned int vlanid ATTRIBUTE_UNUSED, + char vdname[] ATTRIBUTE_UNUSED, + size_t vdname_size ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to destroy vlan-dev on this platform"));
Unable to get vlan-dev name on this platform
Yes. My fault!
+ return -1; +}
Whatever order/changes are made for the other side of the #if, will need to be made in this #else before posting again.
Sorry! I'll do this next time.
+ +#endif /* HAVE_STRUCT_IFREQ */ + + /** * virNetDevValidateConfig: * @ifname: Name of the interface diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 71eaf45..40fb6ee 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -206,6 +206,20 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) int virNetDevGetVLanID(const char *ifname, int *vlanid) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virNetDevLoad8021Q(void) + ATTRIBUTE_RETURN_CHECK; + +int virNetDevCreateVLanDev(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + +int virNetDevDestroyVLanDev(const char *ifname, unsigned int vlanid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +int virNetDevGetVLanDevName(const char *ifname, unsigned int vlanid, + char vdname[], size_t vdname_size) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
Rather than ATTRIBUTE_NONNULL, let's just add checks in the code for ifname == NULL, then virReportError. As for vdname, those should be char **vdname anyway and the vdname_size is irrelevant.
FWIW: The ATTRIBUTE_NONNULL's only work if NULL is passed as an argument. If the argument being passed is NULL, then some bad stuff can happen. Furthermore, if someone adds a check in the .c module for something marked this way, then Coverity gets upset.
John
OK. I'll fix these! Shi Lei
+ int virNetDevGetMaster(const char *ifname, char **master) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
[...]
+ +static int +controlVlanDev(unsigned int cmd, + const char *ifname, unsigned int vlanid)
One line per argument.
OK
+{
int ret = -1;
Why need ret?
I was going to suggest changing the code to use a cleanup: label like many other functions, but then figured it wasn't necessary, but forgot to go back and remove this line. John
+ int fd; + struct vlan_ioctl_args if_request; + memset(&if_request, 0, sizeof(struct vlan_ioctl_args)); + if_request.cmd = cmd; +
[...]
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/network_conf.c | 12 ++++--- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 80 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 630a87f..6e1de6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -50,7 +50,7 @@ VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "open", "bridge", "private", "vepa", "passthrough", - "hostdev") + "hostdev", "vlan") VIR_ENUM_IMPL(virNetworkBridgeMACTableManager, VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LAST, @@ -1876,6 +1876,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) */ switch (def->forward.type) { case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_VLAN: break; case VIR_NETWORK_FORWARD_ROUTE: @@ -1963,9 +1964,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) (def->forward.type != VIR_NETWORK_FORWARD_NONE && def->forward.type != VIR_NETWORK_FORWARD_NAT && def->forward.type != VIR_NETWORK_FORWARD_ROUTE && - def->forward.type != VIR_NETWORK_FORWARD_OPEN)) { + def->forward.type != VIR_NETWORK_FORWARD_OPEN && + def->forward.type != VIR_NETWORK_FORWARD_VLAN)) { virReportError(VIR_ERR_XML_ERROR, - _("mtu size only allowed in open, route, nat, " + _("mtu size only allowed in open, route, nat, vlan " "and isolated mode, not in %s (network '%s')"), virNetworkForwardTypeToString(def->forward.type), def->name); @@ -2474,6 +2476,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->bridge || def->macTableManager) { virBufferAddLit(buf, "<bridge"); @@ -2481,7 +2484,8 @@ virNetworkDefFormatBuf(virBufferPtr buf, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { virBufferAsprintf(buf, " stp='%s' delay='%ld'", def->stp ? "on" : "off", def->delay); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 54c8ed1..47bb83e 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -53,6 +53,7 @@ typedef enum { VIR_NETWORK_FORWARD_VEPA, VIR_NETWORK_FORWARD_PASSTHROUGH, VIR_NETWORK_FORWARD_HOSTDEV, + VIR_NETWORK_FORWARD_VLAN, VIR_NETWORK_FORWARD_LAST, } virNetworkForwardType; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index da3c32e..314f78c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -451,6 +451,7 @@ networkUpdateState(virNetworkObjPtr obj, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: /* If bridge doesn't exist, then mark it inactive */ if (!(def->bridge && virNetDevExists(def->bridge) == 1)) virNetworkObjSetActive(obj, false); @@ -2092,7 +2093,8 @@ networkRefreshDaemonsHelper(virNetworkObjPtr obj, ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (def->forward.type == VIR_NETWORK_FORWARD_OPEN))) { + (def->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only the three L3 network types that are configured by * libvirt will have a dnsmasq or radvd daemon associated * with them. Here we send a SIGHUP to an existing @@ -2131,7 +2133,8 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj, if (virNetworkObjIsActive(obj) && ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only three of the L3 network types that are configured by * libvirt need to have iptables rules reloaded. The 4th L3 * network type, forward='open', doesn't need this because it @@ -2513,6 +2516,27 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) goto err5; + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ]; + /* ifs[0].device.dev and vlan.tag[0] have been validated in networkValidate() */ + if (virNetDevCreateVLanDev(def->forward.ifs[0].device.dev, def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName)) < 0) + goto err5; + + if (virNetDevBridgeAddPort(def->bridge, vlanDevName) < 0) { + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0])); + goto err5; + } + + if (virNetDevSetOnline(vlanDevName, true) < 0) { + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0])); + goto err5; + } + } + VIR_FREE(macTapIfName); VIR_FREE(macMapFile); @@ -2576,6 +2600,18 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, pid_t radvdPid; pid_t dnsmasqPid; + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ]; + ignore_value(virNetDevGetVLanDevName(def->forward.ifs[0].device.dev, + def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName))); + + ignore_value(virNetDevSetOnline(vlanDevName, false)); + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0])); + } + if (def->bandwidth) virNetDevBandwidthClear(def->bridge); @@ -2719,6 +2755,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: case VIR_NETWORK_FORWARD_LAST: /* by definition these will never be encountered here */ break; @@ -2817,6 +2854,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: if (networkStartNetworkVirtual(driver, obj) < 0) goto cleanup; break; @@ -2899,6 +2937,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ret = networkShutdownNetworkVirtual(driver, obj); break; @@ -3276,7 +3315,8 @@ networkValidate(virNetworkDriverStatePtr driver, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { /* if no bridge name was given in the config, find a name * unused by any other libvirt networks and assign it. @@ -3447,7 +3487,8 @@ networkValidate(virNetworkDriverStatePtr driver, * a pool, and those using an Open vSwitch bridge. */ - vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || + vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_VLAN || + def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH || (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && def->virtPortProfile && @@ -3530,6 +3571,28 @@ networkValidate(virNetworkDriverStatePtr driver, } } } + + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + if (virNetDevLoad8021Q() < 0) + return -1; + + if (def->forward.nifs != 1 || + strlen(def->forward.ifs[0].device.dev) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid dev in <forward mode='vlan' dev='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + if (def->vlan.nTags != 1 || def->vlan.tag[0] >= 4096) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported vlan config. check <vlan><tag id='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + return 0; } @@ -3757,7 +3820,8 @@ networkUpdate(virNetworkPtr net, */ if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { switch (section) { case VIR_NETWORK_SECTION_FORWARD: case VIR_NETWORK_SECTION_FORWARD_INTERFACE: @@ -4443,7 +4507,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) { + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (netdef->forward.type == VIR_NETWORK_FORWARD_VLAN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in * iface->data.network.actual @@ -4701,7 +4766,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, * mode) and openvswitch bridges. Otherwise log an error and * fail */ - if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || + if (!(netdef->forward.type == VIR_NETWORK_FORWARD_VLAN || + actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || (actualType == VIR_DOMAIN_NET_TYPE_DIRECT && virDomainNetGetActualDirectMode(iface) == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) || -- 2.7.4
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/network_conf.c | 12 ++++--- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 80 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 82 insertions(+), 11 deletions(-)
This would be missing RNG changes in docs/schemas/network.rng, documentation changes in docs/formatnetwork.html.in in order to describe things, and networkxml2* tests. Usually if you look at the last person to add a new element to something it'll give you a good hint. In this case, see commit id 25e8112d7 when "open" was added. I use gitk to chase changes, but others use straight git - whatever works for you.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 630a87f..6e1de6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -50,7 +50,7 @@ VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "open", "bridge", "private", "vepa", "passthrough", - "hostdev") + "hostdev", "vlan")
VIR_ENUM_IMPL(virNetworkBridgeMACTableManager, VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LAST, @@ -1876,6 +1876,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) */ switch (def->forward.type) { case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_VLAN: break;
case VIR_NETWORK_FORWARD_ROUTE: @@ -1963,9 +1964,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) (def->forward.type != VIR_NETWORK_FORWARD_NONE && def->forward.type != VIR_NETWORK_FORWARD_NAT && def->forward.type != VIR_NETWORK_FORWARD_ROUTE && - def->forward.type != VIR_NETWORK_FORWARD_OPEN)) { + def->forward.type != VIR_NETWORK_FORWARD_OPEN && + def->forward.type != VIR_NETWORK_FORWARD_VLAN)) { virReportError(VIR_ERR_XML_ERROR, - _("mtu size only allowed in open, route, nat, " + _("mtu size only allowed in open, route, nat, vlan " "and isolated mode, not in %s (network '%s')"), virNetworkForwardTypeToString(def->forward.type), def->name); @@ -2474,6 +2476,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->bridge || def->macTableManager) {
virBufferAddLit(buf, "<bridge"); @@ -2481,7 +2484,8 @@ virNetworkDefFormatBuf(virBufferPtr buf, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { virBufferAsprintf(buf, " stp='%s' delay='%ld'", def->stp ? "on" : "off", def->delay); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 54c8ed1..47bb83e 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -53,6 +53,7 @@ typedef enum { VIR_NETWORK_FORWARD_VEPA, VIR_NETWORK_FORWARD_PASSTHROUGH, VIR_NETWORK_FORWARD_HOSTDEV, + VIR_NETWORK_FORWARD_VLAN,
VIR_NETWORK_FORWARD_LAST, } virNetworkForwardType; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index da3c32e..314f78c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -451,6 +451,7 @@ networkUpdateState(virNetworkObjPtr obj, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: /* If bridge doesn't exist, then mark it inactive */ if (!(def->bridge && virNetDevExists(def->bridge) == 1)) virNetworkObjSetActive(obj, false); @@ -2092,7 +2093,8 @@ networkRefreshDaemonsHelper(virNetworkObjPtr obj, ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (def->forward.type == VIR_NETWORK_FORWARD_OPEN))) { + (def->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only the three L3 network types that are configured by * libvirt will have a dnsmasq or radvd daemon associated * with them. Here we send a SIGHUP to an existing @@ -2131,7 +2133,8 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj, if (virNetworkObjIsActive(obj) && ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only three of the L3 network types that are configured by * libvirt need to have iptables rules reloaded. The 4th L3 * network type, forward='open', doesn't need this because it @@ -2513,6 +2516,27 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) goto err5;
+ if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ];
char *vlanDevName = NULL; (needs to be at the top)
+ /* ifs[0].device.dev and vlan.tag[0] have been validated in networkValidate() */ + if (virNetDevCreateVLanDev(def->forward.ifs[0].device.dev, def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName)) < 0)
Make sure the vlanDevName aligns under the def->forward...
+ goto err5;
err5 is for bandwidth failure, maybe there needs to be a err6 for VLAN and within that there will need to be a VIR_FREE(vlanDevName);
+ + if (virNetDevBridgeAddPort(def->bridge, vlanDevName) < 0) { + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0])); + goto err5; + } + + if (virNetDevSetOnline(vlanDevName, true) < 0) { + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0]));
BTW: If all callers to virNetDevDestroyVLanDev are just going to be wrapped in ignore_value(), then let's just make it a void and be done with it.
+ goto err5; + }
VIR_FREE(vlanDevName);
+ } + VIR_FREE(macTapIfName); VIR_FREE(macMapFile);
@@ -2576,6 +2600,18 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, pid_t radvdPid; pid_t dnsmasqPid;
+ if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ];
char *vlanDevName = NULL;
+ ignore_value(virNetDevGetVLanDevName(def->forward.ifs[0].device.dev, + def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName)));
don't think this can be ignored since it's used as input for other functions that follow that may not expect a NULL vlanDevName
+ + ignore_value(virNetDevSetOnline(vlanDevName, false)); + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev,
I'm now having more thoughts related to whether virNetDevDestroyVLanDev should take an ifname. Above you do the virNetDevGetVLanDevName call and then again when calling virNetDevDestroyVLanDev, so why not just have it take the vdname instead.
+ def->vlan.tag[0]));
VIR_FREE(vlanDevName); John
+ } + if (def->bandwidth) virNetDevBandwidthClear(def->bridge);
@@ -2719,6 +2755,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: case VIR_NETWORK_FORWARD_LAST: /* by definition these will never be encountered here */ break; @@ -2817,6 +2854,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: if (networkStartNetworkVirtual(driver, obj) < 0) goto cleanup; break; @@ -2899,6 +2937,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ret = networkShutdownNetworkVirtual(driver, obj); break;
@@ -3276,7 +3315,8 @@ networkValidate(virNetworkDriverStatePtr driver, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
/* if no bridge name was given in the config, find a name * unused by any other libvirt networks and assign it. @@ -3447,7 +3487,8 @@ networkValidate(virNetworkDriverStatePtr driver, * a pool, and those using an Open vSwitch bridge. */
- vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || + vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_VLAN || + def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH || (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && def->virtPortProfile && @@ -3530,6 +3571,28 @@ networkValidate(virNetworkDriverStatePtr driver, } } } + + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + if (virNetDevLoad8021Q() < 0) + return -1; + + if (def->forward.nifs != 1 || + strlen(def->forward.ifs[0].device.dev) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid dev in <forward mode='vlan' dev='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + if (def->vlan.nTags != 1 || def->vlan.tag[0] >= 4096) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported vlan config. check <vlan><tag id='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + return 0; }
@@ -3757,7 +3820,8 @@ networkUpdate(virNetworkPtr net, */ if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { switch (section) { case VIR_NETWORK_SECTION_FORWARD: case VIR_NETWORK_SECTION_FORWARD_INTERFACE: @@ -4443,7 +4507,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) { + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (netdef->forward.type == VIR_NETWORK_FORWARD_VLAN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in * iface->data.network.actual @@ -4701,7 +4766,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, * mode) and openvswitch bridges. Otherwise log an error and * fail */ - if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || + if (!(netdef->forward.type == VIR_NETWORK_FORWARD_VLAN || + actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || (actualType == VIR_DOMAIN_NET_TYPE_DIRECT && virDomainNetGetActualDirectMode(iface) == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) ||
Sent: Wednesday, July 18, 2018 at 12:38 AM From: "John Ferlan" <jferlan@redhat.com> To: "Shi Lei" <shilei.massclouds@gmx.com>, libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/3] support new forward mode of vlan for virtual network
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/network_conf.c | 12 ++++--- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 80 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 82 insertions(+), 11 deletions(-)
This would be missing RNG changes in docs/schemas/network.rng, documentation changes in docs/formatnetwork.html.in in order to describe things, and networkxml2* tests. Usually if you look at the last person to add a new element to something it'll give you a good hint. In this case, see commit id 25e8112d7 when "open" was added. I use gitk to chase changes, but others use straight git - whatever works for you.
OK. Thank you! I will add these in patch v2.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 630a87f..6e1de6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -50,7 +50,7 @@ VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "open", "bridge", "private", "vepa", "passthrough", - "hostdev") + "hostdev", "vlan")
VIR_ENUM_IMPL(virNetworkBridgeMACTableManager, VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LAST, @@ -1876,6 +1876,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) */ switch (def->forward.type) { case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_VLAN: break;
case VIR_NETWORK_FORWARD_ROUTE: @@ -1963,9 +1964,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) (def->forward.type != VIR_NETWORK_FORWARD_NONE && def->forward.type != VIR_NETWORK_FORWARD_NAT && def->forward.type != VIR_NETWORK_FORWARD_ROUTE && - def->forward.type != VIR_NETWORK_FORWARD_OPEN)) { + def->forward.type != VIR_NETWORK_FORWARD_OPEN && + def->forward.type != VIR_NETWORK_FORWARD_VLAN)) { virReportError(VIR_ERR_XML_ERROR, - _("mtu size only allowed in open, route, nat, " + _("mtu size only allowed in open, route, nat, vlan " "and isolated mode, not in %s (network '%s')"), virNetworkForwardTypeToString(def->forward.type), def->name); @@ -2474,6 +2476,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->bridge || def->macTableManager) {
virBufferAddLit(buf, "<bridge"); @@ -2481,7 +2484,8 @@ virNetworkDefFormatBuf(virBufferPtr buf, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { virBufferAsprintf(buf, " stp='%s' delay='%ld'", def->stp ? "on" : "off", def->delay); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 54c8ed1..47bb83e 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -53,6 +53,7 @@ typedef enum { VIR_NETWORK_FORWARD_VEPA, VIR_NETWORK_FORWARD_PASSTHROUGH, VIR_NETWORK_FORWARD_HOSTDEV, + VIR_NETWORK_FORWARD_VLAN,
VIR_NETWORK_FORWARD_LAST, } virNetworkForwardType; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index da3c32e..314f78c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -451,6 +451,7 @@ networkUpdateState(virNetworkObjPtr obj, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: /* If bridge doesn't exist, then mark it inactive */ if (!(def->bridge && virNetDevExists(def->bridge) == 1)) virNetworkObjSetActive(obj, false); @@ -2092,7 +2093,8 @@ networkRefreshDaemonsHelper(virNetworkObjPtr obj, ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (def->forward.type == VIR_NETWORK_FORWARD_OPEN))) { + (def->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only the three L3 network types that are configured by * libvirt will have a dnsmasq or radvd daemon associated * with them. Here we send a SIGHUP to an existing @@ -2131,7 +2133,8 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj, if (virNetworkObjIsActive(obj) && ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) { /* Only three of the L3 network types that are configured by * libvirt need to have iptables rules reloaded. The 4th L3 * network type, forward='open', doesn't need this because it @@ -2513,6 +2516,27 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) goto err5;
+ if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ];
char *vlanDevName = NULL; (needs to be at the top)
OK.
+ /* ifs[0].device.dev and vlan.tag[0] have been validated in networkValidate() */ + if (virNetDevCreateVLanDev(def->forward.ifs[0].device.dev, def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName)) < 0)
Make sure the vlanDevName aligns under the def->forward...
OK.
+ goto err5;
err5 is for bandwidth failure, maybe there needs to be a err6 for VLAN and within that there will need to be a VIR_FREE(vlanDevName);
OK.
+ + if (virNetDevBridgeAddPort(def->bridge, vlanDevName) < 0) { + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0])); + goto err5; + } + + if (virNetDevSetOnline(vlanDevName, true) < 0) { + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev, + def->vlan.tag[0]));
BTW: If all callers to virNetDevDestroyVLanDev are just going to be wrapped in ignore_value(), then let's just make it a void and be done with it.
OK. I think about it.
+ goto err5; + }
VIR_FREE(vlanDevName);
+ } + VIR_FREE(macTapIfName); VIR_FREE(macMapFile);
@@ -2576,6 +2600,18 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, pid_t radvdPid; pid_t dnsmasqPid;
+ if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + char vlanDevName[IFNAMSIZ];
char *vlanDevName = NULL;
+ ignore_value(virNetDevGetVLanDevName(def->forward.ifs[0].device.dev, + def->vlan.tag[0], + vlanDevName, sizeof(vlanDevName)));
don't think this can be ignored since it's used as input for other functions that follow that may not expect a NULL vlanDevName
Yes. I'll fix it!
+ + ignore_value(virNetDevSetOnline(vlanDevName, false)); + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName)); + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev,
I'm now having more thoughts related to whether virNetDevDestroyVLanDev should take an ifname. Above you do the virNetDevGetVLanDevName call and then again when calling virNetDevDestroyVLanDev, so why not just have it take the vdname instead.
+ def->vlan.tag[0]));
VIR_FREE(vlanDevName);
John
Yes. It's better to take the vdname. Thanks! Shi Lei
+ } + if (def->bandwidth) virNetDevBandwidthClear(def->bridge);
@@ -2719,6 +2755,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: case VIR_NETWORK_FORWARD_LAST: /* by definition these will never be encountered here */ break; @@ -2817,6 +2854,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: if (networkStartNetworkVirtual(driver, obj) < 0) goto cleanup; break; @@ -2899,6 +2937,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ret = networkShutdownNetworkVirtual(driver, obj); break;
@@ -3276,7 +3315,8 @@ networkValidate(virNetworkDriverStatePtr driver, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || - def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + def->forward.type == VIR_NETWORK_FORWARD_OPEN || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
/* if no bridge name was given in the config, find a name * unused by any other libvirt networks and assign it. @@ -3447,7 +3487,8 @@ networkValidate(virNetworkDriverStatePtr driver, * a pool, and those using an Open vSwitch bridge. */
- vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || + vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_VLAN || + def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH || (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && def->virtPortProfile && @@ -3530,6 +3571,28 @@ networkValidate(virNetworkDriverStatePtr driver, } } } + + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) { + if (virNetDevLoad8021Q() < 0) + return -1; + + if (def->forward.nifs != 1 || + strlen(def->forward.ifs[0].device.dev) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid dev in <forward mode='vlan' dev='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + if (def->vlan.nTags != 1 || def->vlan.tag[0] >= 4096) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported vlan config. check <vlan><tag id='xxx'> " + "of network '%s' with forward mode='%s'"), + def->name, virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + return 0; }
@@ -3757,7 +3820,8 @@ networkUpdate(virNetworkPtr net, */ if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN) { switch (section) { case VIR_NETWORK_SECTION_FORWARD: case VIR_NETWORK_SECTION_FORWARD_INTERFACE: @@ -4443,7 +4507,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) || - (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) { + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) || + (netdef->forward.type == VIR_NETWORK_FORWARD_VLAN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in * iface->data.network.actual @@ -4701,7 +4766,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, * mode) and openvswitch bridges. Otherwise log an error and * fail */ - if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || + if (!(netdef->forward.type == VIR_NETWORK_FORWARD_VLAN || + actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || (actualType == VIR_DOMAIN_NET_TYPE_DIRECT && virDomainNetGetActualDirectMode(iface) == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) ||
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/domain_conf.c | 1 + src/conf/virnetworkobj.c | 1 + src/qemu/qemu_process.c | 1 + 3 files changed, 3 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4e59f6..bd8b050 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29912,6 +29912,7 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface) if ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN) || (def->forward.type == VIR_NETWORK_FORWARD_OPEN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index e00c8a7..3869021 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1012,6 +1012,7 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->forward.type == VIR_NETWORK_FORWARD_OPEN) { if (!def->mac_specified) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 40d35cb..0e3e1af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4434,6 +4434,7 @@ qemuProcessGetNetworkAddress(const char *netname, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ipdef = virNetworkDefGetIPByIndex(netdef, AF_UNSPEC, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.7.4
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/domain_conf.c | 1 + src/conf/virnetworkobj.c | 1 + src/qemu/qemu_process.c | 1 + 3 files changed, 3 insertions(+)
This should have been merged into the previous patch; however, this doing point out to me that perhaps rather than "if" type conditions, maybe it'd be better if there was a typed switch() with case:'s that would then be noted during compilation when a new VIR_NETWORK_FORWARD_* type was added.... That could be a patch added *prior to* patch 2. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4e59f6..bd8b050 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29912,6 +29912,7 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface) if ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN) || (def->forward.type == VIR_NETWORK_FORWARD_OPEN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index e00c8a7..3869021 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1012,6 +1012,7 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
if (!def->mac_specified) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 40d35cb..0e3e1af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4434,6 +4434,7 @@ qemuProcessGetNetworkAddress(const char *netname, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ipdef = virNetworkDefGetIPByIndex(netdef, AF_UNSPEC, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR,
Sent: Wednesday, July 18, 2018 at 12:38 AM From: "John Ferlan" <jferlan@redhat.com> To: "Shi Lei" <shilei.massclouds@gmx.com>, libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 3/3] fix other functions to add VIR_NETWORK_FORWARD_VLAN
On 07/05/2018 11:36 PM, Shi Lei wrote:
Signed-off-by: Shi Lei <shilei.massclouds@gmx.com> --- src/conf/domain_conf.c | 1 + src/conf/virnetworkobj.c | 1 + src/qemu/qemu_process.c | 1 + 3 files changed, 3 insertions(+)
This should have been merged into the previous patch; however, this doing point out to me that perhaps rather than "if" type conditions, maybe it'd be better if there was a typed switch() with case:'s that would then be noted during compilation when a new VIR_NETWORK_FORWARD_* type was added.... That could be a patch added *prior to* patch 2.
John
OK and maybe I can try to make the patch (from 'if' to 'switch'). Thanks again! Shi Lei
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4e59f6..bd8b050 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29912,6 +29912,7 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface) if ((def->forward.type == VIR_NETWORK_FORWARD_NONE) || (def->forward.type == VIR_NETWORK_FORWARD_NAT) || (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (def->forward.type == VIR_NETWORK_FORWARD_VLAN) || (def->forward.type == VIR_NETWORK_FORWARD_OPEN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index e00c8a7..3869021 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1012,6 +1012,7 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, if (def->forward.type == VIR_NETWORK_FORWARD_NONE || def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_VLAN || def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
if (!def->mac_specified) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 40d35cb..0e3e1af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4434,6 +4434,7 @@ qemuProcessGetNetworkAddress(const char *netname, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: + case VIR_NETWORK_FORWARD_VLAN: ipdef = virNetworkDefGetIPByIndex(netdef, AF_UNSPEC, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR,
participants (2)
-
John Ferlan -
Shi Lei