Re: [Qemu-devel] [PATCH v2 03/11] monitor: Make MonitorQMP a child class of Monitor

[Kevin Wolf <kwolf@redhat.com>]

Am 12.06.2019 um 09:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Currently, struct Monitor mixes state that is only relevant for HMP,
> > state that is only relevant for QMP, and some actually shared state.
> > In particular, a MonitorQMP field is present in the state of any
> > monitor, even if it's not a QMP monitor and therefore doesn't use the
> > state.
> >
> > As a first step towards a clean separation between QMP and HMP, let
> > MonitorQMP extend Monitor and create a MonitorQMP object only when the
> > monitor is actually a QMP monitor.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> This is a bit harder to review than necessary, because it mixes the
> largely mechanical "replace QMP member by child class" with the
> necessary prerequisite "clean up to access QMP stuff only when the
> monitor is actually a QMP monitor".  I'm going to post a split.
> 
> Effectively preexisting: we go from Monitor * to MonitorQMP * without
> checking in several places.  I'll throw in assertions.

Since I don't think doing both in one patch makes review a lot harder
(and in fact think your patch 2.5 is harder to review for completeness
that the combined patch) and since both Dave and you already reviewed
the patch in its current form and I don't want to invalidate that
review, I'm going to keep it as a single patch and just squash in the
additional assertions where container_of() is used. The resulting code
is the same anyway.

Kevin