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

[Artem Pisarenko]

> 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.

>ср, 17 окт. 2018 г. в 15:15, Paolo Bonzini <address@hidden>:
>On 17/10/2018 11:12, Stefan Hajnoczi 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.
>
> Yes, the contents of the fields are private.  Are you suggesting a
> different wording for the commit message or the "QEMU Timer attributes"
> doc comment, or something more than that?  Possibly removing
> timer_get_attributes altogether?

Actually, attributes aren't intended to be changed externally of timers
code.
The purpose of timer_get_attributes() is just informational, same like if
timer_get_clock(), timer_get_scale(), etc. would exist. But since none of
such accessors exists, adding just one for 'attributes' looks exceptional.
And it isn't being used even after whole patch series applied. As such, it
could be just removed.
Any suggestons to improve commit message and/or comments, if any ?
-- 

С уважением,
  Артем Писаренко