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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 12.06.2019 um 09:59 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> 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 <address@hidden>
>> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>> 
>> 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)

I disagree with the parenthesis.  The completeness argument is really
simple: each occurence of member qmp is either guarded by a "is a QMP
monitor" conditional, or an "is a QMP monitor" assertion, or in a
callback that takes a QMP monitor converted to void * (I didn't bother
asserting anything there).

>                          and since both Dave and you already reviewed
> the patch in its current form

Actually, I didn't review the patch "in its current form", because I
found that more bothersome than splitting it up and reviewing the parts.

By effectively squashing together the parts, you have of course every
right to claim the resulting code passed my review.  That's not quite
the same as claiming my R-by for the *patch*.

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

Having the commit message explain that the patch mixes mechanical change
for the "replace QMP member by child class" data reorganization with its
prerequisite cleanup "access QMP stuff only when the monitor is actually
a QMP monitor" might suffice to make me acquiesce to the squashed patch.