colo-compare: fix the dangerous assignment

[Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Mao Zhongyi 6 years, 5 months ago

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index ccdcba2..1ce195f 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
                          "drop packet");
         }
     }
-    con = &conn;
+    *con = conn;
 
     return 0;
 }
-- 
2.9.4

Re: [Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Darren Kenny 6 years, 5 months ago

On Thu, Nov 16, 2017 at 10:28:32AM +0800, Mao Zhongyi wrote:
>Cc: Peter Maydell <peter.maydell@linaro.org>
>Cc: Jason Wang <jasowang@redhat.com>
>Cc: Zhang Chen <zhangckid@gmail.com>
>Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
>Reported-by: Peter Maydell <peter.maydell@linaro.org>
>Reported-by: Paolo Bonzini <pbonzini@redhat.com>
>Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Code-wise, this looks like a valid fix to the existing code.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

But testing wise, have you confirmed that things are behaving as you
expected with the previous patch, since previously when calling
colo_compare_connection(), the value of conn would have always been
its initialized value of NULL.

Just want to be sure that fixing this doesn't end up breaking your
expected behaviour given that all your testing before would have had
a NULL value in conn.

Thanks,

Darren.

>---
> net/colo-compare.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/colo-compare.c b/net/colo-compare.c
>index ccdcba2..1ce195f 100644
>--- a/net/colo-compare.c
>+++ b/net/colo-compare.c
>@@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>                          "drop packet");
>         }
>     }
>-    con = &conn;
>+    *con = conn;
>
>     return 0;
> }
>-- 
>2.9.4
>
>
>
>

Re: [Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Mao Zhongyi 6 years, 5 months ago


On 11/16/2017 06:13 PM, Darren Kenny wrote:
> On Thu, Nov 16, 2017 at 10:28:32AM +0800, Mao Zhongyi wrote:
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Zhang Chen <zhangckid@gmail.com>
>> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>
> Code-wise, this looks like a valid fix to the existing code.
>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Hi, Darren

> But testing wise, have you confirmed that things are behaving as you
> expected with the previous patch, since previously when calling
> colo_compare_connection(), the value of conn would have always been
> its initialized value of NULL.

Well, in my test machine the code like *con = conn, but when I made
the patch on another machine I wrote the code con = &conn carelessly.

> Just want to be sure that fixing this doesn't end up breaking your
> expected behaviour given that all your testing before would have had
> a NULL value in conn.

Thanks for the kind reminder.

Thanks,
Mao

> Thanks,
>
> Darren.
>
>> ---
>> net/colo-compare.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index ccdcba2..1ce195f 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>>                          "drop packet");
>>         }
>>     }
>> -    con = &conn;
>> +    *con = conn;
>>
>>     return 0;
>> }
>> --
>> 2.9.4
>>
>>
>>
>>
>
>
>

Re: [Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Stefan Weil 6 years, 5 months ago

Am 16.11.2017 um 03:28 schrieb Mao Zhongyi:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Zhang Chen <zhangckid@gmail.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  net/colo-compare.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index ccdcba2..1ce195f 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>                           "drop packet");
>          }
>      }
> -    con = &conn;
> +    *con = conn;
>  
>      return 0;
>  }

The patch is definitely needed if the new function parameter con
should work.

It also fixes a gcc compiler warning:
net/colo-compare.c:139:67: warning: parameter ‘con’ set but not used
[-Wunused-but-set-parameter]

I‌ think using -Wextra would be really good to catch such bugs earlier.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Mao Zhongyi 6 years, 5 months ago


On 11/17/2017 04:07 AM, Stefan Weil wrote:
> Am 16.11.2017 um 03:28 schrieb Mao Zhongyi:
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Zhang Chen <zhangckid@gmail.com>
>> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  net/colo-compare.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index ccdcba2..1ce195f 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>>                           "drop packet");
>>          }
>>      }
>> -    con = &conn;
>> +    *con = conn;
>>
>>      return 0;
>>  }

Hi, Stefan

> The patch is definitely needed if the new function parameter con
> should work.
>
> It also fixes a gcc compiler warning:
> net/colo-compare.c:139:67: warning: parameter ‘con’ set but not used
> [-Wunused-but-set-parameter]
>
> I‌ think using -Wextra would be really good to catch such bugs earlier.

Thanks a bunch. I got it. :)

Mao

Re: [Qemu-devel] [PATCH] colo-compare: fix the dangerous assignment

Posted by Jason Wang 6 years, 5 months ago


On 2017年11月16日 10:28, Mao Zhongyi wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Zhang Chen <zhangckid@gmail.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 8ec14402029d783720f4312ed8a925548e1dad61
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>   net/colo-compare.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index ccdcba2..1ce195f 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -179,7 +179,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>                            "drop packet");
>           }
>       }
> -    con = &conn;
> +    *con = conn;
>   
>       return 0;
>   }

Applied, thanks.