Re: [PATCH v2 1/2] timer impl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/14/2010 07:34 PM, Wen Congyang wrote:

In addition to Hu's comments, and the fact that you are probably going
to revise the exposed interface anyways, here's some additional points.

> * src/util/timer.c src/util/timer.h src/util/timer_linux.c src/util/timer_win32.c:
>   timer implementation
> * src/Makefile.am: build timer
> * src/libvirt_private.syms: Export public functions
> * src/libvirt.c: Initialize timer
> * configure.ac: check the functions in librt used by timer
> 
> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> 
>  
> -EXTRA_DIST += util/threads-pthread.c util/threads-win32.c
> +EXTRA_DIST += util/threads-pthread.c util/threads-win32.c \
> +		util/timer_linux.c

timer-win32.c?  Also, I'd go with timer-linux.c, not timer_linux.c.

> +# timer.h
> +get_clock;

Bad idea to pollute the namespace with get_clock; better would be
something like virGetClock.

> +virNewTimer;
> +virFreeTimer;
> +virModTimer;
> +virDelTimer;
>  
>  # usb.h
> +
> +static virTimerPtr timer_list = NULL;
> +static void realarm_timer(void);
> +static void __realarm_timer(uint64_t);

It is dangerous to declare functions in the __ namespace, since that is
reserved for libc and friends.

> +uint64_t get_clock(void)
> +{
> +    struct timespec ts;
> +
> +    clock_gettime(CLOCK_MONOTONIC, &ts);
> +    return ts.tv_sec * 1000000000ULL + ts.tv_nsec;

You probably ought to check for overflow here.  Dealing with raw
nanoseconds is rather fine-grained; is it any better to go with micro or
even milliseconds, or does libvirt really require something as precise
as nanosecond timeouts?

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]