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

[David Hildenbrand]

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

-- 

Thanks,

David / dhildenb