Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[Igor Mammedov]

On Fri, 15 Jun 2018 11:48:33 +0200
David Hildenbrand <address@hidden> wrote:

> On 15.06.2018 11:34, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 16:15:48 +0200
> > David Hildenbrand <address@hidden> wrote:
> >   
> >> On 13.06.2018 15:10, Igor Mammedov wrote:  
> >>> On Mon, 11 Jun 2018 14:16:54 +0200
> >>> David Hildenbrand <address@hidden> wrote:
> >>>     
> >>>> We'll be factoring out some pc-dimm specific and some memory-device
> >>>> checks next.
> >>>>
> >>>> Signed-off-by: David Hildenbrand <address@hidden>
> >>>> ---
> >>>>  hw/i386/pc.c             | 2 ++
> >>>>  hw/mem/pc-dimm.c         | 5 +++++
> >>>>  hw/ppc/spapr.c           | 1 +
> >>>>  include/hw/mem/pc-dimm.h | 2 ++
> >>>>  4 files changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>>> index 017396fe84..dc8e7b033b 100644
> >>>> --- a/hw/i386/pc.c
> >>>> +++ b/hw/i386/pc.c
> >>>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler 
> >>>> *hotplug_dev, DeviceState *dev,    
> >>> keeping                              ^^^^^
> >>> similar to newly introduced pc_dimm_memory_pre_plug()
> >>> and I have no clue what to suggest as alternative    
> >>
> >> Can you elaborate?  
> > 
> > It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug
> > are very similar so it become confusing and with name alone
> > you can't figure if both do the same or different things.
> > 
> > Looking at 11/11 maybe you could just drop this and the next
> > patch for now as there isn't obvious (if any) demand for it
> > within this series at all.
> > And introduce similar patch when it's actually required.
> > 
> > I might imagine following naming in future:
> > 
> >    pc_dimm_pre_plug()
> >        memory_device_pre_plug()
> >        ... some pc-dimm only stuff ...
> > 
> >    virtio_mem_pre_plug()
> >        memory_device_pre_plug()
> >        ... some virtio-mem only stuff ...  
> 
> We will always have the chain
> 
> $machine_XXX_pre_plug()
>       ... some machine specific stuff
>       pc_dimm_memory_pre_plug()
>               ... some pc-dimm specific stuff
>               memory_device_pre_plug()
> 
> I can rename
> 
> pc_dimm_(plug|unplug ...) to
> pc_memory_(plug|unplug ...)
> 
> and pc_dimm_memory_(plug|unplug ...) to
> pc_dimm_(plug|unplug ...)
> 
> That way we match spapr naming scheme:
> 
> spapr_memory_plug/spapr_memory_unplug

sounds good to me