> -----Original Message----- > From: Kevin Traynor <[email protected]> > Sent: Friday, May 15, 2020 12:18 PM > To: Wang, Yipeng1 <[email protected]>; [email protected]; Gobriel, > Sameh <[email protected]>; Richardson, Bruce > <[email protected]> > Cc: [email protected]; Yigit, Ferruh <[email protected]>; > [email protected]; [email protected] > Subject: Re: [PATCH] hash: fix gcc 10 maybe-uninitialized warning > > On 15/05/2020 20:06, Wang, Yipeng1 wrote: > >> -----Original Message----- > >> From: Kevin Traynor <[email protected]> > >> Sent: Friday, May 15, 2020 7:28 AM > >> To: [email protected]; Wang, Yipeng1 <[email protected]>; Gobriel, > >> Sameh <[email protected]>; Richardson, Bruce > >> <[email protected]> > >> Cc: [email protected]; Yigit, Ferruh > >> <[email protected]>; [email protected]; Kevin Traynor > >> <[email protected]>; [email protected] > >> Subject: [PATCH] hash: fix gcc 10 maybe-uninitialized warning > >> > >> gcc 10.1.1 reports a warning for the ext_bkt_id variable: > >> > >> ../lib/librte_hash/rte_cuckoo_hash.c: > >> In function ‘__rte_hash_add_key_with_hash’: > >> ../lib/librte_hash/rte_cuckoo_hash.c:1104:29: > >> warning: ‘ext_bkt_id’ may be used uninitialized in this function > >> [-Wmaybe- uninitialized] > >> 1104 | (h->buckets_ext[ext_bkt_id - 1]).sig_current[0] = short_sig; > >> | ~~~~~~~~~~~^~~ > >> > >> The return value of rte_ring_sc_dequeue_elem() is already checked, > >> but also initialize ext_bkt_id to zero (invalid value) and check that > >> it also overwritten. > >> > >> Fixes: fbfe568103b0 ("hash: use 32-bit elements rings to save > >> memory") > >> Cc: [email protected] > >> > >> Signed-off-by: Kevin Traynor <[email protected]> > >> --- > >> lib/librte_hash/rte_cuckoo_hash.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/lib/librte_hash/rte_cuckoo_hash.c > >> b/lib/librte_hash/rte_cuckoo_hash.c > >> index 38767a8a1..90cb99b0e 100644 > >> --- a/lib/librte_hash/rte_cuckoo_hash.c > >> +++ b/lib/librte_hash/rte_cuckoo_hash.c > >> @@ -940,6 +940,6 @@ __rte_hash_add_key_with_hash(const struct > >> rte_hash *h, const void *key, > >> struct rte_hash_bucket *prim_bkt, *sec_bkt, *cur_bkt; > >> struct rte_hash_key *new_k, *keys = h->key_store; > >> + uint32_t ext_bkt_id = 0; > >> uint32_t slot_id; > >> - uint32_t ext_bkt_id; > >> int ret; > >> unsigned n_slots; > >> @@ -1096,5 +1096,6 @@ __rte_hash_add_key_with_hash(const struct > >> rte_hash *h, const void *key, > >> */ > >> if (rte_ring_sc_dequeue_elem(h->free_ext_bkts, &ext_bkt_id, > >> - sizeof(uint32_t)) != 0) { > >> + sizeof(uint32_t)) != 0 || > >> + ext_bkt_id == 0) { > > [Wang, Yipeng] > > If convenient, it would be better to make the two lines aligned with same > indent... > > > > Hi Yipeng, I had checked the coding style [1] about this and I think > it's correct as 'sizeof..' is a wrap from the first condition so gets a > second tab to indicate that, whereas 'ext_bkt_id..' is the second > condition with no wrap. Fine to change it, if I interpret incorrectly. > > [1] third bullet, > http://doc.dpdk.org/guides/contributing/coding_style.html#general [Wang, Yipeng] I see your point now. Thanks for explaining and I think it makes sense to distinguish these two. I guess it just came from my own aesthetic preference. and I don’t have the best coding style : )
> > >> ret = -ENOSPC; > >> goto failure; > >> -- > >> 2.21.3 > > [Wang, Yipeng] > > Thanks for the fix. I think It is also better code in general. > > > > Acked-by: Yipeng Wang <[email protected]> > >
