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

[David Hildenbrand <david@redhat.com>]

On 13.06.2018 15:10, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:54 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We'll be factoring out some pc-dimm specific and some memory-device
>> checks next.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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?

In pc.c we now have:
- static void pc_dimm_plug()
- static void pc_dimm_unplug_request()
- static void pc_dimm_unplug()

And I add
- static void pc_dimm_pre_plug()

I am sticking to the existing naming scheme.

(maybe the problem is that PC_DIMM should never have been named PC_DIMM
but simply DIMM, then the "pc_" prefix would be unique)

> 
>>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>>          return;
>>      }
>> +
>> +    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
>>  }
>>  
>>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index bc79dd04d8..995ce22d8d 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -27,6 +27,11 @@
>>  #include "sysemu/numa.h"
>>  #include "trace.h"
>>  
>> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
>> +                             Error **errp)
>> +{
>> +}
> why introducing shim without anything?

Because you requested for review to split things up :)

I can easily squash this.

> I'd just merge it with following patch and clarify (in commit message)
> why the rest of pre_plug code isn't moved here as well.


-- 

Thanks,

David / dhildenb