On 10.07.2018 14:14, Vishal Deep Ajmera wrote: >>> + >>> + /* preserve the order of packet for flow batching */ >>> + index_map[packets_->count - 1] = map_cnt; >> >> Using the "packets_->count" without accessor + inside a refill loop is highly >> inaccurate/potentially dangerous. Isn't it equal to "n_missed"? >> > Hi Ilya, > > Thanks for the review. However I could not understand your point about > inaccurate/potentially dangerous code. Can you elaborate on a scenario > where this is likely to fail/crash ?
This is potentially dangerous from the future modifications and hard to read for reviewer/person who tries to understand how it works. Current implementation will fail if someone will change the logic of 'DP_PACKET_BATCH_REFILL_FOR_EACH', for example. The key point is that 'DP_PACKET_BATCH_REFILL_FOR_EACH' and 'dp_packet_batch_refill' manipulates with the batch size in a hidden from the end user manner. Using of the 'packets_->count' directly requires knowledge of how refilling works internally, but these functions was intended to hide internals of batch manipulations. Also, as I understand, you're storing 'map_cnt' for each missed packet. So, why not just use 'n_missed' for that purpose? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
