On 02.07.2018 14:44, Igor Mammedov wrote: > On Mon, 2 Jul 2018 12:39:43 +0200 > David Hildenbrand <[email protected]> wrote: > >> On 02.07.2018 12:31, Igor Mammedov wrote: >>> On Mon, 2 Jul 2018 11:37:55 +0200 >>> David Hildenbrand <[email protected]> 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 <[email protected]> >>> with commit message fixup, >>> >>> Reviewed-by: Igor Mammedov <[email protected]> >>> --- >>> 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
