| Message ID | 20190718092219.20081-1-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3] net/colo-compare.c: Fix memory leak and code style issue. | expand |
On 7/18/19 11:22 AM, Zhang Chen wrote: > From: Zhang Chen <chen.zhang@intel.com> > > This patch to fix the origin "char *data" menory leak, code style issue "memory" > and add necessary check here. > Reported-by: Coverity (CID 1402785) > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > net/colo-compare.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index 909dd6c6eb..fcccb4c6f6 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > uint32_t vnet_hdr_len, > bool notify_remote_frame); > > +static bool packet_matches_str(const char *str, > + uint8_t *buf, You can use 'uint8_t *buf'. > + uint32_t packet_len) > +{ > + if (packet_len != strlen(str)) { > + return false; > + } > + > + return !memcmp(str, buf, strlen(str)); If you don't want to use a local variable to hold strlen(str), you can reuse packet_len since it is the same value: return !memcmp(str, buf, packet_len); However it makes the review harder, so I'd prefer using a str_len local var. > +} > + > static void notify_remote_frame(CompareState *s) > { > char msg[] = "DO_CHECKPOINT"; > @@ -1008,21 +1019,24 @@ static void compare_notify_rs_finalize(SocketReadState *notify_rs) > { > CompareState *s = container_of(notify_rs, CompareState, notify_rs); > > - /* Get Xen colo-frame's notify and handle the message */ > - char *data = g_memdup(notify_rs->buf, notify_rs->packet_len); > - char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > + const char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > int ret; > > - if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) { > + if (packet_matches_str("COLO_USERSPACE_PROXY_INIT", > + notify_rs->buf, > + notify_rs->packet_len)) { > ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true); > if (ret < 0) { > error_report("Notify Xen COLO-frame INIT failed"); > } > - } > - > - if (!strcmp(data, "COLO_CHECKPOINT")) { > + } else if (packet_matches_str("COLO_CHECKPOINT", > + notify_rs->buf, > + notify_rs->packet_len)) { > /* colo-compare do checkpoint, flush pri packet and remove sec packet */ > g_queue_foreach(&s->conn_list, colo_flush_packets, s); > + } else { > + error_report("COLO compare got unsupported instruction '%s'", > + (char *)notify_rs->buf); > } > } > >
On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 7/18/19 11:22 AM, Zhang Chen wrote: > > From: Zhang Chen <chen.zhang@intel.com> > > > > This patch to fix the origin "char *data" menory leak, code style issue > > "memory" > > > and add necessary check here. > > Reported-by: Coverity (CID 1402785) > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > net/colo-compare.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c > > index 909dd6c6eb..fcccb4c6f6 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > > uint32_t vnet_hdr_len, > > bool notify_remote_frame); > > > > +static bool packet_matches_str(const char *str, > > + uint8_t *buf, > > You can use 'uint8_t *buf'. ?? that seems to be the same as what is written... > > > + uint32_t packet_len) > > +{ > > + if (packet_len != strlen(str)) { > > + return false; > > + } > > + > > + return !memcmp(str, buf, strlen(str)); > > If you don't want to use a local variable to hold strlen(str), you can > reuse packet_len since it is the same value: > > return !memcmp(str, buf, packet_len); > > However it makes the review harder, so I'd prefer using a str_len local var. I'm pretty sure the compiler is going to optimise the strlen() away into a compile time constant anyway, so this is somewhat unnecessary micro-optimisation I think. thanks -- PMM
On 7/18/19 12:37 PM, Peter Maydell wrote: > On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 7/18/19 11:22 AM, Zhang Chen wrote: >>> From: Zhang Chen <chen.zhang@intel.com> >>> >>> This patch to fix the origin "char *data" menory leak, code style issue >> >> "memory" >> >>> and add necessary check here. >>> Reported-by: Coverity (CID 1402785) >>> >>> Signed-off-by: Zhang Chen <chen.zhang@intel.com> >>> --- >>> net/colo-compare.c | 28 +++++++++++++++++++++------- >>> 1 file changed, 21 insertions(+), 7 deletions(-) >>> >>> diff --git a/net/colo-compare.c b/net/colo-compare.c >>> index 909dd6c6eb..fcccb4c6f6 100644 >>> --- a/net/colo-compare.c >>> +++ b/net/colo-compare.c >>> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, >>> uint32_t vnet_hdr_len, >>> bool notify_remote_frame); >>> >>> +static bool packet_matches_str(const char *str, >>> + uint8_t *buf, >> >> You can use 'uint8_t *buf'. > > ?? that seems to be the same as what is written... Oops sorry, I copy/pasted and did not noticed I removed the 'const' word. So I meant: You can use 'const uint8_t *buf' >> >>> + uint32_t packet_len) >>> +{ >>> + if (packet_len != strlen(str)) { >>> + return false; >>> + } >>> + >>> + return !memcmp(str, buf, strlen(str)); >> >> If you don't want to use a local variable to hold strlen(str), you can >> reuse packet_len since it is the same value: >> >> return !memcmp(str, buf, packet_len); >> >> However it makes the review harder, so I'd prefer using a str_len local var. > > I'm pretty sure the compiler is going to optimise the > strlen() away into a compile time constant anyway, so > this is somewhat unnecessary micro-optimisation I think. I was not sure, I'm glad to learn that :) Thanks, Phil.
On Thu, 18 Jul 2019 at 10:27, Zhang Chen <chen.zhang@intel.com> wrote: > > From: Zhang Chen <chen.zhang@intel.com> > > This patch to fix the origin "char *data" menory leak, code style issue > and add necessary check here. > Reported-by: Coverity (CID 1402785) > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > net/colo-compare.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index 909dd6c6eb..fcccb4c6f6 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > uint32_t vnet_hdr_len, > bool notify_remote_frame); > > +static bool packet_matches_str(const char *str, > + uint8_t *buf, > + uint32_t packet_len) > +{ > + if (packet_len != strlen(str)) { Is '!=' definitely correct? (ie the incoming packet must *not* contain a trailing '\0' or any other trailing data) ? Is there a specification of the protocol somewhere? If so, that presumably should say one way or the other. > + return false; > + } > + > + return !memcmp(str, buf, strlen(str)); > +} thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Thursday, July 18, 2019 9:42 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; > qemu-dev <qemu-devel@nongnu.org>; Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V3] net/colo-compare.c: Fix memory leak and code style > issue. > > On Thu, 18 Jul 2019 at 10:27, Zhang Chen <chen.zhang@intel.com> wrote: > > > > From: Zhang Chen <chen.zhang@intel.com> > > > > This patch to fix the origin "char *data" menory leak, code style > > issue and add necessary check here. > > Reported-by: Coverity (CID 1402785) > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > net/colo-compare.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > > 909dd6c6eb..fcccb4c6f6 100644 > > --- a/net/colo-compare.c > > +++ b/net/colo-compare.c > > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > > uint32_t vnet_hdr_len, > > bool notify_remote_frame); > > > > +static bool packet_matches_str(const char *str, > > + uint8_t *buf, > > + uint32_t packet_len) { > > + if (packet_len != strlen(str)) { > > Is '!=' definitely correct? (ie the incoming packet must > *not* contain a trailing '\0' or any other trailing data) ? Yes, the packet not contain a trail. As Jason comments before, you can see the net/net.c "net_fill_rstate()". We just got the length and data. Thanks Zhang Chen > > Is there a specification of the protocol somewhere? If so, that presumably > should say one way or the other. > > > + return false; > > + } > > + > > + return !memcmp(str, buf, strlen(str)); } > > thanks > -- PMM
> -----Original Message----- > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com] > Sent: Thursday, July 18, 2019 6:54 PM > To: Peter Maydell <peter.maydell@linaro.org> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; > Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; > Zhang Chen <zhangckid@gmail.com> > Subject: Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak > and code style issue. > > On 7/18/19 12:37 PM, Peter Maydell wrote: > > On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> > wrote: > >> > >> On 7/18/19 11:22 AM, Zhang Chen wrote: > >>> From: Zhang Chen <chen.zhang@intel.com> > >>> > >>> This patch to fix the origin "char *data" menory leak, code style > >>> issue > >> > >> "memory" Oh, I will fix this typo in next version. > >> > >>> and add necessary check here. > >>> Reported-by: Coverity (CID 1402785) > >>> > >>> Signed-off-by: Zhang Chen <chen.zhang@intel.com> > >>> --- > >>> net/colo-compare.c | 28 +++++++++++++++++++++------- > >>> 1 file changed, 21 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/net/colo-compare.c b/net/colo-compare.c index > >>> 909dd6c6eb..fcccb4c6f6 100644 > >>> --- a/net/colo-compare.c > >>> +++ b/net/colo-compare.c > >>> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > >>> uint32_t vnet_hdr_len, > >>> bool notify_remote_frame); > >>> > >>> +static bool packet_matches_str(const char *str, > >>> + uint8_t *buf, > >> > >> You can use 'uint8_t *buf'. > > > > ?? that seems to be the same as what is written... > > Oops sorry, I copy/pasted and did not noticed I removed the 'const' > word. So I meant: You can use 'const uint8_t *buf' OK. Thanks Zhang Chen > > >> > >>> + uint32_t packet_len) { > >>> + if (packet_len != strlen(str)) { > >>> + return false; > >>> + } > >>> + > >>> + return !memcmp(str, buf, strlen(str)); > >> > >> If you don't want to use a local variable to hold strlen(str), you > >> can reuse packet_len since it is the same value: > >> > >> return !memcmp(str, buf, packet_len); > >> > >> However it makes the review harder, so I'd prefer using a str_len local var. > > > > I'm pretty sure the compiler is going to optimise the > > strlen() away into a compile time constant anyway, so this is somewhat > > unnecessary micro-optimisation I think. > > I was not sure, I'm glad to learn that :) > > Thanks, > > Phil.
Sorry, I re-sent the old version. Please redirect to V4 patch. Thanks Zhang Chen > -----Original Message----- > From: Zhang, Chen > Sent: Thursday, July 18, 2019 5:22 PM > To: Li Zhijian <lizhijian@cn.fujitsu.com>; Peter Maydell > <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; qemu-dev > <qemu-devel@nongnu.org> > Cc: Zhang Chen <zhangckid@gmail.com>; Zhang, Chen <chen.zhang@intel.com> > Subject: [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue. > > From: Zhang Chen <chen.zhang@intel.com> > > This patch to fix the origin "char *data" menory leak, code style issue and add > necessary check here. > Reported-by: Coverity (CID 1402785) > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > net/colo-compare.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > 909dd6c6eb..fcccb4c6f6 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, > uint32_t vnet_hdr_len, > bool notify_remote_frame); > > +static bool packet_matches_str(const char *str, > + uint8_t *buf, > + uint32_t packet_len) { > + if (packet_len != strlen(str)) { > + return false; > + } > + > + return !memcmp(str, buf, strlen(str)); } > + > static void notify_remote_frame(CompareState *s) { > char msg[] = "DO_CHECKPOINT"; > @@ -1008,21 +1019,24 @@ static void > compare_notify_rs_finalize(SocketReadState *notify_rs) { > CompareState *s = container_of(notify_rs, CompareState, notify_rs); > > - /* Get Xen colo-frame's notify and handle the message */ > - char *data = g_memdup(notify_rs->buf, notify_rs->packet_len); > - char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > + const char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > int ret; > > - if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) { > + if (packet_matches_str("COLO_USERSPACE_PROXY_INIT", > + notify_rs->buf, > + notify_rs->packet_len)) { > ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true); > if (ret < 0) { > error_report("Notify Xen COLO-frame INIT failed"); > } > - } > - > - if (!strcmp(data, "COLO_CHECKPOINT")) { > + } else if (packet_matches_str("COLO_CHECKPOINT", > + notify_rs->buf, > + notify_rs->packet_len)) { > /* colo-compare do checkpoint, flush pri packet and remove sec packet */ > g_queue_foreach(&s->conn_list, colo_flush_packets, s); > + } else { > + error_report("COLO compare got unsupported instruction '%s'", > + (char *)notify_rs->buf); > } > } > > -- > 2.17.GIT
diff --git a/net/colo-compare.c b/net/colo-compare.c index 909dd6c6eb..fcccb4c6f6 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s, uint32_t vnet_hdr_len, bool notify_remote_frame); +static bool packet_matches_str(const char *str, + uint8_t *buf, + uint32_t packet_len) +{ + if (packet_len != strlen(str)) { + return false; + } + + return !memcmp(str, buf, strlen(str)); +} + static void notify_remote_frame(CompareState *s) { char msg[] = "DO_CHECKPOINT"; @@ -1008,21 +1019,24 @@ static void compare_notify_rs_finalize(SocketReadState *notify_rs) { CompareState *s = container_of(notify_rs, CompareState, notify_rs); - /* Get Xen colo-frame's notify and handle the message */ - char *data = g_memdup(notify_rs->buf, notify_rs->packet_len); - char msg[] = "COLO_COMPARE_GET_XEN_INIT"; + const char msg[] = "COLO_COMPARE_GET_XEN_INIT"; int ret; - if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) { + if (packet_matches_str("COLO_USERSPACE_PROXY_INIT", + notify_rs->buf, + notify_rs->packet_len)) { ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true); if (ret < 0) { error_report("Notify Xen COLO-frame INIT failed"); } - } - - if (!strcmp(data, "COLO_CHECKPOINT")) { + } else if (packet_matches_str("COLO_CHECKPOINT", + notify_rs->buf, + notify_rs->packet_len)) { /* colo-compare do checkpoint, flush pri packet and remove sec packet */ g_queue_foreach(&s->conn_list, colo_flush_packets, s); + } else { + error_report("COLO compare got unsupported instruction '%s'", + (char *)notify_rs->buf); } }