> Sent: Wednesday, July 18, 2018 at 12:12 AM
> From: "John Ferlan" <jferlan@xxxxxxxxxx>
> To: "Shi Lei" <shilei.massclouds@xxxxxxx>, libvir-list@xxxxxxxxxx
> Subject: Re: [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@xxxxxxx>
> > ---
> > 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;
> >
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
