Re: [Qemu-block] [PATCH v8 3/9] block: add empty account cookie type

[Vladimir Sementsov-Ogievskiy]

16.05.2019 17:33, Anton Nefedov wrote:
> This adds some protection from accounting uninitialized cookie.
> That is, block_acct_failed/done without previous block_acct_start;
> in that case, cookie probably holds values from previous operation.
> 
> (Note: it might also be uninitialized holding garbage value and there is
>   still "< BLOCK_MAX_IOTYPE" assertion for that.
>   So block_acct_failed/done without previous block_acct_start should be used
>   with caution.)
> 
> Currently this is particularly useful in ide code where it's hard to
> keep track whether the request started accounting or not. For example,
> trim requests do the accounting separately.
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>   include/block/accounting.h | 1 +
>   block/accounting.c         | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index ba8b04d572..878b4c3581 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>   typedef struct BlockAcctStats BlockAcctStats;
>   
>   enum BlockAcctType {
> +    BLOCK_ACCT_NONE = 0,
>       BLOCK_ACCT_READ,
>       BLOCK_ACCT_WRITE,
>       BLOCK_ACCT_FLUSH,
> diff --git a/block/accounting.c b/block/accounting.c
> index 70a3d9a426..8d41c8a83a 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, 
> BlockAcctCookie *cookie,
>   
>       assert(cookie->type < BLOCK_MAX_IOTYPE);
>   
> +    if (cookie->type == BLOCK_ACCT_NONE) {

worth error_report() ?

> +        return;
> +    }
> +
>       qemu_mutex_lock(&stats->lock);
>   
>       if (failed) {
> @@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, 
> BlockAcctCookie *cookie,
>       }
>   
>       qemu_mutex_unlock(&stats->lock);
> +
> +    cookie->type = BLOCK_ACCT_NONE;
>   }
>   
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir