Re: [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak

[Zhang Chen]

On 10/11/2016 10:32 PM, Eric Blake wrote:
> On 10/11/2016 02:33 AM, Zhang Chen wrote:
> > Fix memory leak in colo-compare.c and filter-rewriter.c
> > Report by Coverity.
> This part is fine.
>
> > v2:
> >    - use traces instead of fprintf in colo-compare.c
> >
> > v1:
> >    - initial patch
> ...but this part should live...
>
> > Signed-off-by: Zhang Chen <address@hidden>
> > ---
> ...here, after the --- separator, where 'git am' will ignore it.  It is
> useful information to list readers, but will make no sense in qemu.git
> history a year from now (we don't care how many versions it took to get
> to the version that was committed).  The maintainer can fix it, so by
> itself, that's not a reason to send a v3.

OK, I will remove the version info.

> >   net/colo-compare.c    | 11 +++--------
> >   net/filter-rewriter.c | 17 +++++------------
> >   trace-events          |  1 +
> >   3 files changed, 9 insertions(+), 20 deletions(-)
> >
> > @@ -219,11 +218,9 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet 
> > *ppkt)
> >                   (spkt->size - ETH_HLEN));
> >   
> >       if (res != 0 && trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
> > -        sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
> > -        ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
> > -        fprintf(stderr, "%s: src/dst: %s/%s p: seq/ack=%u/%u"
> > -                " s: seq/ack=%u/%u res=%d flags=%x/%x\n",
> > -                __func__, sdebug, ddebug,
> > +        trace_colo_compare_pkt_info(__func__,
> net/filter-rewriter.c is the only file that currently uses
> trace_...(__func__), that's because the trace mechanism itself already
> expands the trace_* call to something with enough context that manually
> adding __func__ just gives redundant information.  I'd rather see
> __func__ removed from the trace call.

I will remove the __func__ in next version.

> > +                inet_ntoa(ppkt->ip->ip_src),
> > +                inet_ntoa(ppkt->ip->ip_dst),
> >                   (unsigned int)ntohl(ptcp->th_seq),
> >                   (unsigned int)ntohl(ptcp->th_ack),
> >                   (unsigned int)ntohl(stcp->th_seq),
> Are these casts still needed?

will fix it.

> > +++ b/trace-events
> > @@ -149,6 +149,7 @@ 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"
> >   colo_old_packet_check_found(int64_t old_time) "%" PRId64
> >   colo_compare_miscompare(void) ""
> > +colo_compare_pkt_info(const char *func, const char *src, const char *dst, uint32_t pseq, 
> > uint32_t pack, uint32_t sseq, uint32_t sack, int res, uint32_t pflag, uint32_t sflag) 
> > "%s: src/dst: %s/%s p: seq/ack=%u/%u   s: seq/ack=%u/%u res=%d flags=%x/%x\n"
> Again, the 'const char *func' portion is not needed.
>
> Looking forward to v3.

OK, I will send v3 later

Thanks
Zhang Chen


--
Thanks
zhangchen