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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

On Wed, Oct 17, 2018 at 02:24:19PM +0600, 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.

I'm worried that this sentence suggests various parts of QEMU will stash
state in ts->attributes.  That's messy and they shouldn't do this.  Make
the field private to qemu-timer.c.

Attributes should only affect qemu-timer.c behavior.  Other parts of
QEMU should not act differently based on timer attributes (i.e. checking
bits).  If they need to do that then it suggests something isn't
properly encapsulated in qemu-timer.c.

> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..fce9d48 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -388,6 +388,31 @@ struct LinuxAioState *aio_setup_linux_aio(AioContext *ctx, Error **errp);
>  struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
>  
>  /**
> + * aio_timer_new_with_attrs:
> + * @ctx: the aio context
> + * @type: the clock type
> + * @scale: the scale
> + * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign

Further down in this patch the notation is QEMU_TIMER_ATTR_<id>, which I
think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
macro.  Please use the QEMU_TIMER_ATTR_<id> notation consistently.

> +/**
> + * 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.
> + *
> + * No attributes defined currently.
> + */
> +
> +typedef enum {
> +    QEMU_TIMER_ATTRBIT__NONE
> +} QEMUTimerAttrBit;
> +
> +#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)

What is the purpose of this bit?  I guess it's just here as a
placeholder because no real bits have been defined yet.  Hopefully the
next patch removes it (/* This placeholder is removed in the next patch
*/ would be a nice way to document this for reviewers).

The enum isn't needed and makes debugging harder since the bit number is
implicit in the enum ordering.  This alternative is clearer and more
concise:

  #define QEMU_TIMER_ATTR_foo BIT(n)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]