diff mbox series

[ovs-dev,v4,2/6] python: Add global option for JSON output to Python tools.

Message ID 20231116104120.25676-3-jmeng@redhat.com
State Changes Requested
Headers show
Series Add global option to output JSON from ovs-appctl cmds. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Jakob Meng Nov. 16, 2023, 10:41 a.m. UTC
From: Jakob Meng <code@jakobmeng.de>

This patch introduces support for different output formats to the
Python code, as did the previous commit for ovs-xxx tools like
'ovs-appctl --format json dpif/show'.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

This patch does not yet pass the choosen output format to commands.
Doing so requires changes to all command_register() calls and all
command callbacks. To improve readibility those changes have been
split out into a follow up patch. Respectively, whenever an output
format other than 'text' is choosen for tests/appctl.py, the script
will fail.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 NEWS                           |  2 +
 python/ovs/unixctl/__init__.py | 39 +++++++++++++++---
 python/ovs/unixctl/client.py   | 20 +++++++--
 python/ovs/unixctl/server.py   | 74 +++++++++++++++++++++++-----------
 python/ovs/util.py             |  9 +++++
 tests/appctl.py                |  7 +++-
 tests/unixctl-py.at            |  7 ++++
 7 files changed, 126 insertions(+), 32 deletions(-)
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 12a895629..fc77a1613 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,8 @@  Post-v3.2.0
        Reported names adjusted accordingly.
      * Added new option [-f|--format] to choose the output format, e.g. 'json'
        or 'text' (by default).
+   - Python:
+     * Added support for choosing the output format, e.g. 'json' or 'text'.
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
index 8ee312943..6d9bd5715 100644
--- a/python/ovs/unixctl/__init__.py
+++ b/python/ovs/unixctl/__init__.py
@@ -20,10 +20,14 @@  commands = {}
 
 
 class _UnixctlCommand(object):
-    def __init__(self, usage, min_args, max_args, callback, aux):
+    # FIXME: Output format will be passed as 'output_fmts' to the command in
+    #        later patch.
+    def __init__(self, usage, min_args, max_args,  # FIXME: output_fmts,
+                 callback, aux):
         self.usage = usage
         self.min_args = min_args
         self.max_args = max_args
+        # FIXME: self.output_fmts = output_fmts
         self.callback = callback
         self.aux = aux
 
@@ -42,10 +46,13 @@  def _unixctl_help(conn, unused_argv, unused_aux):
     conn.reply(reply)
 
 
