Yuval Shaia <yuval.sh...@oracle.com> writes:
> On Mon, Feb 04, 2019 at 02:03:58PM +0100, Markus Armbruster wrote:
>> Yuval Shaia <yuval.sh...@oracle.com> writes:
>>
>> > Signed-off-by: Yuval Shaia <yuval.sh...@oracle.com>
>> > ---
>> > 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).
