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

[Auger Eric <eric.auger@redhat.com>]

Hi David,
On 07/02/2018 02:47 PM, David Hildenbrand wrote:
> On 02.07.2018 14:44, Igor Mammedov wrote:
>> On Mon, 2 Jul 2018 12:39:43 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 02.07.2018 12:31, Igor Mammedov wrote:
>>>> On Mon,  2 Jul 2018 11:37:55 +0200
>>>> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>  
>>>> with commit message fixup,
>>>>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>> 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.
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric