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

[David Hildenbrand <david@redhat.com>]

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.

-- 

Thanks,

David / dhildenb