HI,
On Wed, May 18, 2022 at 09:51:56AM +0100, Daniel P. Berrangé wrote:
> On Wed, May 18, 2022 at 10:10:48AM +0200, Victor Toso wrote:
> > Hi,
> >
> > On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, May 11, 2022 at 08:38:04AM -0700, Andrea Bolognani wrote:
> > > > On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> > > > > In 7.0.0 we can now generate
> > > > >
> > > > > type BlockResizeArguments struct {
> > > > > V500 *BlockResizeArgumentsV500
> > > > > V520 *BlockResizeArgumentsV520
> > > > > V700 *BlockResizeArgumentsV700
> > > > > }
> > > > >
> > > > > type BlockResizeArgumentsV500 struct {
> > > > > Device string
> > > > > Size int
> > > > > }
> > > > >
> > > > > type BlockResizeArgumentsV520 struct {
> > > > > Device *string
> > > > > NodeName *string
> > > > > Size int
> > > > > }
> > > > >
> > > > > type BlockResizeArgumentsV700 struct {
> > > > > NodeName string
> > > > > Size int
> > > > > }
> > > > >
> > > > > App can use the same as before, or switch to
> > > > >
> > > > > node := "nodedev0"
> > > > > cmd := BlockResizeArguments{
> > > > > V700: &BlockResizeArguments700{
> > > > > NodeName: node,
> > > > > Size: 1 * GiB
> > > > > }
> > > > > }
> > > >
> > > > This honestly looks pretty unwieldy.
> > >
> > > It isn't all that more verbose than without the versions - just
> > > a single struct wrapper.
> > >
> > > >
> > > > If the application already knows it's targeting a specific version of
> > > > the QEMU API, which for the above code to make any sense it will have
> > > > to, couldn't it do something like
> > > >
> > > > import qemu .../qemu/v700
> > > >
> > > > at the beginning of the file and then use regular old
> > > >
> > > > cmd := qemu.BlockResizeArguments{
> > > > NodeName: nodeName,
> > > > Size: size,
> > > > }
> > > >
> > > > instead?
> > >
> > > This would lead to a situation where every struct is duplicated
> > > for every version, even though 90% of the time they'll be identical
> > > across multiple versions. This is not very ammenable to the desire
> > > to be able to dynamically choose per-command which version you
> > > want based on which version of QEMU you're connected to.
> > >
> > > ie
> > >
> > >
> > > var cmd Command
> > > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > > cmd = BlockResizeArguments{
> > > V700: &BlockResizeArguments700{
> > > NodeName: node,
> > > Size: 1 * GiB
> > > }
> > > }
> > > } else {
> > > cmd = BlockResizeArguments{
> > > V520: &BlockResizeArguments520{
> > > Device: dev,
> > > Size: 1 * GiB
> > > }
> > > }
> > > }
> > >
> > > And of course the HasVersion check is going to be different
> > > for each command that matters.
> > >
> > > Having said that, this perhaps shows the nested structs are
> > > overkill. We could have
> > >
> > >
> > > var cmd Command
> > > if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> > > cmd = &BlockResizeArguments700{
> > > NodeName: node,
> > > Size: 1 * GiB
> > > }
> > > } else {
> > > cmd = &BlockResizeArguments520{
> > > Device: dev,
> > > Size: 1 * GiB
> > > }
> > > }
> >
> > The else block would be wrong in versions above 7.0.0 where
> > block_resize changed. There will be a need to know for a specific
> > Type if we are covered with latest qemu/qapi-go or not. Not yet
> > sure how to address that, likely we will need to keep the
> > information that something has been added/changed/removed per
> > version per Type in qapi-go...
>
> I put that in the "nice to have" category. No application can
> predict the future, and nor do they really need to try in
> general.
>
> If the application code was written when the newest QEMU was
> 7.1.0, and the above code is correct for QEMU <= 7.1.0, then
> that's good enough. If the BlockResizeArguments struct changed
> in a later QEMU version 8.0.0, that doesn't matter at the point
> the app code was written.I'm not thinking at runtime, I'm thinking at compile time. I update $myproject's qpai-go to 8.0.0 and get a warnings that some types would not work with 8.0.0 (e.g: because there is a new BlockResizeArguments800) > Much of the time the changes are back compatible, ie just adding > a new field, and so everything will still work fine if the app > carries on using BlockResizeArguments700, despite a new > BlockResizeArguments800 arriving with a new field. > > Only in the cases where a field was removed or changed in a > non-compatible manner would an app have problems, and QEMU will > happily report an error at runtime if the app sends something > incompatible. Yeah, runtime error is fine but if we can provide some extra checks at the time someone wants to move qapi-go from 7.2.0 to 8.0.0, that would be great. Cheers, Victor
signature.asc
Description: PGP signature
