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.