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

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

[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;
 }
-- 
2.9.4

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

[Darren Kenny]

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

[Mao Zhongyi]

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

[Stefan Weil]

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

[Mao Zhongyi]

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

[Jason Wang]

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.