RE: [PATCH v5 1/1] colo-compare: Fix memory leak in packet_enqueue()

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <address@hidden>
> Sent: Sunday, April 26, 2020 7:25 PM
> To: Derek Su <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; Zhang, Chen
> <address@hidden>; address@hidden
> Subject: Re: [PATCH v5 1/1] colo-compare: Fix memory leak in
> packet_enqueue()
> 
> On Fri, 10 Apr 2020 10:00:56 +0800
> Derek Su <address@hidden> wrote:
> 
> > The patch is to fix the "pkt" memory leak in packet_enqueue().
> > The allocated "pkt" needs to be freed if the colo compare primary or
> > secondary queue is too big.
> >
> > Replace the error_report of full queue with a trace event.
> >
> > Signed-off-by: Derek Su <address@hidden>
> 
> Looks good now and works well in my tests.
> Reviewed-by: Lukas Straub <address@hidden>
> Tested-by: Lukas Straub <address@hidden>
> 

I will queue this patch to COLO branch, then submit to Jason.

Thanks
Zhang Chen

> Regards,
> Lukas Straub
> 
> > ---
> >  net/colo-compare.c | 23 +++++++++++++++--------
> >  net/trace-events   |  1 +
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 10c0239f9d..035e11d4d3 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -122,6 +122,10 @@ enum {
> >      SECONDARY_IN,
> >  };
> >
> > +static const char *colo_mode[] = {
> > +    [PRIMARY_IN] = "primary",
> > +    [SECONDARY_IN] = "secondary",
> > +};
> >
> >  static int compare_chr_send(CompareState *s,
> >                              const uint8_t *buf, @@ -217,6 +221,7 @@
> > static int packet_enqueue(CompareState *s, int mode, Connection **con)
> >      ConnectionKey key;
> >      Packet *pkt = NULL;
> >      Connection *conn;
> > +    int ret;
> >
> >      if (mode == PRIMARY_IN) {
> >          pkt = packet_new(s->pri_rs.buf, @@ -245,16 +250,18 @@ static
> > int packet_enqueue(CompareState *s, int mode, Connection **con)
> >      }
> >
> >      if (mode == PRIMARY_IN) {
> > -        if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) {
> > -            error_report("colo compare primary queue size too big,"
> > -                         "drop packet");
> > -        }
> > +        ret = colo_insert_packet(&conn->primary_list, pkt,
> > + &conn->pack);
> >      } else {
> > -        if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) {
> > -            error_report("colo compare secondary queue size too big,"
> > -                         "drop packet");
> > -        }
> > +        ret = colo_insert_packet(&conn->secondary_list, pkt,
> > + &conn->sack);
> >      }
> > +
> > +    if (!ret) {
> > +        trace_colo_compare_drop_packet(colo_mode[mode],
> > +            "queue size too big, drop packet");
> > +        packet_destroy(pkt, NULL);
> > +        pkt = NULL;
> > +    }
> > +
> >      *con = conn;
> >
> >      return 0;
> > diff --git a/net/trace-events b/net/trace-events index
> > 02c13fd0ba..fa49c71533 100644
> > --- a/net/trace-events
> > +++ b/net/trace-events
> > @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s"
> >
> >  # colo-compare.c
> >  colo_compare_main(const char *chr) ": %s"
> > +colo_compare_drop_packet(const char *queue, const char *chr)
> ": %s: %s"
> >  colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
> >  colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
> >  colo_compare_ip_info(int psize, const char *sta, const char *stb, int 
> > ssize,
> const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s,
> spkt size = %d, ip_src = %s, ip_dst = %s"