11:10 p.m.
This started out as a fix for a crash reported in IRC and on libvir-list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
but as I examined the existing code I found so many nits to pick that
I just did them all.
The most important fix here is that virNetDevGetFeatures was creating
a struct ethtool_gfeatures object as a local on the stack and,
although the struct is defined with 0 elements in its features array,
we were telling the ethtool ioctl that we had made space for 2
elements. This led to a crash, as outlined in the email above. The fix
for this is to allocate the memory for the ethtool_gfeatures object
using VIR_ALLOC_N to a char*, giving it the length:
sizeof(ethtool_gfeatures) + (2 * sizeof(ethtool_get_features_block)
because VIR_ALLOC_N is a macro and fails when you try to typecast a
pointer to char* within the invocation, I made a union that has both a
char* and an ethtool_gfeatures*, and used the char* of the union for
VIR_ALLOC_N, and the other for everything else.
Beyond that crash fixer, the following fixups were made to the
hierarchy of functions between virNetDevGetFeatures() and
virNetDevSendEthtoolIoctl():
* macros used to examine the gfeatures bits were renamed from
FEATURE_* to GFEATURE_*
virNetDevSentEthtoolIoctl():
* no need to initialize sock to -1, since it is always set at the top
of the function.
* remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
ever encountered, this would have been *very* problematic, as it
would have led to one of those situations where virsh reports "an
error was encountered but the cause is unknown" (or whatever the
message is when we have an error but no log message).
* always call VIR_FORCE_CLOSE(sock) since we know that sock is either
a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
virNetDevFeatureAvailable()
* simplify it - no need for ret.
* follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
instead of "!bobLobLaw(lawblog)".
virNetDevGFeatureAvailable()
* eliminate this function, as it was ill-conceived (it really was only
checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
virNetDevGetFeatures()
* move all data tables (struct elem) out of the function so that they
will be statics instead of taking up space on the stack.
* remove pointless/incorrect initialization of i = -1.
* remove unnecessary static initialization of struct ethtool_value cmd
* put struct ethtool_gfeatures into a union with a char* as described above
* use libvirt convention of checking return from
virNetDevFeatureAvailable() < 0, instead of
"!virNetDevFeatureAvailable()", and immediately return to caller
with an error when virNetDevFeatureAvailable() returns an error
(previously it was ignored).
* remove superfluous size_t j, and just re-use i instead.
* runtime allocation/free of proper size object for ethtoolfeatures as
described above.
* directly call virNetDevSentEthtoolIoctl() instead of now defunct
virNetDevGFeatureAvailable().
===
NB: This patch does *not* attempt to determine the proper number of
elements for the gfeature array at runtime, as proposed in this patch:
https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
since that wasn't the cause of the crash. I'll leave it up to Moshe to
repost that patch rebased around this one (or whatever ends up being
pushed) if he thinks there is value to it.
Also - as I mentioned in earlier mail in response to the crash, I
noticed when looking into the gfeatures ethtool code that it looks to
me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
missing something. Moshe - can you either confirm or deny that? Where
did you get the value 25 from?
---
src/util/virnetdev.c | 182 ++++++++++++++++++++++++---------------------------
1 file changed, 84 insertions(+), 98 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..7f0837d 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev");
#if HAVE_DECL_ETHTOOL_GFEATURES
# define TX_UDP_TNL 25
# define GFEATURES_SIZE 2
-# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
-# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
-# define FEATURE_BIT_IS_SET(blocks, index, field) \
- (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
+# define GFEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
+# define GFEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
+# define GFEATURE_BIT_IS_SET(blocks, index, field) \
+ (GFEATURE_WORD(blocks, index, field) & GFEATURE_FIELD_FLAG(index))
#endif
typedef enum {
@@ -3032,11 +3032,10 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
{
int ret = -1;
- int sock = -1;
+ int sock;
virIfreq ifr;
- sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
- if (sock < 0) {
+ if ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) {
virReportSystemError(errno, "%s", _("Cannot open control
socket"));
goto cleanup;
}
@@ -3044,27 +3043,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
memset(&ifr, 0, sizeof(ifr));
strcpy(ifr.ifr_name, ifname);
ifr.ifr_data = cmd;
- ret = ioctl(sock, SIOCETHTOOL, &ifr);
- if (ret != 0) {
- switch (errno) {
- case EPERM:
- VIR_DEBUG("ethtool ioctl: permission denied");
- break;
- case EINVAL:
- VIR_DEBUG("ethtool ioctl: invalid request");
- break;
- case EOPNOTSUPP:
- VIR_DEBUG("ethtool ioctl: request not supported");
- break;
- default:
- virReportSystemError(errno, "%s", _("ethtool ioctl
error"));
- goto cleanup;
- }
- }
+ if ((ret = ioctl(sock, SIOCETHTOOL, &ifr)) < 0)
+ virReportSystemError(errno, "%s", _("ethtool ioctl error"));
cleanup:
- if (sock)
- VIR_FORCE_CLOSE(sock);
+ VIR_FORCE_CLOSE(sock);
return ret;
}
@@ -3081,35 +3064,50 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
static int
virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
{
- int ret = -1;
-
cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = cmd->data > 0 ? 1 : 0;
- return ret;
+ if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0)
+ return -1;
+ return cmd->data > 0 ? 1 : 0;
}
-# if HAVE_DECL_ETHTOOL_GFEATURES
-/**
- * virNetDevGFeatureAvailable
- * This function checks for the availability of a network device gfeature
- *
- * @ifname: name of the interface
- * @cmd: reference to a gfeatures ethtool command structure
- *
- * Returns 0 if not found, 1 on success, and -1 on failure.
- */
-static int
-virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
-{
- int ret = -1;
+/* static tables used by virNetDevGetFeatures() */
+struct elem {
+ const int cmd;
+ const virNetDevFeature feat;
+};
- cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
- return ret;
-}
+/* legacy ethtool getters */
+struct elem cmds[] = {
+ {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
+ {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
+ {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
+ {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
+# if HAVE_DECL_ETHTOOL_GGSO
+ {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
+# endif
+# if HAVE_DECL_ETHTOOL_GGRO
+ {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
+# endif
+};
+
+# if HAVE_DECL_ETHTOOL_GFLAGS
+/* ethtool masks */
+struct elem flags[] = {
+# if HAVE_DECL_ETH_FLAG_LRO
+ {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
+# endif
+# if HAVE_DECL_ETH_FLAG_TXVLAN
+ {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
+ {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
+# endif
+# if HAVE_DECL_ETH_FLAG_NTUBLE
+ {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
+# endif
+# if HAVE_DECL_ETH_FLAG_RXHASH
+ {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
+# endif
+};
# endif
@@ -3127,71 +3125,59 @@ int
virNetDevGetFeatures(const char *ifname,
virBitmapPtr *out)
{
- size_t i = -1;
- struct ethtool_value cmd = { 0 };
+ size_t i;
+ int ret;
+ struct ethtool_value cmd;
# if HAVE_DECL_ETHTOOL_GFEATURES
- struct ethtool_gfeatures g_cmd = { 0 };
+ union {
+ struct ethtool_gfeatures *cmd;
+ char *cmd_char;
+ } g;
# endif
- struct elem{
- const int cmd;
- const virNetDevFeature feat;
- };
- /* legacy ethtool getters */
- struct elem cmds[] = {
- {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
- {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
- {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
- {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
-# if HAVE_DECL_ETHTOOL_GGSO
- {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
-# endif
-# if HAVE_DECL_ETHTOOL_GGRO
- {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
-# endif
- };
if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST)))
return -1;
+ /* first set of capabilities are one capability per
+ * command. cmd.data is set if the interface has that
+ * capability
+ */
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) {
cmd.cmd = cmds[i].cmd;
- if (virNetDevFeatureAvailable(ifname, &cmd))
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ if (ret)
ignore_value(virBitmapSetBit(*out, cmds[i].feat));
}
# if HAVE_DECL_ETHTOOL_GFLAGS
- size_t j = -1;
- /* ethtool masks */
- struct elem flags[] = {
-# if HAVE_DECL_ETH_FLAG_LRO
- {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
-# endif
-# if HAVE_DECL_ETH_FLAG_TXVLAN
- {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
- {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
-# endif
-# if HAVE_DECL_ETH_FLAG_NTUBLE
- {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
-# endif
-# if HAVE_DECL_ETH_FLAG_RXHASH
- {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
-# endif
- };
-
+ /* second set of capabilities are all stored as 1 bit each in
+ * cmd.data of the result of the single ETHTOOL_GFLAGS command
+ */
cmd.cmd = ETHTOOL_GFLAGS;
- if (virNetDevFeatureAvailable(ifname, &cmd)) {
- for (j = 0; j < ARRAY_CARDINALITY(flags); j++) {
- if (cmd.data & flags[j].cmd)
- ignore_value(virBitmapSetBit(*out, flags[j].feat));
- }
- }
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ for (i = 0; ret && i < ARRAY_CARDINALITY(flags); i++)
+ if (cmd.data & flags[i].cmd)
+ ignore_value(virBitmapSetBit(*out, flags[i].feat));
# endif
# if HAVE_DECL_ETHTOOL_GFEATURES
- g_cmd.cmd = ETHTOOL_GFEATURES;
- g_cmd.size = GFEATURES_SIZE;
- if (virNetDevGFeatureAvailable(ifname, &g_cmd))
+ /* allocate an object with GFEATURES_SIZE elements in the features array */
+ if (VIR_ALLOC_N(g.cmd_char,
+ sizeof(struct ethtool_gfeatures) +
+ (sizeof(struct ethtool_get_features_block) * GFEATURES_SIZE)) <
0)
+ return -1;
+
+ g.cmd->cmd = ETHTOOL_GFEATURES;
+ g.cmd->size = GFEATURES_SIZE;
+ if (virNetDevSendEthtoolIoctl(ifname, g.cmd) < 0) {
+ VIR_FREE(g.cmd);
+ return -1;
+ }
+ if (GFEATURE_BIT_IS_SET(g.cmd->features, TX_UDP_TNL, active))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+ VIR_FREE(g.cmd);
# endif
if (virNetDevRDMAFeature(ifname, out) < 0)
--
2.1.0
1:47 a.m.
New subject: [libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
This started out as a fix for a crash reported in IRC and on libvir-list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
but as I examined the existing code I found so many small nits to pick
in surrounding functions that I decided to fix them all in this patch.
The most important fix here is that virNetDevGetFeatures was creating
a struct ethtool_gfeatures object as a local on the stack and,
although the struct is defined with 0 elements in its features array,
we were telling the ethtool ioctl that we had made space for 2
elements. This led to a crash, as outlined in the email above. The fix
for this is to allocate the memory for the ethtool_gfeatures object
using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type
ethtool_get_features_block
Beyond that crash fixer, the following fixups were made to the
hierarchy of functions between virNetDevGetFeatures() and
virNetDevSendEthtoolIoctl():
* macros used to examine the gfeatures bits were renamed from
FEATURE_* to GFEATURE_*
virNetDevSentEthtoolIoctl():
* no need to initialize sock to -1, since it is always set at the top
of the function.
* remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
ever encountered, this would have been *very* problematic, as it
would have led to one of those situations where virsh reports "an
error was encountered but the cause is unknown" (or whatever the
message is when we have an error but no log message).
* always call VIR_FORCE_CLOSE(sock) since we know that sock is either
a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
virNetDevFeatureAvailable()
* simplify it - no need for ret.
* follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
instead of "!bobLobLaw(lawblog)".
virNetDevGFeatureAvailable()
* eliminate this function, as it was ill-conceived (it really was only
checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
virNetDevGetFeatures()
* move all data tables (struct elem) out of the function so that they
will be statics instead of taking up space on the stack.
* remove pointless/incorrect initialization of i = -1.
* remove unnecessary static initialization of struct ethtool_value cmd
* g_cmd is now a pointer instead of automatic
* use libvirt convention of checking return from
virNetDevFeatureAvailable() < 0, instead of
"!virNetDevFeatureAvailable()", and immediately return to caller
with an error when virNetDevFeatureAvailable() returns an error
(previously it was ignored).
* remove superfluous size_t j, and just re-use i instead.
* runtime allocation/free of proper size object for ethtoolfeatures as
described above.
* directly call virNetDevSentEthtoolIoctl() instead of now defunct
virNetDevGFeatureAvailable().
---
V2: I had been looking for something like VIR_ALLOC_VAR() when writing
the patch originally, but somehow missed it, and did an ugly hack with
VIR_ALLOC_N and a union. In this version I clean that up and use
VIR_ALLOC_VAR() instead.
NB: This patch does *not* attempt to determine the proper number of
elements for the gfeature array at runtime, as proposed in this patch:
https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
since that wasn't the cause of the crash. I'll leave it up to Moshe to
repost that patch rebased around this one (or whatever ends up being
pushed) if he thinks there is value to it.
Also - as I mentioned in earlier mail in response to the crash, I
noticed when looking into the gfeatures ethtool code that it looks to
me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
missing something. Moshe - can you either confirm or deny that? Where
did you get the value 25 from?
src/util/virnetdev.c | 177 +++++++++++++++++++++++----------------------------
1 file changed, 79 insertions(+), 98 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..05fbff5 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev");
#if HAVE_DECL_ETHTOOL_GFEATURES
# define TX_UDP_TNL 25
# define GFEATURES_SIZE 2
-# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
-# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
-# define FEATURE_BIT_IS_SET(blocks, index, field) \
- (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
+# define GFEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
+# define GFEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
+# define GFEATURE_BIT_IS_SET(blocks, index, field) \
+ (GFEATURE_WORD(blocks, index, field) & GFEATURE_FIELD_FLAG(index))
#endif
typedef enum {
@@ -3032,11 +3032,10 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
{
int ret = -1;
- int sock = -1;
+ int sock;
virIfreq ifr;
- sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
- if (sock < 0) {
+ if ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) {
virReportSystemError(errno, "%s", _("Cannot open control
socket"));
goto cleanup;
}
@@ -3044,27 +3043,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
memset(&ifr, 0, sizeof(ifr));
strcpy(ifr.ifr_name, ifname);
ifr.ifr_data = cmd;
- ret = ioctl(sock, SIOCETHTOOL, &ifr);
- if (ret != 0) {
- switch (errno) {
- case EPERM:
- VIR_DEBUG("ethtool ioctl: permission denied");
- break;
- case EINVAL:
- VIR_DEBUG("ethtool ioctl: invalid request");
- break;
- case EOPNOTSUPP:
- VIR_DEBUG("ethtool ioctl: request not supported");
- break;
- default:
- virReportSystemError(errno, "%s", _("ethtool ioctl
error"));
- goto cleanup;
- }
- }
+ if ((ret = ioctl(sock, SIOCETHTOOL, &ifr)) < 0)
+ virReportSystemError(errno, "%s", _("ethtool ioctl error"));
cleanup:
- if (sock)
- VIR_FORCE_CLOSE(sock);
+ VIR_FORCE_CLOSE(sock);
return ret;
}
@@ -3081,35 +3064,50 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
static int
virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
{
- int ret = -1;
-
cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = cmd->data > 0 ? 1 : 0;
- return ret;
+ if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0)
+ return -1;
+ return cmd->data > 0 ? 1 : 0;
}
-# if HAVE_DECL_ETHTOOL_GFEATURES
-/**
- * virNetDevGFeatureAvailable
- * This function checks for the availability of a network device gfeature
- *
- * @ifname: name of the interface
- * @cmd: reference to a gfeatures ethtool command structure
- *
- * Returns 0 if not found, 1 on success, and -1 on failure.
- */
-static int
-virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
-{
- int ret = -1;
+/* static tables used by virNetDevGetFeatures() */
+struct elem {
+ const int cmd;
+ const virNetDevFeature feat;
+};
- cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
- return ret;
-}
+/* legacy ethtool getters */
+struct elem cmds[] = {
+ {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
+ {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
+ {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
+ {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
+# if HAVE_DECL_ETHTOOL_GGSO
+ {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
+# endif
+# if HAVE_DECL_ETHTOOL_GGRO
+ {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
+# endif
+};
+
+# if HAVE_DECL_ETHTOOL_GFLAGS
+/* ethtool masks */
+struct elem flags[] = {
+# if HAVE_DECL_ETH_FLAG_LRO
+ {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
+# endif
+# if HAVE_DECL_ETH_FLAG_TXVLAN
+ {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
+ {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
+# endif
+# if HAVE_DECL_ETH_FLAG_NTUBLE
+ {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
+# endif
+# if HAVE_DECL_ETH_FLAG_RXHASH
+ {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
+# endif
+};
# endif
@@ -3127,71 +3125,54 @@ int
virNetDevGetFeatures(const char *ifname,
virBitmapPtr *out)
{
- size_t i = -1;
- struct ethtool_value cmd = { 0 };
+ size_t i;
+ int ret;
+ struct ethtool_value cmd;
# if HAVE_DECL_ETHTOOL_GFEATURES
- struct ethtool_gfeatures g_cmd = { 0 };
+ struct ethtool_gfeatures *g_cmd;
# endif
- struct elem{
- const int cmd;
- const virNetDevFeature feat;
- };
- /* legacy ethtool getters */
- struct elem cmds[] = {
- {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
- {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
- {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
- {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
-# if HAVE_DECL_ETHTOOL_GGSO
- {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
-# endif
-# if HAVE_DECL_ETHTOOL_GGRO
- {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
-# endif
- };
if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST)))
return -1;
+ /* first set of capabilities are one capability per
+ * command. cmd.data is set if the interface has that
+ * capability
+ */
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) {
cmd.cmd = cmds[i].cmd;
- if (virNetDevFeatureAvailable(ifname, &cmd))
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ if (ret)
ignore_value(virBitmapSetBit(*out, cmds[i].feat));
}
# if HAVE_DECL_ETHTOOL_GFLAGS
- size_t j = -1;
- /* ethtool masks */
- struct elem flags[] = {
-# if HAVE_DECL_ETH_FLAG_LRO
- {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
-# endif
-# if HAVE_DECL_ETH_FLAG_TXVLAN
- {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
- {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
-# endif
-# if HAVE_DECL_ETH_FLAG_NTUBLE
- {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
-# endif
-# if HAVE_DECL_ETH_FLAG_RXHASH
- {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
-# endif
- };
-
+ /* second set of capabilities are all stored as 1 bit each in
+ * cmd.data of the result of the single ETHTOOL_GFLAGS command
+ */
cmd.cmd = ETHTOOL_GFLAGS;
- if (virNetDevFeatureAvailable(ifname, &cmd)) {
- for (j = 0; j < ARRAY_CARDINALITY(flags); j++) {
- if (cmd.data & flags[j].cmd)
- ignore_value(virBitmapSetBit(*out, flags[j].feat));
- }
- }
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ for (i = 0; ret && i < ARRAY_CARDINALITY(flags); i++)
+ if (cmd.data & flags[i].cmd)
+ ignore_value(virBitmapSetBit(*out, flags[i].feat));
# endif
# if HAVE_DECL_ETHTOOL_GFEATURES
- g_cmd.cmd = ETHTOOL_GFEATURES;
- g_cmd.size = GFEATURES_SIZE;
- if (virNetDevGFeatureAvailable(ifname, &g_cmd))
+ /* allocate an object with GFEATURES_SIZE elements in the features array */
+ if (VIR_ALLOC_VAR(g_cmd, struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
+ return -1;
+
+ g_cmd->cmd = ETHTOOL_GFEATURES;
+ g_cmd->size = GFEATURES_SIZE;
+ if (virNetDevSendEthtoolIoctl(ifname, g_cmd) < 0) {
+ VIR_FREE(g_cmd);
+ return -1;
+ }
+ if (GFEATURE_BIT_IS_SET(g_cmd->features, TX_UDP_TNL, active))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+ VIR_FREE(g_cmd);
# endif
if (virNetDevRDMAFeature(ifname, out) < 0)
--
2.1.0
6:41 a.m.
New subject: [libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
[reducing the cc-list]
On Tue, Aug 11, 2015 at 02:47:09AM -0400, Laine Stump wrote:
This started out as a fix for a crash reported in IRC and on
libvir-list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
but as I examined the existing code I found so many small nits to pick
in surrounding functions that I decided to fix them all in this patch.
The most important fix here is that virNetDevGetFeatures was creating
a struct ethtool_gfeatures object as a local on the stack and,
although the struct is defined with 0 elements in its features array,
we were telling the ethtool ioctl that we had made space for 2
elements. This led to a crash, as outlined in the email above. The fix
for this is to allocate the memory for the ethtool_gfeatures object
using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type
ethtool_get_features_block
Beyond that crash fixer, the following fixups were made to the
hierarchy of functions between virNetDevGetFeatures() and
virNetDevSendEthtoolIoctl():
This looks like an enumeration of things that should've been separate :)
* macros used to examine the gfeatures bits were renamed from
FEATURE_* to GFEATURE_*
virNetDevSentEthtoolIoctl():
* no need to initialize sock to -1, since it is always set at the top
of the function.
No functional changes here.
* remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
ever encountered, this would have been *very* problematic, as it
would have led to one of those situations where virsh reports "an
error was encountered but the cause is unknown" (or whatever the
message is when we have an error but no log message).
This does not seem problematic, since we do not treat -1 as a failure.
* always call VIR_FORCE_CLOSE(sock) since we know that sock is
either
a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
No fucntional change there either.
virNetDevFeatureAvailable()
* simplify it - no need for ret.
* follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
instead of "!bobLobLaw(lawblog)".
virNetDevGFeatureAvailable()
* eliminate this function, as it was ill-conceived (it really was only
checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
virNetDevGetFeatures()
* move all data tables (struct elem) out of the function so that they
will be statics instead of taking up space on the stack.
* remove pointless/incorrect initialization of i = -1.
* remove unnecessary static initialization of struct ethtool_value cmd
* g_cmd is now a pointer instead of automatic
* use libvirt convention of checking return from
virNetDevFeatureAvailable() < 0, instead of
"!virNetDevFeatureAvailable()", and immediately return to caller
with an error when virNetDevFeatureAvailable() returns an error
(previously it was ignored).
This is the change that makes the lack of error reporting problematic.
* remove superfluous size_t j, and just re-use i instead.
Another style cleanup that could be separated.
* runtime allocation/free of proper size object for ethtoolfeatures
as
described above.
* directly call virNetDevSentEthtoolIoctl() instead of now defunct
virNetDevGFeatureAvailable().
---
V2: I had been looking for something like VIR_ALLOC_VAR() when writing
the patch originally, but somehow missed it, and did an ugly hack with
VIR_ALLOC_N and a union. In this version I clean that up and use
VIR_ALLOC_VAR() instead.
NB: This patch does *not* attempt to determine the proper number of
elements for the gfeature array at runtime, as proposed in this patch:
https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
since that wasn't the cause of the crash. I'll leave it up to Moshe to
repost that patch rebased around this one (or whatever ends up being
pushed) if he thinks there is value to it.
Also - as I mentioned in earlier mail in response to the crash, I
noticed when looking into the gfeatures ethtool code that it looks to
me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
missing something. Moshe - can you either confirm or deny that? Where
did you get the value 25 from?
src/util/virnetdev.c | 177 +++++++++++++++++++++++----------------------------
1 file changed, 79 insertions(+), 98 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..05fbff5 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev");
#if HAVE_DECL_ETHTOOL_GFEATURES
# define TX_UDP_TNL 25
# define GFEATURES_SIZE 2
Do we need to (re)define these? I'd expect these constants to be present
in some header file.
Jan
10:18 a.m.
New subject: [libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
On 08/11/2015 07:41 AM, Ján Tomko wrote:
[reducing the cc-list]
On Tue, Aug 11, 2015 at 02:47:09AM -0400, Laine Stump wrote:
> This started out as a fix for a crash reported in IRC and on libvir-list:
>
> https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
>
> but as I examined the existing code I found so many small nits to pick
> in surrounding functions that I decided to fix them all in this patch.
>
> The most important fix here is that virNetDevGetFeatures was creating
> a struct ethtool_gfeatures object as a local on the stack and,
> although the struct is defined with 0 elements in its features array,
> we were telling the ethtool ioctl that we had made space for 2
> elements. This led to a crash, as outlined in the email above. The fix
> for this is to allocate the memory for the ethtool_gfeatures object
> using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type
> ethtool_get_features_block
>
> Beyond that crash fixer, the following fixups were made to the
> hierarchy of functions between virNetDevGetFeatures() and
> virNetDevSendEthtoolIoctl():
This looks like an enumeration of things that should've been separate :)
Possibly, but it would increase the bureaucratic overhead by an order of
magnitude, and many of these things are too inconsequential to bother
changing if they are in a patch by themselves.
> * macros used to examine the gfeatures bits were renamed from
> FEATURE_* to GFEATURE_*
>
> virNetDevSentEthtoolIoctl():
>
> * no need to initialize sock to -1, since it is always set at the top
> of the function.
No functional changes here.
> * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
> errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
> ever encountered, this would have been *very* problematic, as it
> would have led to one of those situations where virsh reports "an
> error was encountered but the cause is unknown" (or whatever the
> message is when we have an error but no log message).
>
This does not seem problematic, since we do not treat -1 as a failure.
Yeah, I hadn't thought of it that way, although this makes it a problem
that we are ever logging the errors. I a virReportError logging function
is called, we should be failing, and if we're not failing, then we
shouldn't be logging an error.
I'll rework the stuff to make the lower level function never log errors,
and change the high level functions back to ignoring the errors
> * always call VIR_FORCE_CLOSE(sock) since we know that sock is either
> a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
>
No fucntional change there either.
> virNetDevFeatureAvailable()
>
> * simplify it - no need for ret.
>
> * follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
> instead of "!bobLobLaw(lawblog)".
>
> virNetDevGFeatureAvailable()
>
> * eliminate this function, as it was ill-conceived (it really was only
> checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
>
> virNetDevGetFeatures()
>
> * move all data tables (struct elem) out of the function so that they
> will be statics instead of taking up space on the stack.
>
> * remove pointless/incorrect initialization of i = -1.
>
> * remove unnecessary static initialization of struct ethtool_value cmd
>
> * g_cmd is now a pointer instead of automatic
>
> * use libvirt convention of checking return from
> virNetDevFeatureAvailable() < 0, instead of
> "!virNetDevFeatureAvailable()", and immediately return to caller
> with an error when virNetDevFeatureAvailable() returns an error
> (previously it was ignored).
>
This is the change that makes the lack of error reporting problematic.
> * remove superfluous size_t j, and just re-use i instead.
>
Another style cleanup that could be separated.
But where do you draw the line? If each one of these was put in a
separate patch, there would be 8 or 10 patches here.
But anyway, I cave - I'll separate out the crash fix from all the other
"refactor" stuff and resubmit.
> * runtime allocation/free of proper size object for ethtoolfeatures as
> described above.
>
> * directly call virNetDevSentEthtoolIoctl() instead of now defunct
> virNetDevGFeatureAvailable().
>
> ---
>
> V2: I had been looking for something like VIR_ALLOC_VAR() when writing
> the patch originally, but somehow missed it, and did an ugly hack with
> VIR_ALLOC_N and a union. In this version I clean that up and use
> VIR_ALLOC_VAR() instead.
>
> NB: This patch does *not* attempt to determine the proper number of
> elements for the gfeature array at runtime, as proposed in this patch:
>
> https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
>
> since that wasn't the cause of the crash. I'll leave it up to Moshe to
> repost that patch rebased around this one (or whatever ends up being
> pushed) if he thinks there is value to it.
>
> Also - as I mentioned in earlier mail in response to the crash, I
> noticed when looking into the gfeatures ethtool code that it looks to
> me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
> missing something. Moshe - can you either confirm or deny that? Where
> did you get the value 25 from?
> src/util/virnetdev.c | 177 +++++++++++++++++++++++----------------------------
> 1 file changed, 79 insertions(+), 98 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 1e20789..05fbff5 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev");
> #if HAVE_DECL_ETHTOOL_GFEATURES
> # define TX_UDP_TNL 25
> # define GFEATURES_SIZE 2
Do we need to (re)define these? I'd expect these constants to be present
in some header file.
Yeah, that's why I've been asking about their origin in all the other
threads. I greatly dislike pulling magic constants out of thin air.
Unfortunately I've so far only seen something related-looking defined in
a kernel header (and that value is apparently wrong, according to
Moshe). Here's what I saw in the kernel:
http://lxr.free-electrons.com/source/net/core/ethtool.c#L87
[NETIF_F_GSO_UDP_TUNNEL_BIT] = /"tx-udp_tnl-segmentation"/,
then in
http://lxr.free-electrons.com/source/include/linux/netdev_features.h
NETIF_F_GSO_UDP_TUNNEL_BIT is defined in an enum, and when I compile
that enum into a program and print out the value of
NETIF_F_GSO_UDP_TUNNEL_BIT, I get "26". On the other hand, Moshe has
shown in other email where he got the value 25 from in the ethtool
commandline utility's source.
2:55 a.m.
-----Original Message-----
From: Laine Stump [mailto:laine@laine.org]
Sent: Tuesday, August 11, 2015 7:10 AM
To: libvir-list(a)redhat.com
Cc: Moshe Levi; Brian Rak; james.p.chapman(a)intel.com
Subject: [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers
This started out as a fix for a crash reported in IRC and on libvir-list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
but as I examined the existing code I found so many nits to pick that I just did
them all.
The most important fix here is that virNetDevGetFeatures was creating a
struct ethtool_gfeatures object as a local on the stack and, although the
struct is defined with 0 elements in its features array, we were telling the
ethtool ioctl that we had made space for 2 elements. This led to a crash, as
outlined in the email above. The fix for this is to allocate the memory for the
ethtool_gfeatures object using VIR_ALLOC_N to a char*, giving it the length:
I test
it on rhel7 several time with enabling and disabling the TX_UDP_TNL
sizeof(ethtool_gfeatures) + (2 * sizeof(ethtool_get_features_block)
because VIR_ALLOC_N is a macro and fails when you try to typecast a pointer
to char* within the invocation, I made a union that has both a
char* and an ethtool_gfeatures*, and used the char* of the union for
VIR_ALLOC_N, and the other for everything else.
Beyond that crash fixer, the following fixups were made to the hierarchy of
functions between virNetDevGetFeatures() and
virNetDevSendEthtoolIoctl():
* macros used to examine the gfeatures bits were renamed from
FEATURE_* to GFEATURE_*
virNetDevSentEthtoolIoctl():
* no need to initialize sock to -1, since it is always set at the top
of the function.
* remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
ever encountered, this would have been *very* problematic, as it
would have led to one of those situations where virsh reports "an
error was encountered but the cause is unknown" (or whatever the
message is when we have an error but no log message).
* always call VIR_FORCE_CLOSE(sock) since we know that sock is either
a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
virNetDevFeatureAvailable()
* simplify it - no need for ret.
* follow libvirt convention of checking for "bobLobLaw(lawblog) < 0"
instead of "!bobLobLaw(lawblog)".
virNetDevGFeatureAvailable()
* eliminate this function, as it was ill-conceived (it really was only
checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
virNetDevGetFeatures()
* move all data tables (struct elem) out of the function so that they
will be statics instead of taking up space on the stack.
* remove pointless/incorrect initialization of i = -1.
* remove unnecessary static initialization of struct ethtool_value cmd
* put struct ethtool_gfeatures into a union with a char* as described above
* use libvirt convention of checking return from
virNetDevFeatureAvailable() < 0, instead of
"!virNetDevFeatureAvailable()", and immediately return to caller
with an error when virNetDevFeatureAvailable() returns an error
(previously it was ignored).
* remove superfluous size_t j, and just re-use i instead.
* runtime allocation/free of proper size object for ethtoolfeatures as
described above.
* directly call virNetDevSentEthtoolIoctl() instead of now defunct
virNetDevGFeatureAvailable().
===
NB: This patch does *not* attempt to determine the proper number of
elements for the gfeature array at runtime, as proposed in this patch:
https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
since that wasn't the cause of the crash. I'll leave it up to Moshe to repost
that patch rebased around this one (or whatever ends up being
pushed) if he thinks there is value to it.
Also - as I mentioned in earlier mail in response to the crash, I noticed when
looking into the gfeatures ethtool code that it looks to me like TX_UDP_TNL
should actually be 26 rather than 25, but I may be missing something. Moshe
- can you either confirm or deny that? Where did you get the value 25 from?
I
answer to your question on my patch
https://www.redhat.com/archives/libvir-list/2015-August/msg00389.html
---
src/util/virnetdev.c | 182 ++++++++++++++++++++++++------------------------
---
1 file changed, 84 insertions(+), 98 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..7f0837d
100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev"); #if
HAVE_DECL_ETHTOOL_GFEATURES # define TX_UDP_TNL 25 # define
GFEATURES_SIZE 2 -# define FEATURE_WORD(blocks, index, field)
((blocks)[(index) / 32U].field)
-# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
-# define FEATURE_BIT_IS_SET(blocks, index, field) \
- (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
+# define GFEATURE_WORD(blocks, index, field) ((blocks)[(index) /
32U].field)
+# define GFEATURE_FIELD_FLAG(index) (1U << (index) % 32U)
+# define GFEATURE_BIT_IS_SET(blocks, index, field) \
+ (GFEATURE_WORD(blocks, index, field) &
GFEATURE_FIELD_FLAG(index))
#endif
typedef enum {
@@ -3032,11 +3032,10 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) {
int ret = -1;
- int sock = -1;
+ int sock;
virIfreq ifr;
- sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
- if (sock < 0) {
+ if ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) {
virReportSystemError(errno, "%s", _("Cannot open control
socket"));
goto cleanup;
}
@@ -3044,27 +3043,11 @@ virNetDevSendEthtoolIoctl(const char *ifname,
void *cmd)
memset(&ifr, 0, sizeof(ifr));
strcpy(ifr.ifr_name, ifname);
ifr.ifr_data = cmd;
- ret = ioctl(sock, SIOCETHTOOL, &ifr);
- if (ret != 0) {
- switch (errno) {
- case EPERM:
- VIR_DEBUG("ethtool ioctl: permission denied");
- break;
- case EINVAL:
- VIR_DEBUG("ethtool ioctl: invalid request");
- break;
- case EOPNOTSUPP:
- VIR_DEBUG("ethtool ioctl: request not supported");
- break;
- default:
- virReportSystemError(errno, "%s", _("ethtool ioctl
error"));
- goto cleanup;
- }
- }
+ if ((ret = ioctl(sock, SIOCETHTOOL, &ifr)) < 0)
+ virReportSystemError(errno, "%s", _("ethtool ioctl
error"));
cleanup:
- if (sock)
- VIR_FORCE_CLOSE(sock);
+ VIR_FORCE_CLOSE(sock);
return ret;
}
@@ -3081,35 +3064,50 @@ virNetDevSendEthtoolIoctl(const char *ifname,
void *cmd) static int virNetDevFeatureAvailable(const char *ifname, struct
ethtool_value *cmd) {
- int ret = -1;
-
cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = cmd->data > 0 ? 1 : 0;
- return ret;
+ if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0)
+ return -1;
+ return cmd->data > 0 ? 1 : 0;
}
-# if HAVE_DECL_ETHTOOL_GFEATURES
-/**
- * virNetDevGFeatureAvailable
- * This function checks for the availability of a network device gfeature
- *
- * @ifname: name of the interface
- * @cmd: reference to a gfeatures ethtool command structure
- *
- * Returns 0 if not found, 1 on success, and -1 on failure.
- */
-static int
-virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures
*cmd) -{
- int ret = -1;
+/* static tables used by virNetDevGetFeatures() */ struct elem {
+ const int cmd;
+ const virNetDevFeature feat;
+};
- cmd = (void*)cmd;
- if (!virNetDevSendEthtoolIoctl(ifname, cmd))
- ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
- return ret;
-}
+/* legacy ethtool getters */
+struct elem cmds[] = {
+ {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
+ {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
+ {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
+ {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, # if
HAVE_DECL_ETHTOOL_GGSO
+ {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, # endif # if
+HAVE_DECL_ETHTOOL_GGRO
+ {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, # endif };
+
+# if HAVE_DECL_ETHTOOL_GFLAGS
+/* ethtool masks */
+struct elem flags[] = {
+# if HAVE_DECL_ETH_FLAG_LRO
+ {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, # endif # if
+HAVE_DECL_ETH_FLAG_TXVLAN
+ {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
+ {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, # endif # if
+HAVE_DECL_ETH_FLAG_NTUBLE
+ {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, # endif # if
+HAVE_DECL_ETH_FLAG_RXHASH
+ {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, # endif };
# endif
@@ -3127,71 +3125,59 @@ int
virNetDevGetFeatures(const char *ifname,
virBitmapPtr *out)
{
- size_t i = -1;
- struct ethtool_value cmd = { 0 };
+ size_t i;
+ int ret;
+ struct ethtool_value cmd;
# if HAVE_DECL_ETHTOOL_GFEATURES
- struct ethtool_gfeatures g_cmd = { 0 };
+ union {
+ struct ethtool_gfeatures *cmd;
+ char *cmd_char;
+ } g;
# endif
- struct elem{
- const int cmd;
- const virNetDevFeature feat;
- };
- /* legacy ethtool getters */
- struct elem cmds[] = {
- {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM},
- {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
- {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
- {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
-# if HAVE_DECL_ETHTOOL_GGSO
- {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
-# endif
-# if HAVE_DECL_ETHTOOL_GGRO
- {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
-# endif
- };
if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST)))
return -1;
+ /* first set of capabilities are one capability per
+ * command. cmd.data is set if the interface has that
+ * capability
+ */
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) {
cmd.cmd = cmds[i].cmd;
- if (virNetDevFeatureAvailable(ifname, &cmd))
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ if (ret)
ignore_value(virBitmapSetBit(*out, cmds[i].feat));
}
# if HAVE_DECL_ETHTOOL_GFLAGS
- size_t j = -1;
- /* ethtool masks */
- struct elem flags[] = {
-# if HAVE_DECL_ETH_FLAG_LRO
- {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
-# endif
-# if HAVE_DECL_ETH_FLAG_TXVLAN
- {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
- {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
-# endif
-# if HAVE_DECL_ETH_FLAG_NTUBLE
- {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
-# endif
-# if HAVE_DECL_ETH_FLAG_RXHASH
- {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
-# endif
- };
-
+ /* second set of capabilities are all stored as 1 bit each in
+ * cmd.data of the result of the single ETHTOOL_GFLAGS command
+ */
cmd.cmd = ETHTOOL_GFLAGS;
- if (virNetDevFeatureAvailable(ifname, &cmd)) {
- for (j = 0; j < ARRAY_CARDINALITY(flags); j++) {
- if (cmd.data & flags[j].cmd)
- ignore_value(virBitmapSetBit(*out, flags[j].feat));
- }
- }
+ if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0)
+ return -1;
+ for (i = 0; ret && i < ARRAY_CARDINALITY(flags); i++)
+ if (cmd.data & flags[i].cmd)
+ ignore_value(virBitmapSetBit(*out, flags[i].feat));
# endif
# if HAVE_DECL_ETHTOOL_GFEATURES
- g_cmd.cmd = ETHTOOL_GFEATURES;
- g_cmd.size = GFEATURES_SIZE;
- if (virNetDevGFeatureAvailable(ifname, &g_cmd))
+ /* allocate an object with GFEATURES_SIZE elements in the features array
*/
+ if (VIR_ALLOC_N(g.cmd_char,
+ sizeof(struct ethtool_gfeatures) +
+ (sizeof(struct ethtool_get_features_block) * GFEATURES_SIZE)) <
0)
Ok I see so basically I missed this part in ethtool code
state = malloc(sizeof(*state) +
FEATURE_BITS_TO_BLOCKS(defs->n_features) *
sizeof(state->features.features[0]));
It wired how it work for me, I did some test on redhat7 like enabling disabling
The TX_UDP_TNL feature and calling nodedev-dumpxml to see it represented
In the nodedev xml.
Thanks for the help.
+ return -1;
+
+ g.cmd->cmd = ETHTOOL_GFEATURES;
+ g.cmd->size = GFEATURES_SIZE;
+ if (virNetDevSendEthtoolIoctl(ifname, g.cmd) < 0) {
+ VIR_FREE(g.cmd);
+ return -1;
+ }
+ if (GFEATURE_BIT_IS_SET(g.cmd->features, TX_UDP_TNL, active))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+ VIR_FREE(g.cmd);
# endif
if (virNetDevRDMAFeature(ifname, out) < 0)
--
2.1.0
1:07 p.m.
I just posted a much simplified V2 of this patch that *only* fixes the
crash, doesn't make any other cleanups:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
3357
days inactive
3357
days old
5 comments
3 participants
participants (3)
-
Ján Tomko -
Laine Stump -
Moshe Levi