[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu mon
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command |
Date: |
Thu, 9 Feb 2023 13:38:50 +0000 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote:
> > On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter for initiating a migration stream.
> > > This scheme has a significant flaw in it - double encoding of existing
> > > URIs to extract migration info.
> > >
> > > The current patch maps QAPI uri design onto well defined MigrateChannel
> > > struct. This modified QAPI helps in preventing multi-level uri
> > > encodings ('uri' parameter is kept for backward compatibility).
> > >
> > > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > > Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > > qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 129 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index c84fa10e86..79acfcfe4e 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -1449,12 +1449,108 @@
> > > ##
> > > { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > >
> > > +##
> > > +# @MigrateTransport:
> > > +#
> > > +# The supported communication transport mechanisms for migration
> > > +#
> > > +# @socket: Supported communication type between two devices for
> > > migration.
> > > +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > > +# 'fd' already
> > > +#
> > > +# @exec: Supported communication type to redirect migration stream into
> > > file.
> > > +#
> > > +# @rdma: Supported communication type to redirect rdma type migration
> > > stream.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'enum': 'MigrateTransport',
> > > + 'data': ['socket', 'exec', 'rdma'] }
> > > +
> > > +##
> > > +# @MigrateSocketAddr:
> > > +#
> > > +# To support different type of socket.
> > > +#
> > > +# @socket-type: Different type of socket connections.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateSocketAddr',
> > > + 'data': {'socket-type': 'SocketAddress' } }
> >
> > Here, you use 'socket-type',...
> >
> > > +
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > + 'data' : {'data': ['str'] } }
> >
> > Inconsistent on whether you have a space before :. Most of our qapi
> > files prefer the layout:
> >
> > 'key': 'value'
> >
> > that is, no space before, one space after. It doesn't affect
> > correctness, but a consistent visual style is worth striving for.
> >
> > > +
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > + 'data' : {'data': 'InetSocketAddress' } }
> >
> > ...while these branches supply everything else under 'data'. Also,
> > while you documented @socket-type above, you did not document @data in
> > either of these two types. [1]
> >
> > > +
> > > +##
> > > +# @MigrateAddress:
> > > +#
> > > +# The options available for communication transport mechanisms for
> > > migration
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'union' : 'MigrateAddress',
> > > + 'base' : { 'transport' : 'MigrateTransport'},
> > > + 'discriminator' : 'transport',
> > > + 'data' : {
> > > + 'socket' : 'MigrateSocketAddr',
> > > + 'exec' : 'MigrateExecAddr',
> > > + 'rdma': 'MigrateRdmaAddr' } }
> >
> > Another example of inconsistent spacing around :.
> >
> > I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
> > is that SocketAddress is itself a discriminated union, and Markus does
> > not yet have the QAPI generator wired up to support one union as a
> > branch of another larger union? It leads to extra nesting on the wire
> > [2]
>
> I don't know the backstory on this limitation. Is it something that
> is very difficult to resolve ? I think it is highly desirable to have
> 'socket': 'SocketAddress' here. It would be a shame to introduce this
> better migration API design and then have it complicated by a possibly
> short term limitation of QAPI.
So to understand this better I did this change on top of Het's
patch:
diff --git a/qapi/migration.json b/qapi/migration.json
index 79acfcfe4e..bbc3e66ad6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1467,18 +1467,6 @@
{ 'enum': 'MigrateTransport',
'data': ['socket', 'exec', 'rdma'] }
-##
-# @MigrateSocketAddr:
-#
-# To support different type of socket.
-#
-# @socket-type: Different type of socket connections.
-#
-# Since 8.0
-##
-{ 'struct': 'MigrateSocketAddr',
- 'data': {'socket-type': 'SocketAddress' } }
-
##
# @MigrateExecAddr:
#
@@ -1487,14 +1475,6 @@
{ 'struct': 'MigrateExecAddr',
'data' : {'data': ['str'] } }
-##
-# @MigrateRdmaAddr:
-#
-# Since 8.0
-##
-{ 'struct': 'MigrateRdmaAddr',
- 'data' : {'data': 'InetSocketAddress' } }
-
##
# @MigrateAddress:
#
@@ -1506,9 +1486,9 @@
'base' : { 'transport' : 'MigrateTransport'},
'discriminator' : 'transport',
'data' : {
- 'socket' : 'MigrateSocketAddr',
+ 'socket' : 'SocketAddress',
'exec' : 'MigrateExecAddr',
- 'rdma': 'MigrateRdmaAddr' } }
+ 'rdma': 'InetSocketAddress' } }
##
# @MigrateChannelType:
That results in a build error
/home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from
../qapi/qapi-schema.json:79:
../qapi/migration.json: In union 'MigrateAddress':
../qapi/migration.json:1505: branch 'socket' cannot use union type
'SocketAddress'
To understand what the limitation of QAPI generation is, I blindly
disabled this check
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cd8661125c..cb5c0385bd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -653,7 +653,7 @@ def check(self, schema, seen):
"branch '%s' is not a value of %s"
% (v.name, self.tag_member.type.describe()))
if (not isinstance(v.type, QAPISchemaObjectType)
- or v.type.variants):
+ or v.type.variants) and False:
raise QAPISemError(
self.info,
"%s cannot use %s"
@@ -664,7 +664,8 @@ def check_clash(self, info, seen):
for v in self.variants:
# Reset seen map for each variant, since qapi names from one
# branch do not affect another branch
- v.type.check_clash(info, dict(seen))
+ #v.type.check_clash(info, dict(seen))
+ pass
class QAPISchemaMember:
After doing that, the QAPI code generated handled the union-inside-union
case without any problems that I can see. The generated code looks sane
and it compiles correctly.
So is this actually just a case of overly strict input validation in
the QAPI schema parser ? If so, what's the correct way to adapt the
checks to permit this usage.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
- Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow, (continued)
[PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/08
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Eric Blake, 2023/02/08
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Daniel P . Berrangé, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command,
Daniel P . Berrangé <=
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/10
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Markus Armbruster, 2023/02/10
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Markus Armbruster, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/10
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Daniel P . Berrangé, 2023/02/09
QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command), Markus Armbruster, 2023/02/10
Re: QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command), Het Gala, 2023/02/10