Re: [Qemu-devel] [PATCH 06/10] hw/pvrdma: Dump device statistics counters to file

[Markus Armbruster]

Yuval Shaia <address@hidden> writes:

> On Mon, Feb 04, 2019 at 02:03:58PM +0100, Markus Armbruster wrote:
>> Yuval Shaia <address@hidden> writes:
>> 
>> > Signed-off-by: Yuval Shaia <address@hidden>
>> > ---
>> >  hw/rdma/vmw/pvrdma.h      |  1 +
>> >  hw/rdma/vmw/pvrdma_main.c | 72 +++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 73 insertions(+)
>> >
>> > diff --git a/hw/rdma/vmw/pvrdma.h b/hw/rdma/vmw/pvrdma.h
>> > index 167706ec2c..dc10f21ca0 100644
>> > --- a/hw/rdma/vmw/pvrdma.h
>> > +++ b/hw/rdma/vmw/pvrdma.h
>> > @@ -133,5 +133,6 @@ static inline void post_interrupt(PVRDMADev *dev, 
>> > unsigned vector)
>> >  }
>> >  
>> >  int pvrdma_exec_cmd(PVRDMADev *dev);
>> > +void pvrdma_dump_statistics(FILE *f, fprintf_function fprintf_func);
>> >  
>> >  #endif
>> 
>> The only user appears in the next patch.  I'd squash the two patches.
>> Matter of taste.
>
> Agree with you.
> I just did to help reviewers so ones that familiar with 'monitor' can
> review the other patch while rdma folks can review this one.
>
> Will probably squash them as soon as the conversion on the other patch will
> be over.
>
>> 
>> > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
>> > index cf82e78f08..79900076ec 100644
>> > --- a/hw/rdma/vmw/pvrdma_main.c
>> > +++ b/hw/rdma/vmw/pvrdma_main.c
>> > @@ -14,6 +14,7 @@
>> >   */
>> >  
>> >  #include "qemu/osdep.h"
>> > +#include "qemu/units.h"
>> >  #include "qapi/error.h"
>> >  #include "hw/hw.h"
>> >  #include "hw/pci/pci.h"
>> > @@ -36,6 +37,8 @@
>> >  #include 
>> > "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h"
>> >  #include "pvrdma_qp_ops.h"
>> >  
>> > +GSList *devices;
>> > +
>> >  static Property pvrdma_dev_properties[] = {
>> >      DEFINE_PROP_STRING("netdev", PVRDMADev, backend_eth_device_name),
>> >      DEFINE_PROP_STRING("ibdev", PVRDMADev, backend_device_name),
>> > @@ -55,6 +58,72 @@ static Property pvrdma_dev_properties[] = {
>> >      DEFINE_PROP_END_OF_LIST(),
>> >  };
>> >  
>> > +static void pvrdma_dump_device_statistics(gpointer data, gpointer 
>> > user_data)
>> > +{
>> > +    CPUListState *s = user_data;
>> > +    PCIDevice *pdev = data;
>> > +    PVRDMADev *dev = PVRDMA_DEV(pdev);
>> > +
>> > +    (*s->cpu_fprintf)(s->file, "%s_%x.%x\n", pdev->name,
>> > +                 PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> 
>> Why the indirection through CPUListState?  What's wrong with straight
>> monitor_printf()?
>
> No special reasoning, just wanted to utilize an existing mechanism and
> design.

Please use the simplest available mechanism unless you have an actual
need for a more complex one.  Complicating things just to prepare for
future needs rarely pays off, because YAGNI (you ain't gonna need it).