[libvirt] [PATCH v3 1/2] timer impl

[Eric Blake]

On 12/17/2010 01:49 AM, Wen Congyang wrote:
> * tools/timer.c tools/timer.h: timer implementation
> * tools/virsh.c: Initialize timer
> * tools/Makefile.am: build timer
> 
> Signed-off-by: Wen Congyang <wency at cn.fujitsu.com>
> 
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +struct virTimer {
> +    int timer_id;
> +
> +    int frequency;

In which unit?  milliseconds?  Worse, this variable is unused.

> +/* use timerFunc to prevent the user know timer id. */
> +static void timerFunc(int timer_id ATTRIBUTE_UNUSED, void *opaque)
> +{
> +    virTimerPtr timer = (virTimerPtr)opaque;

This cast is not necessary (in general, in C you can go from void* to
any other type without a cast).

> +    timer->function(timer->opaque);
> +}
> +
> +virTimerPtr virNewTimer(virTimerCallback callback, void *opaque)

Would you mind adding some documentation to each public function,
describing the parameters and return value?  Right now you're only using
it with virsh, but it may make sense to move it to src/util, and good
documentation will ensure that we like the interface (or the act of
documenting it may help you find ways to improve it).

> +
> +int virAddTimer(virTimerPtr timer, int expire_time)
> +{
> +    int ret;
> +
> +    if ((ret = virEventAddTimeout(expire_time, timerFunc,
> +                                  timer, NULL)) < 0) {

Is this function safe to call when the timer is already in use?  If not,
should you add some sanity checking?

> +int virModTimer(virTimerPtr timer, int expire_time)
> +{
> +    if (timer->timer_id == -1)
> +        return virAddTimer(timer, expire_time);
> +
> +    virEventUpdateTimeout(timer->timer_id, expire_time);
> +    return 0;

Do we need both Add and Update, or can we have a single SetTimeout
interface that nukes the previous timeout value and replaces it with the
new parameter (cf. the alarm() function in POSIX).

> +}
> +
> +int virDelTimer(virTimerPtr timer)
> +{
> +    if (timer->timer_id == -1)
> +        return 0;
> +
> +    if (virEventRemoveTimeout(timer->timer_id) < 0)
> +        return -1;

If you merge Add and Mod into one interface, then having a sentiel
expire_time value of -1 would also serve as a way to disarm any existing
timer, so you could also merge Del into that same interface.

> +void virFreeTimer(virTimerPtr timer)
> +{

if (timer == NULL) return;

> +    VIR_FREE(timer);

Also, modify cfg.mk to list this as a free-like function.

> +}
> +
> +static int timer_initialized = 0;
> +static virThread timer_thread;
> +static bool timer_running = false;
> +static bool timer_quit = false;

Explicit zero-initialization is not required for file-scope variables (C
guarantees it) - three instances.

> +int virTimerInitialize(void)
> +{
> +    if (timer_initialized)
> +        return 0;
> +
> +    if (virMutexInit(&timer_lock) < 0)
> +        return -1;
> +
> +    timer_initialized = 1;
> +    timer_running = false;
> +    timer_quit = false;

At least two bad data races if we expose virTimerInitialize to a
multi-threaded environment (and probably more):

Thread 1 calls virTimerInitialize, calls virMutexInit, but gets swapped
out before time_initialized is set to 1; thread 2 calls
virTimerInitialize and tries to reinitialize the mutex, which results in
undefined behavior.

Thread 1 calls virTimerInitialize, sets timer_initialized to 1, but gets
swapped out before timer_running is set to false; thread 2 calls
virTimerInitialize, which returns, then starts a timer; thread 1 resumes
and sets timer_running to false, nuking thread 2's timer.


I'm thinking we need to teach src/util/threads.h about some guaranteed
once-only wrappers around pthread_once and the Windows counterpart, and
then use virOnce throughout our code base to guarantee race-free
one-time initialization for things such as virMutexInit, so that we are
immune to data races where two threads both try to initialize the same
mutex (this affects more than just your patch); once you have race-free
mutex initialization, you can then use that mutex to reliably avoid the
remaining races.

But for your particular use case, you are only using timers in the
context of a single-threaded virsh, so it's something we can defer to
later without impacting the utility of your patch.

> +
> +typedef struct virTimer virTimer;

Our convention is:

typedef struct _virTimer virTimer;

that is, the struct version has a leading _ if the type is intentionally
opaque.

> +typedef virTimer *virTimerPtr;
> +typedef void (*virTimerCallback)(void *);
> +
> +extern virTimerPtr virNewTimer(virTimerCallback, void *);

ATTRIBUTE_NONNULL(1)

> +extern int virAddTimer(virTimerPtr, int);

ATTRIBUTE_NONNULL(1)

> +extern int virModTimer(virTimerPtr, int);

ATTRIBUTE_NONNULL(1)

> +extern int virDelTimer(virTimerPtr);

ATTRIBUTE_NONNULL(1)

> +extern void virFreeTimer(virTimerPtr);
> +extern int virStartTimer(void);
> +extern int virStopTimer(void);
> +extern int virTimerInitialize(void);

Is the intent here that virTimerInitialize creates the timer resources,
then you can use virStartTimer/virStopTimer in pairs at will to control
whether timers are in effect, and finally virFreeTimer to remove all
resources?  I'm agreeing with Hu that this seems a bit complex, and you
may be better served by merging virStartTimer into virTimerInitialize (a
one-shot call to enable you to call virAddTimer/virFreeTimer at will),
and delete virStopTimer (the one-time resource never needs to be
reclaimed).  See how we have virThreadInitialize, called once at
initialization, with no other functions to control whether thread
resources are in use.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101217/469f6cfc/attachment-0001.sig>