On 15.04.2016 07:46, Changlong Xie wrote: > Signed-off-by: Changlong Xie <xiecl.f...@cn.fujitsu.com> > --- > 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
