Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug

[David Hildenbrand]

On 02.07.2018 14:44, Igor Mammedov wrote:
> On Mon, 2 Jul 2018 12:39:43 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 02.07.2018 12:31, Igor Mammedov wrote:
>>> On Mon,  2 Jul 2018 11:37:55 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> We can assign and verify the slot before realizing and trying to plug.  
>>> s/slot/"addr"/
>>>   
>>>> reading/writing the address property should never fail for DIMMs, so let's
>>>> reduce error handling a bit by using &error_abort. Getting access to the
>>>> memory region now might however fail. So forward errors from
>>>> get_memory_region() properly.
>>>>
>>>> As all memory devices should use the alignment of the underlying memory
>>>> region for guest physical address asignment, do detection of the
>>>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>>>> alignment for compatibility handling.
>>>>
>>>> Signed-off-by: David Hildenbrand <address@hidden>  
>>> with commit message fixup,
>>>
>>> Reviewed-by: Igor Mammedov <address@hidden>
>>> ---
>>> For future reference, I don't really like 2 things about patch
>>>  1: mixes both error handling and functional changes (should be separate 
>>> patches)
>>>  2: that property setter may crash QEMU at preplug stage
>>>     where it should gracefully error out (so put proper error check
>>>     as follow up patch or respin this one maybe taking in account #1.)  
>>
>> Thanks for the review.
>>
>> 1. could have been factored out into a separate patch.
>>
>> Regarding 2, I won't respin as long there is nothing else to take care
>> of. As long as we are in the pc-dimm world and we have static
>> properties, I don't see any reason to add error handling that cannot
>> happen. It will be different once we factor out more stuff into memory
>> device code. I assume you have a different opinion about that, but I
>> consider these nits.
> it's not really nits. it happens to work now but static properties are
> just current impl. detail which can easily change without amending
> *plug handlers.
> It's cleaner/safer to handle errors properly and keeping device model
> separate from machine helpers as much as possible from maintainer pov,
> even at expense of extra boiler plate.
> 
> I expect you to re-introduce proper error checking when you
> factor out memory_device_pre_plug() part /it's condition for my ack
> on this patch/.
> 
Yes, already contained.

-- 

Thanks,

David / dhildenb