On 27 April 2016 at 21:43, Zoltan Kiss <[email protected]> wrote:
> Move release to _odp_packet_classifier(), because caller has no way to > know if new_pkt were allocated. In that case there would be a double free > on pkt, and new_pkt would be leaked. > I am little skeptical about this classifier module freeing up the packet since the error in CoS can only happen during a wrong configuration by the application and hence it would be better if the error is percolated to the application so that he can take some intelligent action ( either reconfigure classification and send the packet again or free the packet ). Regarding the issue of freeing up the packet when a new packet is created by the classification module, we can better change the signature of _odp_packet_classifier() API to receive odp_packet_t as a pointer and update the pointer with new packet. The main reason I am against freeing up the packet inside classification module is that this function is currently called only by loopback device but in future development it could be called from other sources also and hence it is better if freeing of the packet during error is done by the caller rather than classification module. Regards, Bala > > Signed-off-by: Zoltan Kiss <[email protected]> > --- > platform/linux-generic/odp_classification.c | 21 ++++++++++++--------- > platform/linux-generic/pktio/loop.c | 1 - > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/odp_classification.c > b/platform/linux-generic/odp_classification.c > index 3a18a78..a1466fd 100644 > --- a/platform/linux-generic/odp_classification.c > +++ b/platform/linux-generic/odp_classification.c > @@ -734,22 +734,20 @@ int _odp_packet_classifier(pktio_entry_t *entry, > odp_packet_t pkt) > odp_packet_t new_pkt; > uint8_t *pkt_addr; > > - if (entry == NULL) > + if (entry == NULL) { > + odp_packet_free(pkt); > return -1; > + } > > pkt_hdr = odp_packet_hdr(pkt); > pkt_addr = odp_packet_data(pkt); > > /* Matching PMR and selecting the CoS for the packet*/ > cos = pktio_select_cos(entry, pkt_addr, pkt_hdr); > - if (cos == NULL) > - return -1; > - > - if (cos->s.pool == NULL) > - return -1; > - > - if (cos->s.queue == NULL) > + if (cos == NULL || cos->s.queue == NULL || cos->s.pool == NULL) { > + odp_packet_free(pkt); > return -1; > + } > > if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) { > new_pkt = odp_packet_copy(pkt, cos->s.pool->s.pool_hdl); > @@ -762,7 +760,12 @@ int _odp_packet_classifier(pktio_entry_t *entry, > odp_packet_t pkt) > > /* Enqueuing the Packet based on the CoS */ > queue = cos->s.queue; > - return queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0); > + if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0)) { > + odp_packet_free(new_pkt); > + return -1; > + } else { > + return 0; > + } > } > > cos_t *pktio_select_cos(pktio_entry_t *entry, const uint8_t *pkt_addr, > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index f6a8c1d..676e98b 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -76,7 +76,6 @@ static int loopback_recv(pktio_entry_t *pktio_entry, > odp_packet_t pkts[], > } else { > pktio_entry->s.stats.in_errors += > odp_packet_len(pkt); > - odp_packet_free(pkt); > } > } > nbr = j; > -- > 1.9.1 > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
