[ovs-dev,v12,2/6] python: Add option for JSON output to unixctl classes and appctl.py.

From: Jakob Meng <code@jakobmeng.de>

This patch introduces support for different output formats to Python
Unixctl* classes and appctl.py, similar to what the previous commit did
for ovs-appctl.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 NEWS                         |  3 +++
 python/ovs/unixctl/client.py |  5 ++--
 python/ovs/unixctl/server.py | 52 +++++++++++++++++++++++++++++++-----
 python/ovs/util.py           |  8 ++++++
 tests/appctl.py              | 38 +++++++++++++++++++++-----
 tests/unixctl-py.at          |  3 +++
 6 files changed, 93 insertions(+), 16 deletions(-)

On 5/16/24 17:41, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
> 
> This patch introduces support for different output formats to Python
> Unixctl* classes and appctl.py, similar to what the previous commit did
> for ovs-appctl.
> In particular, tests/appctl.py gains a global option '-f,--format'
> which allows users to request JSON instead of plain-text for humans.
> 
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>
> ---
>  NEWS                         |  3 +++
>  python/ovs/unixctl/client.py |  5 ++--
>  python/ovs/unixctl/server.py | 52 +++++++++++++++++++++++++++++++-----
>  python/ovs/util.py           |  8 ++++++
>  tests/appctl.py              | 38 +++++++++++++++++++++-----
>  tests/unixctl-py.at          |  3 +++
>  6 files changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 3c52e5ec1..7076939c5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -3,6 +3,9 @@ Post-v3.3.0
>     - ovs-appctl:
>       * Added new option [-f|--format] to choose the output format, e.g. 'json'
>         or 'text' (by default).
> +   - Python:
> +     * Added support for different output formats like 'json' to appctl.py and

appctl.py is a test utility, it shouldn't be named in NEWS.

Note: we gained the 'Python' news section with another recent commit.

> +       Python's unixctl classes.
>     - Userspace datapath:
>       * Conntrack now supports 'random' flag for selecting ports in a range
>         while natting and 'persistent' flag for selection of the IP address
> diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
> index 8283f99bb..8a6fcb1b9 100644
> --- a/python/ovs/unixctl/client.py
> +++ b/python/ovs/unixctl/client.py
> @@ -14,6 +14,7 @@
>  
>  import os
>  
> +import ovs.json
>  import ovs.jsonrpc
>  import ovs.stream
>  import ovs.util
> @@ -41,10 +42,10 @@ class UnixctlClient(object):
>              return error, None, None
>  
>          if reply.error is not None:
> -            return 0, str(reply.error), None
> +            return 0, reply.error, None
>          else:
>              assert reply.result is not None
> -            return 0, None, str(reply.result)
> +            return 0, None, reply.result
>  
>      def close(self):
>          self._conn.close()
> diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
> index d24a7092c..0665eb837 100644
> --- a/python/ovs/unixctl/server.py
> +++ b/python/ovs/unixctl/server.py
> @@ -12,6 +12,7 @@
>  # See the License for the specific language governing permissions and
>  # limitations under the License.
>  
> +import argparse
>  import copy
>  import errno
>  import os
> @@ -35,6 +36,7 @@ class UnixctlConnection(object):
>          assert isinstance(rpc, ovs.jsonrpc.Connection)
>          self._rpc = rpc
>          self._request_id = None
> +        self._fmt = ovs.util.OutputFormat.TEXT
>  
>      def run(self):
>          self._rpc.run()
> @@ -63,10 +65,29 @@ class UnixctlConnection(object):
>          return error
>  
>      def reply(self, body):
> -        self._reply_impl(True, body)
> +        assert body is None or isinstance(body, str)
> +
> +        if body is None:
> +            body = ""
> +
> +        if self._fmt == ovs.util.OutputFormat.JSON:
> +            body = {
> +                "reply-format": "plain",
> +                "reply": body
> +            }
> +
> +        return self._reply_impl_json(True, body)
> +
> +    def reply_json(self, body):
> +        self._reply_impl_json(True, body)
>  
>      def reply_error(self, body):
> -        self._reply_impl(False, body)
> +        assert body is None or isinstance(body, str)
> +
> +        if body is None:
> +            body = ""
> +
> +        return self._reply_impl_json(False, body)
>  
>      # Called only by unixctl classes.
>      def _close(self):
> @@ -78,15 +99,11 @@ class UnixctlConnection(object):
>          if not self._rpc.get_backlog():
>              self._rpc.recv_wait(poller)
>  
> -    def _reply_impl(self, success, body):
> +    def _reply_impl_json(self, success, body):
>          assert isinstance(success, bool)
> -        assert body is None or isinstance(body, str)
>  
>          assert self._request_id is not None
>  
> -        if body is None:
> -            body = ""
> -
>          if success:
>              reply = Message.create_reply(body, self._request_id)
>          else:
> @@ -133,6 +150,24 @@ def _unixctl_version(conn, unused_argv, version):
>      conn.reply(version)
>  
>  
> +def _unixctl_set_options(conn, argv, unused_aux):
> +    assert isinstance(conn, UnixctlConnection)
> +
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("--format", default="text",
> +                        choices=[fmt.name.lower()
> +                                 for fmt in ovs.util.OutputFormat])
> +
> +    try:
> +        args = parser.parse_args(args=argv)
> +    except argparse.ArgumentError as e:
> +        conn.reply_error(str(e))
> +        return
> +
> +    conn._fmt = ovs.util.OutputFormat[args.format.upper()]
> +    conn.reply(None)
> +
> +
>  class UnixctlServer(object):
>      def __init__(self, listener):
>          assert isinstance(listener, ovs.stream.PassiveStream)
> @@ -207,4 +242,7 @@ class UnixctlServer(object):
>          ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
>                                       version)
>  
> +        ovs.unixctl.command_register("set-options", "[--format text|json]", 0,

We shoudl require at least one argument.

> +                                     2, _unixctl_set_options, None)
> +
>          return 0, UnixctlServer(listener)
> diff --git a/python/ovs/util.py b/python/ovs/util.py
> index 3dba022f8..272ca683d 100644
> --- a/python/ovs/util.py
> +++ b/python/ovs/util.py
> @@ -15,11 +15,19 @@
>  import os
>  import os.path
>  import sys
> +import enum
>  
>  PROGRAM_NAME = os.path.basename(sys.argv[0])
>  EOF = -1
>  
>  
> +@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
> +
> +

