Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Alex Bligh <alex@alex.org.uk>]

On 1 Jul 2014, at 02:12, Lb peace <peaceustc@gmail.com> wrote:

> 1)“ it's an automated output of perl simply changing one calling convention to another”.What do you mean... I 
> can not find the point :)

I'd thought this was the huge patch which was generated by
perl to change the API, but it's not.

> 2) The problem is :
>       a.use QEMU to run linux-0.2.img with command qemu-system-i386 linux-0.2.img or sth. like that
>       b.hwclock ,and we get a date1(2014-07-01 12:00,e.g.)
>       c.change the clock of host before date1(2014-04-01 12:00),called date2
>       d.hwclock,and we get another date3(2014-07-01,12:01)
>       e. before that patch date3 is synchronized with host,so it changed to a time   date3>date2 && date3<date1
>           after that patch  date3 is not synchronized with host.It has its own clock,so date3 >date1

OK.

Your patch changes QEMU_CLOCK_REALTIME (a clock type) to rtc_clock (an instance of a clock type)

rtc_clock is set up in vl.c as follows:

    rtc_clock = QEMU_CLOCK_HOST;
    ...
    value = qemu_opt_get(opts, "clock");
    if (value) {
        if (!strcmp(value, "host")) {
            rtc_clock = QEMU_CLOCK_HOST;
        } else if (!strcmp(value, "rt")) {
            rtc_clock = QEMU_CLOCK_REALTIME;
        } else if (!strcmp(value, "vm")) {
            rtc_clock = QEMU_CLOCK_VIRTUAL;
        } else {
            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
            exit(1);
        }
    }

So by default it uses the QEMU_CLOCK_HOST option, rather than QEMU_CLOCK_REALTIME.

> 3)why we set a reset_notifiers of QEMU_CLOCK_REALTIME? Any comment to that? A QEMU_CLOCK_REALTIME clock will 
>  not call its  reset_notifiers when the time is adjusted to a time-point before.The reset_notifiers is useless for this type of QEMUClock.

I think this is a staightforward bug. I suspect the issue is that at that point
in the patch series I hadn't get converted the reset_notifier stuff to use the
new API. It looks like I introduced it. Therefore:

Reviewed-by: Alex Bligh <alex@alex.org.uk>

Stefan & Paolo copied

Alex

> 
> 
> 
> 
> 2014-07-01 4:08 GMT+08:00 Alex Bligh <alex@alex.org.uk>:
> 
> 
> On 30 Jun 2014, at 20:53, Lb peace <peaceustc@gmail.com> wrote:
> 
> > If you use hwclock in guest os ,you will find the result of hwclock isn't changed after changing host os's clock.
> > I find this issue is generated in this patch:
> 
> I find it hard to believe that the patch you mention is the problem,
> as it's an automated output of perl simply changing one calling
> convention to another.
> 
> Is this because you are using an unusual clock= option?
> 
> Alex
> 
> 
> 
> > http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> > Before this patch,the result will be changed if you change host's clock.
> > It makes use of the following codes in qemu-timer.c:
> >         if (now < last) {
> >             notifier_list_notify(&clock->reset_notifiers, &now);
> >         }
> > It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
> > ---
> >  hw/timer/mc146818rtc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > index df54546..821c27e 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
> >      check_update_timer(s);
> >
> >      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
> > -    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
> > +    qemu_clock_register_reset_notifier(rtc_clock,
> >                                         &s->clock_reset_notifier);
> >
> >      s->suspend_notifier.notify = rtc_notify_suspend;
> > --
> >
> 
> --
> Alex Bligh
> 
> 
> 
> 
> 

-- 
Alex Bligh