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 <da...@redhat.com> wrote: >> >>> On 02.07.2018 12:31, Igor Mammedov wrote: >>>> On Mon, 2 Jul 2018 11:37:55 +0200 >>>> David Hildenbrand <da...@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 <da...@redhat.com> >>>> with commit message fixup, >>>> >>>> Reviewed-by: Igor Mammedov <imamm...@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.au...@redhat.com>
Thanks Eric