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. -- Thanks, David / dhildenb
