2017-02-28 15:40+0800, Peter Xu:
> On Tue, Feb 28, 2017 at 03:34:24PM +0800, Peter Xu wrote:
>
> [...]
>
>> > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
>> > index ee4af898bcf6..552ae2b5e911 100644
>> > --- a/arch/mips/kvm/emulate.c
>> > +++ b/arch/mips/kvm/emulate.c
>> > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu)
>> > * check if any I/O interrupts are pending.
>> > */
>> > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) {
>> > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
>> > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu);
>>
>> Shall we just remove above line since we cleared it already?
>
> Please ignore this since I see patch 4. :-)
>
> It'll be nice if patch 4 will be before this one, but it's trivial.
I put [4/5] there to demonstrate that this error would have been less
likely with the new naming. I didn't expect that reviewers would go
through the coccinelle transformation. :)
On Wed, Mar 01, 2017 at 06:02:49PM +0100, Radim Krčmář wrote:
> 2017-02-28 15:40+0800, Peter Xu:
> > On Tue, Feb 28, 2017 at 03:34:24PM +0800, Peter Xu wrote:
> >
> > [...]
> >
> >> > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
> >> > index ee4af898bcf6..552ae2b5e911 100644
> >> > --- a/arch/mips/kvm/emulate.c
> >> > +++ b/arch/mips/kvm/emulate.c
> >> > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu)
> >> > * check if any I/O interrupts are pending.
> >> > */
> >> > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) {
> >> > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
> >> > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu);
> >>
> >> Shall we just remove above line since we cleared it already?
> >
> > Please ignore this since I see patch 4. :-)
> >
> > It'll be nice if patch 4 will be before this one, but it's trivial.
>
> I put [4/5] there to demonstrate that this error would have been less
> likely with the new naming. I didn't expect that reviewers would go
> through the coccinelle transformation. :)
Yeah, I noticed it mostly because it's the first one touched.
Meanwhile, I think it's still worthwhile to go through the patch even
it's from cocinelle since sometimes coccinelle might do something that
we (or only me?) didn't expect. E.g., afaik it cannot handle well with
over-80-chars lines, so we need to wrap them on our own (I got a patch
from the author though to fix this, but not yet tested). And also
since patches are going to be merged changes, it just feel unsafe if
we merge something without reading it. :)
Thanks,
-- peterx
On Thu, Mar 02, 2017 at 09:15:33AM +0800, Peter Xu wrote:
[...]
> Meanwhile, I think it's still worthwhile to go through the patch even
> it's from cocinelle since sometimes coccinelle might do something that
> we (or only me?) didn't expect. E.g., afaik it cannot handle well with
> over-80-chars lines, so we need to wrap them on our own (I got a patch
> from the author though to fix this, but not yet tested).
Sorry I should definitely mention that that issue only exists in
special conditions, iirc like printf("string1" "string2") and when the
strings are very long, since I believe in most cases coccinelle works
perfectly well. Thanks,
-- peterx