Re: [Qemu-devel] [qemu-s390x] [PATCH v3 2/9] s390x/kvm: pass values instead of pointers to kvm_s390_set_clock_*()

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 26 Jun 2018 11:54:32 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 25.06.2018 18:03, Cornelia Huck wrote:
> > On Mon, 25 Jun 2018 17:54:42 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 25.06.2018 17:50, Cornelia Huck wrote:  
> >>> On Mon, 25 Jun 2018 13:53:45 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> We are going to factor out the TOD into a separate device and use const
> >>>> pointers for device class functions where possible. We are passing right
> >>>> now ordinary pointers that should never be touched when setting the TOD.
> >>>> Let's just pass the values directly.
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>  target/s390x/cpu.c       |  4 ++--
> >>>>  target/s390x/kvm-stub.c  |  4 ++--
> >>>>  target/s390x/kvm.c       | 12 ++++++------
> >>>>  target/s390x/kvm_s390x.h |  4 ++--
> >>>>  4 files changed, 12 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >>>> index c268065887..68512e3e54 100644
> >>>> --- a/target/s390x/cpu.c
> >>>> +++ b/target/s390x/cpu.c
> >>>> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)    
> >>>
> >>> Any reason why you keep the pointers here?
> >>>     
> >>>>      int r = 0;
> >>>>  
> >>>>      if (kvm_enabled()) {
> >>>> -        r = kvm_s390_set_clock_ext(tod_high, tod_low);
> >>>> +        r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
> >>>>          if (r == -ENXIO) {
> >>>> -            return kvm_s390_set_clock(tod_high, tod_low);
> >>>> +            return kvm_s390_set_clock(*tod_high, *tod_low);    
> >>>
> >>> Especially as it would be more clean to check for !NULL before
> >>> dereferencing...    
> >>
> >> See the next patch :)
> >>
> >> (I assume that refactoring code in order to rip it out does not make sense)  
> > 
> > Add a comment in the commit message?
> > 
> > "Note that s390_set_clock() will be removed in a follow-on patch and
> > therefore its calling convention is not changed."
> >   
> 
> Sure I can do that. Thanks!
> 

Well, no need to respin if nothing else comes up, I can add that myself.