* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 9:22 [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue Zhang Chen
@ 2019-07-18 10:28 ` Philippe Mathieu-Daudé
2019-07-18 10:37 ` Peter Maydell
2019-07-18 13:42 ` Peter Maydell
2019-07-21 9:07 ` Zhang, Chen
2 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-18 10:28 UTC (permalink / raw)
To: Zhang Chen, Li Zhijian, Peter Maydell, Jason Wang, qemu-dev; +Cc: Zhang Chen
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);
> }
> }
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 10:28 ` Philippe Mathieu-Daudé
@ 2019-07-18 10:37 ` Peter Maydell
2019-07-18 10:53 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-07-18 10:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Zhang Chen, Jason Wang, qemu-dev, Li Zhijian, Zhang Chen
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 10:37 ` Peter Maydell
@ 2019-07-18 10:53 ` Philippe Mathieu-Daudé
2019-07-19 1:56 ` Zhang, Chen
0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-18 10:53 UTC (permalink / raw)
To: Peter Maydell; +Cc: Zhang Chen, Jason Wang, qemu-dev, Li Zhijian, Zhang Chen
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 10:53 ` Philippe Mathieu-Daudé
@ 2019-07-19 1:56 ` Zhang, Chen
0 siblings, 0 replies; 8+ messages in thread
From: Zhang, Chen @ 2019-07-19 1:56 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell
Cc: Jason Wang, qemu-dev, Li Zhijian, Zhang Chen
> -----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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 9:22 [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue Zhang Chen
2019-07-18 10:28 ` Philippe Mathieu-Daudé
@ 2019-07-18 13:42 ` Peter Maydell
2019-07-19 1:54 ` Zhang, Chen
2019-07-21 9:07 ` Zhang, Chen
2 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-07-18 13:42 UTC (permalink / raw)
To: Zhang Chen; +Cc: Jason Wang, qemu-dev, Li Zhijian, Zhang Chen
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 13:42 ` Peter Maydell
@ 2019-07-19 1:54 ` Zhang, Chen
0 siblings, 0 replies; 8+ messages in thread
From: Zhang, Chen @ 2019-07-19 1:54 UTC (permalink / raw)
To: Peter Maydell; +Cc: Jason Wang, qemu-dev, Li Zhijian, Zhang Chen
> -----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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
2019-07-18 9:22 [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue Zhang Chen
2019-07-18 10:28 ` Philippe Mathieu-Daudé
2019-07-18 13:42 ` Peter Maydell
@ 2019-07-21 9:07 ` Zhang, Chen
2 siblings, 0 replies; 8+ messages in thread
From: Zhang, Chen @ 2019-07-21 9:07 UTC (permalink / raw)
To: Li Zhijian, Peter Maydell, Jason Wang, qemu-dev; +Cc: Zhang Chen
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
^ permalink raw reply [flat|nested] 8+ messages in thread