Re: [PATCH v3 1/6] throttle: introduce enum ThrottleType

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Use enum ThrottleType instead of number index.
>
> Reviewed-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: zhenwei pi<pizhenwei@bytedance.com>
> ---
>   include/qemu/throttle.h | 11 ++++++++---
>   util/throttle.c         | 16 +++++++++-------
>   2 files changed, 17 insertions(+), 10 deletions(-)

[...]

> diff --git a/util/throttle.c b/util/throttle.c
> index 81f247a8d1..5642e61763 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts,
>   void throttle_timers_attach_aio_context(ThrottleTimers *tt,
>                                           AioContext *new_context)
>   {
> -    tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                                  tt->read_timer_cb, tt->timer_opaque);
> -    tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                                  tt->write_timer_cb, tt->timer_opaque);
> +    tt->timers[THROTTLE_READ] =
> +        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                      tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
> +    tt->timers[THROTTLE_WRITE] =
> +        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                      tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);

This could benefit from the new structure:

```
for (int i = 0; i < THROTTLE_MAX; i++) {
    tt->timers[i] = aio_timer_new(..., tt->timer_cb[i], ...);
}
```

Even more so after patch 3.  Still, optional, of course, so either way:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Of note is that we still have instances in util/throttle.c and 
block/throttle-groups.c that don’t use these enums to access the 
tt->timers[] array, and that’s a bit unfortunate now (i.e. 
`tt->timers[is_write]` should rather be `tt->timers[is_write ? 
THROTTLE_WRITE : THROTTLE_READ]` instead).  But functionally it’s OK and 
I believe patches 3 and 6 do address this.