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!
> 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;
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