Same as for C code in previous versions, this should be part of unixctl module.
And the names should reflect that.

>  def abs_file_name(dir_, file_name):
>      """If 'file_name' starts with '/', returns a copy of 'file_name'.
>      Otherwise, returns an absolute path to 'file_name' considering it relative
> diff --git a/tests/appctl.py b/tests/appctl.py
> index e5cc28138..cf3ea3642 100644
> --- a/tests/appctl.py
> +++ b/tests/appctl.py
> @@ -37,6 +37,18 @@ def connect_to_target(target):
>      return client
>  
>  
> +def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
> +    if fmt == ovs.util.OutputFormat.TEXT:
> +        body = str(reply)
> +
> +        if body and not body.endswith("\n"):
> +            body += "\n"
> +
> +        return body
> +    else:
> +        return ovs.json.to_string(reply)

Same as for C version, the line break should be added to JSON
output as well.

> +
> +
>  def main():
>      parser = argparse.ArgumentParser(description="Python Implementation of"
>                                       " ovs-appctl.")
> @@ -49,30 +61,42 @@ 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)
>  
>      ovs.vlog.Vlog.init()
>      target = args.target
> +    format = ovs.util.OutputFormat[args.format.upper()]
>      client = connect_to_target(target)
> +
> +    if format != ovs.util.OutputFormat.TEXT:
> +        err_no, error, _ = client.transact(
> +            "set-options", ["--format", args.format])
> +
> +        if err_no:
> +            ovs.util.ovs_fatal(err_no, "%s: transaction error" % target)
> +        elif error is not None:
> +            sys.stderr.write(reply_to_string(error))
> +            ovs.util.ovs_error(0, "%s: server returned an error" % target)
> +            sys.exit(2)
> +
>      err_no, error, result = client.transact(args.command, args.argv)
>      client.close()
>  
>      if err_no:
>          ovs.util.ovs_fatal(err_no, "%s: transaction error" % target)
>      elif error is not None:
> -        sys.stderr.write(error)
> -        if error and not error.endswith("\n"):
> -            sys.stderr.write("\n")
> -
> +        sys.stderr.write(reply_to_string(error))
>          ovs.util.ovs_error(0, "%s: server returned an error" % target)
>          sys.exit(2)
>      else:
>          assert result is not None
> -        sys.stdout.write(result)
> -        if result and not result.endswith("\n"):
> -            sys.stdout.write("\n")
> +        sys.stdout.write(reply_to_string(result, format))
>  
>  
>  if __name__ == '__main__':
> diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
> index 724006118..92f557b67 100644
> --- a/tests/unixctl-py.at
> +++ b/tests/unixctl-py.at
> @@ -100,6 +100,7 @@ The available commands are:
>    exit
>    help
>    log                     [[arg ...]]
> +  set-options             [[--format text|json]]
>    version
>    vlog/close
>    vlog/list
> @@ -112,6 +113,8 @@ AT_CHECK([PYAPPCTL_PY -t test-unixctl.py help], [0], [expout])
>  AT_CHECK([ovs-vsctl --version | sed 's/ovs-vsctl/test-unixctl.py/' | head -1 > expout])
>  AT_CHECK([APPCTL -t test-unixctl.py version], [0], [expout])
>  AT_CHECK([PYAPPCTL_PY -t test-unixctl.py version], [0], [expout])
> +AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json version], [0], [dnl
> +{"reply":"$(cat expout)","reply-format":"plain"}])
>  
>  AT_CHECK([APPCTL -t test-unixctl.py echo robot ninja], [0], [stdout])
>  AT_CHECK([cat stdout | sed -e "s/u'/'/g"], [0], [dnl