[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qem
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem |
Date: |
Wed, 17 Oct 2018 10:12:04 +0100 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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)
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH v2 0/4] Introduce attributes for timers subsystem and remove QEMU_CLOCK_VIRTUAL_EXT clock type, Artem Pisarenko, 2018/10/17
- [Qemu-devel] [PATCH v2 1/4] Revert some patches from recent [PATCH v6] "Fixing record/replay and adding reverse debugging", Artem Pisarenko, 2018/10/17
- [Qemu-devel] [PATCH v2 4/4] Optimize record/replay checkpointing for all clocks it applies to, Artem Pisarenko, 2018/10/17
- [Qemu-devel] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Artem Pisarenko, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Paolo Bonzini, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Artem Pisarenko, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Paolo Bonzini, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Artem Pisarenko, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Paolo Bonzini, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Artem Pisarenko, 2018/10/17
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] Introduce attributes to qemu timer subsystem, Stefan Hajnoczi, 2018/10/18
[Qemu-devel] [PATCH v2 3/4] Restores record/replay behavior related to special virtual clock processing for timers used in external subsystems., Artem Pisarenko, 2018/10/17