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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 2 Jul 2018 12:39:43 +0200
David Hildenbrand <david@redhat.com> wrote:

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