Re: [PATCH 1/2] block: pass OnOffAuto instead of bool to block_acct_setup()

[Vladimir Sementsov-Ogievskiy]

On 8/17/22 12:28, Denis V. Lunev wrote:
> We would have one more place for block_acct_setup() calling, which should
> not corrupt original value.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Peter Krempa <pkrempa@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   block/accounting.c         | 24 ++++++++++++++++++++----
>   blockdev.c                 | 17 ++++++++++++++---
>   include/block/accounting.h |  6 +++---
>   3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/block/accounting.c b/block/accounting.c
> index 2030851d79..73ac639656 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -40,11 +40,27 @@ void block_acct_init(BlockAcctStats *stats)
>       }
>   }
>   
> -void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
> -                      bool account_failed)
> +static bool bool_from_onoffauto(OnOffAuto val, bool def)
>   {
> -    stats->account_invalid = account_invalid;
> -    stats->account_failed = account_failed;
> +    switch (val) {
> +    case ON_OFF_AUTO_AUTO:
> +        return def;
> +    case ON_OFF_AUTO_ON:
> +        return true;
> +    case ON_OFF_AUTO_OFF:
> +        return false;
> +    default:
> +        abort();
> +    }
> +}
> +
> +void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
> +                      enum OnOffAuto account_failed)
> +{
> +    stats->account_invalid = bool_from_onoffauto(account_invalid,
> +                                                 stats->account_invalid);
> +    stats->account_failed = bool_from_onoffauto(account_failed,
> +                                                stats->account_failed);
>   }
>   
>   void block_acct_cleanup(BlockAcctStats *stats)
> diff --git a/blockdev.c b/blockdev.c
> index 9230888e34..392d9476e6 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -455,6 +455,17 @@ static void extract_common_blockdev_options(QemuOpts 
> *opts, int *bdrv_flags,
>       }
>   }
>   
> +static OnOffAuto account_get_opt(QemuOpts *opts, const char *name)
> +{
> +    if (!qemu_opt_find(opts, name)) {
> +        return ON_OFF_AUTO_AUTO;
> +    }
> +    if (qemu_opt_get_bool(opts, name, true)) {
> +        return ON_OFF_AUTO_ON;
> +    }
> +    return ON_OFF_AUTO_OFF;
> +}
> +
>   /* Takes the ownership of bs_opts */
>   static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>                                      Error **errp)
> @@ -462,7 +473,7 @@ static BlockBackend *blockdev_init(const char *file, QDict 
> *bs_opts,
>       const char *buf;
>       int bdrv_flags = 0;
>       int on_read_error, on_write_error;
> -    bool account_invalid, account_failed;
> +    OnOffAuto account_invalid, account_failed;
>       bool writethrough, read_only;
>       BlockBackend *blk;
>       BlockDriverState *bs;
> @@ -496,8 +507,8 @@ static BlockBackend *blockdev_init(const char *file, QDict 
> *bs_opts,
>       /* extract parameters */
>       snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
>   
> -    account_invalid = qemu_opt_get_bool(opts, "stats-account-invalid", true);
> -    account_failed = qemu_opt_get_bool(opts, "stats-account-failed", true);
> +    account_invalid = account_get_opt(opts, "stats-account-invalid");
> +    account_failed = account_get_opt(opts, "stats-account-failed");

Hm. Actually here you change default behavior from true to false.

In the next patch you fix it back, but better is to avoid the extra change 
somehow.

>   
>       writethrough = !qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true);
>   
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 878b4c3581..b9caad60d5 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -27,7 +27,7 @@
>   
>   #include "qemu/timed-average.h"
>   #include "qemu/thread.h"
> -#include "qapi/qapi-builtin-types.h"
> +#include "qapi/qapi-types-common.h"
>   
>   typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>   typedef struct BlockAcctStats BlockAcctStats;
> @@ -100,8 +100,8 @@ typedef struct BlockAcctCookie {
>   } BlockAcctCookie;
>   
>   void block_acct_init(BlockAcctStats *stats);
> -void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
> -                     bool account_failed);
> +void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
> +                      enum OnOffAuto account_failed);
>   void block_acct_cleanup(BlockAcctStats *stats);
>   void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length);
>   BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,


--
Best regards,
Vladimir