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

[Anton Nefedov]

On 23/11/2018 7:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> 31.10.2018 14:34, Anton Nefedov wrote:
>> This adds some protection from accounting unitialized cookie.
> 
> uninitialized
> 

fixed

>> 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 unitialized holding garbage value and there is
> 
> and here.
> 

fixed

>>    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.
> 
> so, is it an existing bug? Could you please give an example in the code of 
> such wrong (before the patch) accounting?
> 

e.g. look at ide_dma_cb()->ide_handle_rw_error() call. In IDE_DMA_TRIM
case, the cookie is never initialized, but it is block_acct_failed()-ed
anyway.

>>
>> 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) {
>> +        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)
>>
> 
>