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