Re: [Qemu-devel] [PATCH v2 2/2] machine: Introduce QEMU_COMPAT_* macros

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 10:23:06AM -0300, Eduardo Habkost wrote:
> On Wed, Jun 25, 2014 at 08:56:43AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 24, 2014 at 11:04:45PM -0300, Eduardo Habkost wrote:
> > > The QEMU_COMPAT_* macros will contain compat properties that are not
> > > specific to PC, and may be reused by other machine-types.
> > > 
> > > The compat properties for PC-specific devices were moved to
> > > QEMU_COMPAT_* too, because they are simply not going to be applied to
> > > any device if they are not instantiated.
> > > 
> > > Signed-off-by: Eduardo Habkost <address@hidden>
> > 
> > I don't see value in this.
> > If some target wants to start versioning their machine types,
> > let them start from 2.0 (or even 2.1 - does any non-PC target
> > plan to support cross-version compatibility for 2.0?).
> > Why carry around useless code for < 2.0 for non PC platforms?
> > 
> > > ---
> > >  include/hw/compat.h  | 210 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/i386/pc.h | 206 
> > > +++-----------------------------------------------
> > >  2 files changed, 221 insertions(+), 195 deletions(-)
> > >  create mode 100644 include/hw/compat.h
> > 
> > And we have lost all we have gained in 1/2 :(
> > 
> 
> As we will be required to follow the QEMU_COMPAT/PC_COMPAT pattern
> starting from 2.1, I like to keep exactly the same pattern for all older
> macros, even if QEMU_COMPAT_1_* are used only by PC. Makes it easier to
> read and understand, and information easier to find (instead of having
> the old compat lists scattered all over the place, most of them are now
> in a single header file).
> 
> Also, note that the PC_COMPAT_* macros will be removed once we convert
> PC to pure QOM code after 2.1.0.
> 
> Anyway, if the maintainers see no value in having QEMU_COMPAT_* macros
> used only by PC, we can do this only after 2.1. Otherwise, if there's
> value in a QEMU_COMPAT_2_0 macro used only by PC, there's value in
> QEMU_COMPAT_1_* too.
> 
> In either case, this is simply a demonstration of how we can proceed to
> introduce the versioned non-PC machine-types later. We don't strictly
> need this patch today.

I think it's best for you guys to hammer on it on a private
branch. It's past devel freeze and this is new code,
so I'm not applying anything that's not trivial.
Patch 1/2 seems to hit the sweet spot.


> > > 
> [...]
> > > +    }
> > > +
> > > +
> > 
> > Two empty lines.
> 
> I will fix it in case I submit it again. Thanks.
> 
> > 
> [...]
> > > -        }
> > > +    PC_COMPAT_1_5,\
> > > +    QEMU_COMPAT_1_4
> > > +
> > >  
> > 
> > Two emoty lines.
> 
> Ditto.
> 
> -- 
> Eduardo