[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH v1 2/2] block: remove redundant stats of block

From: Max Reitz
Subject: Re: [Qemu-trivial] [PATCH v1 2/2] block: remove redundant stats of block_acct_start()
Date: Sun, 17 Apr 2016 00:11:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2

On 15.04.2016 07:46, Changlong Xie wrote:
> Signed-off-by: Changlong Xie <address@hidden>
> ---
>  block/accounting.c         |  4 ++--
>  dma-helpers.c              |  2 +-
>  hw/block/nvme.c            |  3 +--
>  hw/block/virtio-blk.c      |  6 ++----
>  hw/block/xen_disk.c        |  6 ++----
>  hw/ide/atapi.c             | 12 ++++--------
>  hw/ide/core.c              | 16 +++++++---------
>  hw/ide/macio.c             |  9 +++------
>  hw/scsi/scsi-disk.c        | 29 ++++++++++-------------------
>  include/block/accounting.h |  4 ++--
>  qemu-io-cmds.c             |  8 +++-----
>  11 files changed, 37 insertions(+), 62 deletions(-)

Without having looked too closely into each hunk, the patch itself looks OK.

But I'm not so sure whether we want it. Of course, it's obvious that the
parameter is not needed in a technical sense, but the question is why it
exists at all. It definitely wasn't forgotten at some point: Commit
5366d0c8 made the function take this parameter instead of a BDS and it
was totally obvious that it didn't make use of it at all. And this
wasn't something new, the BDS parameter before that commit wasn't used

Every single function in block/accounting.c takes a BlockAcctStats
argument as its first parameter. It just so happens that this function
doesn't make use of it. But I think that was a conscious choice.

So if you had asked me before you wrote this patch, I would have said
that we don't need it. But now it's here, so the pain of writing it is
gone. I don't have a real preference either way. Both having that
parameter and not having it are valid choices which can be argued for or

I'd like to say that my inertia is keeping me from applying this patch,
but I'd feel like a hypocrite for saying that, considering it would have
taken much less time than writing this response...


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]