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

[Paolo Bonzini]

On 17/10/2018 12:57, Artem Pisarenko wrote:
>> 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.
> 
> Yes, I've just forgot to update comments after previous patch version,
> where it actually was macro.
> 
>> 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).
> 
> It's just to prevent compilation errors, as required by
> https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
> 
>> 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)
> 
> Agree.

Like this?

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 86ce70f20e..ef7526e389 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -55,25 +55,13 @@ typedef enum {
 /**
  * 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.
+ * An individual timer may be given one or multiple attributes when
initialized.
+ * Each attribute corresponds to one bit.  Attributes modify the processing
+ * of timers when they fire.
  *
  * No attributes defined currently.
  */

-typedef enum {
-    QEMU_TIMER_ATTRBIT__NONE
-} QEMUTimerAttrBit;
-
-#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)
-
 typedef struct QEMUTimerList QEMUTimerList;

 struct QEMUTimerListGroup {
@@ -640,14 +628,6 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType
type, QEMUTimerCB *cb,
     return timer_new(type, SCALE_MS, cb, opaque);
 }

-/**
- * timer_get_attributes:
- * @ts: the timer
- *
- * Return 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values
- */
-int timer_get_attributes(QEMUTimer *ts);
-
 /**
  * timer_deinit:
  * @ts: the timer to be de-initialised
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 2046b68c15..04527a343f 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -355,11 +355,6 @@ void timer_init_full(QEMUTimer *ts,
     ts->expire_time = -1;
 }

-int timer_get_attributes(QEMUTimer *ts)
-{
-    return ts->attributes;
-}
-
 void timer_deinit(QEMUTimer *ts)
 {
     assert(ts->expire_time == -1);