-def command_register(name, usage, min_args, max_args, callback, aux):
+def command_register_fmt(name, usage, min_args, max_args, output_fmts,
+                         callback, aux):
     """ Registers a command with the given 'name' to be exposed by the
     UnixctlServer. 'usage' describes the arguments to the command; it is used
-    only for presentation to the user in "help" output.
+    only for presentation to the user in "help" output.  'output_fmts' is a
+    bitmap that defines what output formats a command supports, e.g.
+    OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
 
     'callback' is called when the command is received.  It is passed a
     UnixctlConnection object, the list of arguments as unicode strings, and
@@ -63,8 +70,30 @@  def command_register(name, usage, min_args, max_args, callback, aux):
     assert callable(callback)
 
     if name not in commands:
-        commands[name] = _UnixctlCommand(usage, min_args, max_args, callback,
-                                         aux)
+        commands[name] = _UnixctlCommand(usage, min_args, max_args,
+                                         # FIXME: output_fmts,
+                                         callback, aux)
+
+
+# FIXME: command_register() will be replaced with command_register_fmt() in a
+#        later patch of this series. It is is kept temporarily to reduce the
+#        amount of changes in this patch.
+def command_register(name, usage, min_args, max_args, callback, aux):
+    """ Registers a command with the given 'name' to be exposed by the
+    UnixctlServer. 'usage' describes the arguments to the command; it is used
+    only for presentation to the user in "help" output.
+
+    'callback' is called when the command is received.  It is passed a
+    UnixctlConnection object, the list of arguments as unicode strings, and
+    'aux'.  Normally 'callback' should reply by calling
+    UnixctlConnection.reply() or UnixctlConnection.reply_error() before it
+    returns, but if the command cannot be handled immediately, then it can
+    defer the reply until later.  A given connection can only process a single
+    request at a time, so a reply must be made eventually to avoid blocking
+    that connection."""
+
+    command_register_fmt(name, usage, min_args, max_args,
+                         ovs.util.OutputFormat.TEXT, callback, aux)
 
 
 def socket_name_from_target(target):
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..29a5f3df9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -26,13 +26,20 @@  class UnixctlClient(object):
         assert isinstance(conn, ovs.jsonrpc.Connection)
         self._conn = conn
 
-    def transact(self, command, argv):
+    def transact(self, command, argv, fmt):
         assert isinstance(command, str)
         assert isinstance(argv, list)
         for arg in argv:
             assert isinstance(arg, str)
+        assert isinstance(fmt, ovs.util.OutputFormat)
+        plain_rpc = (fmt == ovs.util.OutputFormat.TEXT)
+
+        if plain_rpc:
+            request = ovs.jsonrpc.Message.create_request(command, argv)
+        else:
+            request = ovs.jsonrpc.Message.create_request(
+                "execute/v1", [command, fmt.name.lower()] + argv)
 
-        request = ovs.jsonrpc.Message.create_request(command, argv)
         error, reply = self._conn.transact_block(request)
 
         if error:
@@ -41,7 +48,14 @@  class UnixctlClient(object):
             return error, None, None
 
         if reply.error is not None:
-            return 0, str(reply.error), None
+            plain_rpc_error = ('"%s" is not a valid command' %
+                               ovs.util.RPC_MARKER)
+            if str(reply.error).startswith(plain_rpc_error):
+                return 0, "JSON RPC reply indicates incompatible "\
+                          "server. Please upgrade server side to "\
+                          "newer version.", None
+            else:
+                return 0, str(reply.error), None
         else:
             assert reply.result is not None
             return 0, None, str(reply.result)
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index b9cb52fad..5e92bc570 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -104,30 +104,57 @@  class UnixctlConnection(object):
 
         self._request_id = request.id
 
-        error = None
-        params = request.params
-        method = request.method
-        command = ovs.unixctl.commands.get(method)
-        if command is None:
-            error = '"%s" is not a valid command' % method
-        elif len(params) < command.min_args:
-            error = '"%s" command requires at least %d arguments' \
-                    % (method, command.min_args)
-        elif len(params) > command.max_args:
-            error = '"%s" command takes at most %d arguments' \
-                    % (method, command.max_args)
-        else:
-            for param in params:
-                if not isinstance(param, str):
-                    error = '"%s" command has non-string argument' % method
-                    break
+        try:
+            plain_rpc = request.method != ovs.util.RPC_MARKER
+            args_offset = 0 if plain_rpc else 2
 
-            if error is None:
-                unicode_params = [str(p) for p in params]
-                command.callback(self, unicode_params, command.aux)
+            if not plain_rpc and len(request.params) < 2:
+                raise ValueError(
+                    "JSON-RPC API mismatch: Unexpected # of params: %d"
+                    % len(request.params))
 
-        if error:
-            self.reply_error(error)
+            for param in request.params:
+                if not isinstance(param, str):
+                    raise ValueError(
+                        "command has non-string argument: %s" % param)
+
+            method = request.method if plain_rpc else request.params[0]
+            if plain_rpc:
+                fmt = ovs.util.OutputFormat.TEXT
+            else:
+                fmt = ovs.util.OutputFormat[request.params[1].upper()]
+            params = request.params[args_offset:]
+
+            command = ovs.unixctl.commands.get(method)
+            if command is None:
+                raise ValueError('"%s" is not a valid command' % method)
+            elif len(params) < command.min_args:
+                raise ValueError(
+                    '"%s" command requires at least %d arguments'
+                    % (method, command.min_args))
+            elif len(params) > command.max_args:
+                raise ValueError(
+                    '"%s" command takes at most %d arguments'
+                    % (method, command.max_args))
+            # FIXME: Uncomment when command.output_fmts is available
+            # elif fmt not in command.output_fmts:
+            #    raise ValueError('"%s" command does not support output format'
+            #                     ' "%s" %d %d' % (method, fmt.name.lower(),
+            #                                      command.output_fmts, fmt))
+
+            # FIXME: Remove this check once output format will be passed to the
+            #        command handler below.
+            if fmt != ovs.util.OutputFormat.TEXT:
+                raise ValueError(
+                    'output format "%s" has not been implemented yet'
+                    % fmt.name.lower())
+
+            unicode_params = [str(p) for p in params]
+            command.callback(self, unicode_params,  # FIXME: fmt,
+                             command.aux)
+
+        except Exception as e:
+            self.reply_error(str(e))
 
 
 def _unixctl_version(conn, unused_argv, version):
@@ -207,7 +234,8 @@  class UnixctlServer(object):
                                % path)
             return error, None
 
-        ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
+        ovs.unixctl.command_register("version", "", 0, 0,
+                                     _unixctl_version,
                                      version)
 
         return 0, UnixctlServer(listener)
diff --git a/python/ovs/util.py b/python/ovs/util.py
index 3dba022f8..75ba4ee12 100644
--- a/python/ovs/util.py
+++ b/python/ovs/util.py
@@ -15,9 +15,18 @@ 
 import os
 import os.path
 import sys
+import enum
 
 PROGRAM_NAME = os.path.basename(sys.argv[0])
 EOF = -1
+RPC_MARKER = "execute/v1"
+
+
+@enum.unique
+# FIXME: Use @enum.verify(enum.NAMED_FLAGS) from Python 3.11 when available.
+class OutputFormat(enum.IntFlag):
+    TEXT = 1 << 0
+    JSON = 1 << 1
 
 
 def abs_file_name(dir_, file_name):
diff --git a/tests/appctl.py b/tests/appctl.py
index b85b364fa..42f96b2a3 100644
--- a/tests/appctl.py
+++ b/tests/appctl.py
@@ -49,6 +49,10 @@  def main():
                         help="Arguments to the command.")
     parser.add_argument("-T", "--timeout", metavar="SECS",
                         help="wait at most SECS seconds for a response")
+    parser.add_argument("-f", "--format", metavar="FMT",
+                        help="Output format.", default="text",
+                        choices=[fmt.name.lower()
+                                 for fmt in ovs.util.OutputFormat])
     args = parser.parse_args()
 
     signal_alarm(int(args.timeout) if args.timeout else None)
@@ -56,7 +60,8 @@  def main():
     ovs.vlog.Vlog.init()
     target = args.target
     client = connect_to_target(target)
-    err_no, error, result = client.transact(args.command, args.argv)
+    err_no, error, result = client.transact(
+        args.command, args.argv, ovs.util.OutputFormat[args.format.upper()])
     client.close()
 
     if err_no:
diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index 724006118..c149268af 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -65,6 +65,13 @@  AT_CHECK([head -1 stderr], [0], [dnl
 sed 's/ovs-appctl/appctl.py/' stderr > experr
 AT_CHECK([PYAPPCTL_PY bond/hash mac vlan basis extra], [2], [], [experr])
 
+AT_CHECK([APPCTL --format json dpif-netdev/pmd-rxq-show], [2], [], [stderr])
+AT_CHECK([head -1 stderr], [0], [dnl
+"dpif-netdev/pmd-rxq-show" command does not support output format "json" 1 2
+])
+sed 's/ovs-appctl/appctl.py/' stderr > experr
+AT_CHECK([PYAPPCTL_PY --format json dpif-netdev/pmd-rxq-show], [2], [], [experr])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP