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;