Re: [Qemu-devel] [RFC] [PATCHv6 01/16] aio / timers: add qemu-timer.c utility functions

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

Stefan,

--On 6 August 2013 14:02:18 +0200 Stefan Hajnoczi <stefanha@redhat.com> 
wrote:

> There is still too much happening in this patch.  Making
> qemu_new_clock()/qemu_free_clock() public and moving the clock source
> constants can be done in a single patch.

OK I'll split it up.

> The next patch can change the semantics of qemu_clock_deadline() to
> return INT32_MAX when the clock source is disabled.  I'm not sure why
> you do this and whether you checked that existing users continue to work
> correctly?

Rationale: I suspect it doesn't really matter (see below) but the
rationale was that if the clock is disabled, timers will expire in
an infinite time. I was trying to make treatment of expire times
consistent throughout qemu-timer, and this was the one place a
disabled clock was being treated as if the timers were going to
expire.

Audit: The only users (at least by the time my patch set is finished) are:

* cpus.c within qtest_warp(), which appears not to consider the case of
  vm_clock being disabled. I do not think this function has been written
  considering the possibility that there are no active vm_clock timers
  or where the deadline is > INT32_MAX ns away.

* qtest_process_command(), evaluating clock_step, which calls the above.

My preference would be to move these to qemu_clock_deadline_ns (without
the INT32_MAX check) and delete the old qemu_clock_deadline routine
entirely, but I don't really understand the full set of circumstances
in which the qtest routines are meant to work.

Of course cpus.c now uses qemu_clock_deadline_ns_all for a couple of
the previous two uses, and in patch 14 of my series I've commented
that I think the previous use was buggy but have maintained bug-for-bug
compatibility.

I'd particularly like comments on patch 14, which I've mostly blind
coded based on what Paolo asked for. I'm afraid I found the icount
stuff a bit opaque. I'll hold off from v7 until someone's had a look
at these.

> Please include
> an explanation of why qemu_timeout_ns_to_ms() will be needed in the
> future (there are no callers in this patch).

You mean in the commit text as well as the following?

+/* Transition function to convert a nanosecond timeout to ms
+ * This is used where a system does not support ppoll
+ */

-- 
Alex Bligh