On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
> From: Jakob Meng <c...@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 <c...@jakobmeng.de>
Note reviewed anything yet, but I get some flake8 errors when compiling your
series:
pends.py build-aux/soexpand.py build-aux/xml2nroff' && \
flake8 $src --select=H231,H232,H233,H238 && \
flake8 $src
--ignore=E121,E123,E125,E126,E127,E128,E129,E131,E203,E722,W503,W504,F811,D,H,I
&& \
touch flake8-check
python/ovs/unixctl/__init__.py:25:50: E261 at least two spaces before inline
comment
python/ovs/unixctl/__init__.py:30:9: E265 block comment should start with '# '
python/ovs/unixctl/__init__.py:73:68: E261 at least two spaces before inline
comment
python/ovs/unixctl/server.py:114:27: F821 undefined name 'params'
python/ovs/unixctl/server.py:118:19: E111 indentation is not a multiple of 4
python/ovs/unixctl/server.py:148:51: E261 at least two spaces before inline
comment
python/ovs/unixctl/server.py:239:1: E302 expected 2 blank lines, found 1
make[1]: *** [Makefile:6795: flake8-check] Error 1
FYI if you install flake8 on your system, re-run ./configure it will pick these
up as part of your compile process (and make sure you add --enable-Werror).
//Eelco
> ---
> python/ovs/unixctl/__init__.py | 38 +++++++++++++++---
> python/ovs/unixctl/client.py | 20 ++++++++--
> python/ovs/unixctl/server.py | 71 ++++++++++++++++++++++------------
> python/ovs/util.py | 9 +++++
> tests/appctl.py | 7 +++-
> 5 files changed, 112 insertions(+), 33 deletions(-)
>
> diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
> index 8ee312943..aa8969c61 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):
> + # TODO: Output format will be passed as 'fmt' to the command in later
> + # patch.
> + def __init__(self, usage, min_args, max_args, # fmt,
> + callback, aux):
> self.usage = usage
> self.min_args = min_args
> self.max_args = max_args
> + #self.fmt = fmt
> self.callback = callback
> self.aux = aux
>
> @@ -42,10 +46,12 @@ 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, fmt, 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
> @@ -60,11 +66,33 @@ def command_register(name, usage, min_args, max_args,
> callback, aux):
> assert isinstance(usage, str)
> assert isinstance(min_args, int)
> assert isinstance(max_args, int)
> + assert isinstance(fmt, ovs.util.OutputFormat)
> 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, # fmt,
> + callback, aux)
> +
> +
> +# TODO: 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 5f9b3e739..7228f8fd9 100644
> --- a/python/ovs/unixctl/server.py
> +++ b/python/ovs/unixctl/server.py
> @@ -104,30 +104,52 @@ 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(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))
> +
> + # TODO: 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, # fmt,
> + command.aux)
> +
> + except Exception as e:
> + self.reply_error(str(e))
>
>
> def _unixctl_version(conn, unused_argv, version):
> @@ -207,12 +229,13 @@ 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)
>
> -
> +# TODO: What is this? A copy of UnixctlClient from client.py?
> class UnixctlClient(object):
> def __init__(self, conn):
> assert isinstance(conn, ovs.jsonrpc.Connection)
> diff --git a/python/ovs/util.py b/python/ovs/util.py
> index 3dba022f8..6206f90d3 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
> +@enum.verify(enum.NAMED_FLAGS)
> +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:
> --
> 2.39.2
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
