Re: [libvirt] [PATCH 1/3] add functions: load 8021q module, create/destroy vlan-dev

Sent: Wednesday, July 18, 2018 at 12:12 AM From: "John Ferlan" <jferlan(a)redhat.com&gt; To: "Shi Lei" <shilei.massclouds(a)gmx.com&gt;, libvir-list(a)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(a)gmx.com&gt; > --- > 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; > >