| Message ID | 20171116022832.24894-1-maozy.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | colo-compare: fix the dangerous assignment | expand |
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 > > > >
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 >> >> >> >> > > >
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
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
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.
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; }
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(-)