* [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak
@ 2016-10-11 7:33 Zhang Chen
2016-10-11 14:32 ` Eric Blake
0 siblings, 1 reply; 3+ messages in thread
From: Zhang Chen @ 2016-10-11 7:33 UTC (permalink / raw)
To: qemu devel, Jason Wang, Zhang Chen, Eric Blake; +Cc: Li Zhijian, Paolo Bonzini
Fix memory leak in colo-compare.c and filter-rewriter.c
Report by Coverity.
v2:
- use traces instead of fprintf in colo-compare.c
v1:
- initial patch
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
net/colo-compare.c | 11 +++--------
net/filter-rewriter.c | 17 +++++------------
trace-events | 1 +
3 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 22b1da1..bbb6ca6 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -188,7 +188,6 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
{
struct tcphdr *ptcp, *stcp;
int res;
- char *sdebug, *ddebug;
trace_colo_compare_main("compare tcp");
if (ppkt->size != spkt->size) {
@@ -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__,
+ 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),
@@ -235,8 +232,6 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
fprintf(stderr, "Secondary len = %d\n", spkt->size);
qemu_hexdump((char *)spkt->data, stderr, "colo-compare", spkt->size);
- g_free(sdebug);
- g_free(ddebug);
}
return res;
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 89abe72..c4ab91c 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -68,15 +68,11 @@ static int handle_primary_tcp_pkt(NetFilterState *nf,
tcp_pkt = (struct tcphdr *)pkt->transport_header;
if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
- char *sdebug, *ddebug;
- sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
- ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
- trace_colo_filter_rewriter_pkt_info(__func__, sdebug, ddebug,
+ trace_colo_filter_rewriter_pkt_info(__func__,
+ inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
tcp_pkt->th_flags);
trace_colo_filter_rewriter_conn_offset(conn->offset);
- g_free(sdebug);
- g_free(ddebug);
}
if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_SYN)) {
@@ -116,15 +112,11 @@ static int handle_secondary_tcp_pkt(NetFilterState *nf,
tcp_pkt = (struct tcphdr *)pkt->transport_header;
if (trace_event_get_state(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
- char *sdebug, *ddebug;
- sdebug = strdup(inet_ntoa(pkt->ip->ip_src));
- ddebug = strdup(inet_ntoa(pkt->ip->ip_dst));
- trace_colo_filter_rewriter_pkt_info(__func__, sdebug, ddebug,
+ trace_colo_filter_rewriter_pkt_info(__func__,
+ inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
tcp_pkt->th_flags);
trace_colo_filter_rewriter_conn_offset(conn->offset);
- g_free(sdebug);
- g_free(ddebug);
}
if (((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
@@ -162,6 +154,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
iov_to_buf(iov, iovcnt, 0, buf, size);
pkt = packet_new(buf, size);
+ g_free(buf);
/*
* if we get tcp packet
diff --git a/trace-events b/trace-events
index 1cb9d37..8115430 100644
--- a/trace-events
+++ 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"
# net/filter-rewriter.c
colo_filter_rewriter_debug(void) ""
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak
2016-10-11 7:33 [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak Zhang Chen
@ 2016-10-11 14:32 ` Eric Blake
2016-10-12 2:35 ` Zhang Chen
0 siblings, 1 reply; 3+ messages in thread
From: Eric Blake @ 2016-10-11 14:32 UTC (permalink / raw)
To: Zhang Chen, qemu devel, Jason Wang; +Cc: Li Zhijian, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2909 bytes --]
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.fnst@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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak
2016-10-11 14:32 ` Eric Blake
@ 2016-10-12 2:35 ` Zhang Chen
0 siblings, 0 replies; 3+ messages in thread
From: Zhang Chen @ 2016-10-12 2:35 UTC (permalink / raw)
To: Eric Blake, qemu devel, Jason Wang; +Cc: Li Zhijian, Paolo Bonzini
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 <zhangchen.fnst@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.
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-12 2:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-11 7:33 [Qemu-devel] [PATCH V2] colo-proxy: fix memory leak Zhang Chen
2016-10-11 14:32 ` Eric Blake
2016-10-12 2:35 ` Zhang Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.