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/.
