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 <zhangchen.f...@cn.fujitsu.com>
> ---
...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.
> 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.
> + 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?
> +++ 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
