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

[Max Reitz]

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
either.

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
against.

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...

Max

signature.asc
Description: OpenPGP digital signature