Re: [Qemu-devel] [PATCH 2/3] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 14/10/2018 16:55, Artem Pisarenko wrote:
> Attributes are simple flags, associated with individual timers for their 
> whole lifetime.
> They intended to be used to mark individual timers for special handling by 
> various qemu features operating at qemu core level.
> Existing timer, aio and coroutine interface extended with attribute-enabled 
> variants of functions, which create/initialize timers.
> 
> Signed-off-by: Artem Pisarenko <address@hidden>
> ---
> 
> Notes:
>     Conversion and association between QEMUTimerAttrBit and accessor macro 
> are dumb.
>     Maybe better alternatives exist (like QFlags in Qt framework) or existing 
> qemu code may be reused, if any.
>     Attributes also may be better named as flags, but they looks like 
> something volatile, whereas 'attribute' expresses constant nature better.
> 
>  include/block/aio.h         |  50 +++++++++++++++++++-
>  include/qemu/coroutine.h    |   5 +-
>  include/qemu/timer.h        | 110 
> ++++++++++++++++++++++++++++++++++++++------
>  tests/ptimer-test-stubs.c   |   7 +--
>  util/qemu-coroutine-sleep.c |   6 ++-
>  util/qemu-timer.c           |  12 +++--
>  6 files changed, 165 insertions(+), 25 deletions(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..a6be3fb 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -407,10 +407,35 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, 
> QEMUClockType type,
>                                         int scale,
>                                         QEMUTimerCB *cb, void *opaque)
>  {
> -    return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
> +    return timer_new_a_tl(ctx->tlg.tl[type], scale, 0, cb, opaque);
>  }

The new function _a_tl is a bit ugly.

I would prefer to:

- only have a new timer_new_full that takes all of scale, attribute and
timerlist

- accept a NULL timerlist and default to main_loop_tlg.tl[type]

- add aio_timer_{new,init}_with_attrs

> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f..cffc2b2 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -276,7 +276,10 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
>  /**
>   * Yield the coroutine for a given duration
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +#define qemu_co_sleep_ns(type, ns) \
> +    qemu_co_sleep_a_ns(type, 0, ns)
> +void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
> +                                     int64_t ns);

I wouldn't bother adding co_sleep_a_ns unless it's needed.

>  /**
>   * Yield until a file descriptor becomes readable
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 39ea907..031e3a1 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -52,6 +52,28 @@ typedef enum {
>      QEMU_CLOCK_MAX
>  } QEMUClockType;
>  
> +/**
> + * QEMU Timer attributes:
> + *
> + * An individual timer may be assigned with one or multiple attributes when
> + * initialized.
> + * Attribute is a static flag, meaning that timer has corresponding property.
> + * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
> + * which used to initialize timer, stored to 'attributes' member and can be
> + * retrieved externally with timer_get_attributes() call.
> + * Values of QEMUTimerAttrBit aren't used directly,
> + * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
> + * where 'id' is a unique part of attribute identifier.

I think QEMU_TIMER_ATTR is an unnecessary complication.  I would just
add a TIMER_ATTR_EXTERNAL constant.

Paolo