On 10/29/2014 05:44 PM, Eric Blake wrote:
On 10/29/2014 03:28 PM, John Ferlan wrote:
Coverity discovered a few issues with recent changes in this module from commit id 'cc0e8c24'. This patches fixes those.
Might be nice to summarize what those errors were.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 127bfb2..8bd1af4 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -59,15 +59,20 @@ VIR_LOG_INIT("util.netdev"); #define PROC_NET_DEV_MCAST "/proc/net/dev_mcast" #define MAX_MCAST_SIZE 50*14336 #define VIR_MCAST_NAME_LEN (IFNAMSIZ + 1) -#define VIR_MCAST_INDEX_TOKEN_IDX 0 -#define VIR_MCAST_NAME_TOKEN_IDX 1 -#define VIR_MCAST_USERS_TOKEN_IDX 2 -#define VIR_MCAST_GLOBAL_TOKEN_IDX 3 -#define VIR_MCAST_ADDR_TOKEN_IDX 4 #define VIR_MCAST_NUM_TOKENS 5
Is VIR_MCAST_NUM_TOKENS still used after this patch? Or can you delete it in favor of...
yeah - forgot to go back and remove that line.
#define VIR_MCAST_TOKEN_DELIMS " \n" #define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
+typedef enum { + VIR_MCAST_TYPE_INDEX_TOKEN, + VIR_MCAST_TYPE_NAME_TOKEN, + VIR_MCAST_TYPE_USERS_TOKEN, + VIR_MCAST_TYPE_GLOBAL_TOKEN, + VIR_MCAST_TYPE_ADDR_TOKEN, + + VIR_MCAST_TYPE_LAST
...this?
- switch (ifindex) { - case VIR_MCAST_INDEX_TOKEN_IDX: + switch ((virMCastType)ifindex) { + case VIR_MCAST_TYPE_INDEX_TOKEN: if (virStrToLong_i(token, &endptr, 10, &num) < 0) { virReportSystemError(EINVAL,
@@ -2135,7 +2140,9 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) buf); } break; - default: + + /* coverity[dead_error_begin] */ + case VIR_MCAST_TYPE_LAST: break;
Okay, I can see what you did so far (convert from #defines to an enum to let the compiler warn if we miss any cases, including a way to silence Coverity about the known-dead branch of the switch).
Correct - issue #1 (of 4)
} } @@ -2194,9 +2201,7 @@ static int virNetDevGetMcastList(const char *ifname,
ret = 0; cleanup: - if (ret < 0) - virNetDevMcastListClear(mcast); - + VIR_FREE(buf);
But this one doesn't have any context in the commit message at why it is needed. However, in looking at the function itself and the referenced commit that introduced the problem, I agree with the fix. ACK, although it might be better to do this as two separate commits.
OK - I'll separate these into separate patches... These are coverity issues 2, 3, & 4. There's a followup to the Extend NIC_RX_* series that I made just before this that pointed out that there's a path where calling virNetDevMcastListClear in this context if we jumped here after failing the virFileReadAll, then there'd be a NULL deref on mcast->entries. When I looked at the code - the calling function to this will make the same Clear call on failure. The final 2 Coverity issues were resource leaks on buf, but Coverity also says "cur" is leaked resulting in a twofer. Even though you've ACK'd, I'll split things up and repost anyway for clarity. John