All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Stefan Hajnoczi <stefanha@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: armbru@redhat.com, crosa@redhat.com, qemu-devel@nongnu.org,
	ehabkost@redhat.com
Subject: Re: [PATCH RFC 0/7] RFC: Asynchronous QMP Draft
Date: Wed, 14 Apr 2021 07:38:28 +0100	[thread overview]
Message-ID: <YHaN5JPvjK2Wq6su@stefanha-x1.localdomain> (raw)
In-Reply-To: <20210413155553.2660523-1-jsnow@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 13229 bytes --]

Below are the API docs that I found helpful for understanding the big
picture.

The QMP.execute() API is nice.

Regarding QMP events, I can think of two approaches:
1. Callbacks
2. An async get_event(name=Optional[str]) -> object API
   (plus get_event_nowait(name=Optional[str]) -> object)

(There's probably a third approach using async iterators but it's
similar to get_event().)

Both approaches are useful. The first is good in larger asynchronous
applications that perform many tasks concurrently. The second is good
when there is just one specific thing to do, like waiting for a block
job to complete.

My general impression is that the public API is nice and usable but the
implementation is complex and risks discouraging other people from
hacking on the code. There are too many abstractions and while it's
highly structured, there is a cost to having all this infrastructure. I
think simplifying it would make it easier for others to understand and
contribute to the code.

Ideas: open code or inline simple things instead of defining
abstractions that only have 1 user, drop the pydantic models, drop
classes that just wrap things like Message and the exception hierarchy,
combine protocol and qmp_protocol.

Things that might be worth adding:
1. File descriptor passing support.
2. Introspection support to easily check if a command/feature is
   available. Users can do this manually by sending QMP commands and
   interpreting the response, but this may be common enough to warrant a
   friendly API.

Help on module qmp.qmp_protocol in qmp:

NAME
    qmp.qmp_protocol - QMP Client Implementation

DESCRIPTION
    This module provides the QMP class, which can be used to connect and
    send commands to a QMP server such as QEMU. The QMP class can be used to
    either connect to a listening server, or used to listen and accept an
    incoming connection from the server.

CLASSES
    qmp.error.AQMPError(builtins.Exception)
        ExecuteError
    qmp.protocol.AsyncProtocol(typing.Generic)
        QMP
    
    class ExecuteError(qmp.error.AQMPError)
     |  ExecuteError(sent: qmp.message.Message, received: qmp.message.Message, error: qmp.models.ErrorInfo)
     |  
     |  Execution statement returned failure.
     |  
     |  Method resolution order:
     |      ExecuteError
     |      qmp.error.AQMPError
     |      builtins.Exception
     |      builtins.BaseException
     |      builtins.object
     |  
     |  Methods defined here:
     |  
     |  __init__(self, sent: qmp.message.Message, received: qmp.message.Message, error: qmp.models.ErrorInfo)
     |      Initialize self.  See help(type(self)) for accurate signature.
     |  
     |  __str__(self) -> str
     |      Return str(self).
     |  
     |  ----------------------------------------------------------------------
     |  Data descriptors inherited from qmp.error.AQMPError:
     |  
     |  __weakref__
     |      list of weak references to the object (if defined)
     |  
     |  ----------------------------------------------------------------------
     |  Static methods inherited from builtins.Exception:
     |  
     |  __new__(*args, **kwargs) from builtins.type
     |      Create and return a new object.  See help(type) for accurate signature.
     |  
     |  ----------------------------------------------------------------------
     |  Methods inherited from builtins.BaseException:
     |  
     |  __delattr__(self, name, /)
     |      Implement delattr(self, name).
     |  
     |  __getattribute__(self, name, /)
     |      Return getattr(self, name).
     |  
     |  __reduce__(...)
     |      Helper for pickle.
     |  
     |  __repr__(self, /)
     |      Return repr(self).
     |  
     |  __setattr__(self, name, value, /)
     |      Implement setattr(self, name, value).
     |  
     |  __setstate__(...)
     |  
     |  with_traceback(...)
     |      Exception.with_traceback(tb) --
     |      set self.__traceback__ to tb and return self.
     |  
     |  ----------------------------------------------------------------------
     |  Data descriptors inherited from builtins.BaseException:
     |  
     |  __cause__
     |      exception cause
     |  
     |  __context__
     |      exception context
     |  
     |  __dict__
     |  
     |  __suppress_context__
     |  
     |  __traceback__
     |  
     |  args
    
    class QMP(qmp.protocol.AsyncProtocol)
     |  QMP(name: Optional[str] = None) -> None
     |  
     |  Implements a QMP connection to/from the server.
     |  
     |  Basic usage looks like this::
     |  
     |    qmp = QMP('my_virtual_machine_name')
     |    await qmp.connect(('127.0.0.1', 1234))
     |    ...
     |    res = await qmp.execute('block-query')
     |    ...
     |    await qmp.disconnect()
     |  
     |  :param name: Optional nickname for the connection, used for logging.
     |  
     |  Method resolution order:
     |      QMP
     |      qmp.protocol.AsyncProtocol
     |      typing.Generic
     |      builtins.object
     |  
     |  Methods defined here:
     |  
     |  __init__(self, name: Optional[str] = None) -> None
     |      Initialize self.  See help(type(self)) for accurate signature.
     |  
     |  async execute(self, cmd: str, arguments: Optional[Mapping[str, object]] = None, oob: bool = False) -> object
     |      Execute a QMP command and return the response.
     |      
     |      :param cmd: QMP command name.
     |      :param arguments: Arguments (if any). Must be JSON-serializable.
     |      :param oob: If true, execute "out of band".
     |      
     |      :raise: ExecuteError if the server returns an error response.
     |      :raise: DisconnectedError if the connection was terminated early.
     |      
     |      :return: Execution response from the server. The type of object depends
     |               on the command that was issued, though most return a dict.
     |  
     |  async execute_msg(self, msg: qmp.message.Message) -> object
     |      Execute a QMP message and return the response.
     |      
     |      :param msg: The QMP `Message` to execute.
     |      :raises: ValueError if the QMP `Message` does not have either the
     |               'execute' or 'exec-oob' fields set.
     |      :raises: ExecuteError if the server returns an error response.
     |      :raises: DisconnectedError if the connection was terminated early.
     |      
     |      :return: Execution response from the server. The type of object depends
     |               on the command that was issued, though most return a dict.
     |  
     |  on_event(self, func: Callable[[ForwardRef('QMP'), qmp.message.Message], Awaitable[NoneType]]) -> Callable[[ForwardRef('QMP'), qmp.message.Message], Awaitable[NoneType]]
     |      FIXME: Quick hack: decorator to register event handlers.
     |      
     |      Use it like this::
     |      
     |        @qmp.on_event
     |        async def my_event_handler(qmp, event: Message) -> None:
     |          print(f"Received event: {event['event']}")
     |      
     |      RFC: What kind of event handler would be the most useful in
     |      practical terms? In tests, we are usually waiting for an
     |      event with some criteria to occur; maybe it would be useful
     |      to allow "coroutine" style functions where we can block
     |      until a certain event shows up?
     |  
     |  ----------------------------------------------------------------------
     |  Class methods defined here:
     |  
     |  make_execute_msg(cmd: str, arguments: Optional[Mapping[str, object]] = None, oob: bool = False) -> qmp.message.Message from builtins.type
     |      Create an executable message to be sent by `execute_msg` later.
     |      
     |      :param cmd: QMP command name.
     |      :param arguments: Arguments (if any). Must be JSON-serializable.
     |      :param oob: If true, execute "out of band".
     |      
     |      :return: An executable QMP message.
     |  
     |  ----------------------------------------------------------------------
     |  Data and other attributes defined here:
     |  
     |  __orig_bases__ = (qmp.protocol.AsyncProtocol[qmp.message.Message],)
     |  
     |  __parameters__ = ()
     |  
     |  logger = <Logger qmp.qmp_protocol (WARNING)>
     |  
     |  ----------------------------------------------------------------------
     |  Methods inherited from qmp.protocol.AsyncProtocol:
     |  
     |  async accept(self, address: Union[str, Tuple[str, int]], ssl: Optional[ssl.SSLContext] = None) -> None
     |      Accept a connection and begin processing message queues.
     |      
     |      :param address: Address to connect to;
     |                      UNIX socket path or TCP address/port.
     |      :param ssl:     SSL context to use, if any.
     |      
     |      :raise: `StateError`   (loop is running or disconnecting.)
     |      :raise: `ConnectError` (Connection was not successful.)
     |  
     |  async connect(self, address: Union[str, Tuple[str, int]], ssl: Optional[ssl.SSLContext] = None) -> None
     |      Connect to the server and begin processing message queues.
     |      
     |      :param address: Address to connect to;
     |                      UNIX socket path or TCP address/port.
     |      :param ssl:     SSL context to use, if any.
     |      
     |      :raise: `StateError`   (loop is running or disconnecting.)
     |      :raise: `ConnectError` (Connection was not successful.)
     |  
     |  async disconnect(self) -> None
     |      Disconnect and wait for all tasks to fully stop.
     |      
     |      If there were exceptions that caused the bottom half to terminate
     |      prematurely, they will be raised here.
     |      
     |      :raise: `Exception`      Arbitrary exceptions re-raised on behalf of
     |                               the bottom half.
     |      :raise: `MultiException` Iterable Exception used to multiplex multiple
     |                               exceptions when multiple tasks failed.
     |  
     |  ----------------------------------------------------------------------
     |  Readonly properties inherited from qmp.protocol.AsyncProtocol:
     |  
     |  disconnecting
     |      Return True when the loop is disconnecting, or disconnected.
     |  
     |  running
     |      Return True when the loop is currently connected and running.
     |  
     |  unconnected
     |      Return True when the loop is fully idle and quiesced.
     |      
     |      Returns True specifically when the loop is neither `running`
     |      nor `disconnecting`. A call to `disconnect()` is required
     |      to transition from `disconnecting` to `unconnected`.
     |  
     |  ----------------------------------------------------------------------
     |  Data descriptors inherited from qmp.protocol.AsyncProtocol:
     |  
     |  __dict__
     |      dictionary for instance variables (if defined)
     |  
     |  __weakref__
     |      list of weak references to the object (if defined)
     |  
     |  ----------------------------------------------------------------------
     |  Class methods inherited from typing.Generic:
     |  
     |  __class_getitem__(params) from builtins.type
     |  
     |  __init_subclass__(*args, **kwargs) from builtins.type
     |      This method is called when a class is subclassed.
     |      
     |      The default implementation does nothing. It may be
     |      overridden to extend subclasses.

DATA
    Awaitable = typing.Awaitable
        A generic version of collections.abc.Awaitable.
    
    Callable = typing.Callable
        Callable type; Callable[[int], str] is a function of (int) -> str.
        
        The subscription syntax must always be used with exactly two
        values: the argument list and the return type.  The argument list
        must be a list of types or ellipsis; the return type must be a single type.
        
        There is no syntax to indicate optional or keyword arguments,
        such function types are rarely used as callback types.
    
    Dict = typing.Dict
        A generic version of dict.
    
    List = typing.List
        A generic version of list.
    
    Mapping = typing.Mapping
        A generic version of collections.abc.Mapping.
    
    Optional = typing.Optional
        Optional type.
        
        Optional[X] is equivalent to Union[X, None].
    
    Tuple = typing.Tuple
        Tuple type; Tuple[X, Y] is the cross-product type of X and Y.
        
        Example: Tuple[T1, T2] is a tuple of two elements corresponding
        to type variables T1 and T2.  Tuple[int, float, str] is a tuple
        of an int, a float and a string.
        
        To specify a variable-length tuple of homogeneous type, use Tuple[T, ...].

FILE
    /tmp/foo/qmp/qmp_protocol.py



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  parent reply	other threads:[~2021-04-14  8:23 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13 15:55 [PATCH RFC 0/7] RFC: Asynchronous QMP Draft John Snow
2021-04-13 15:55 ` [PATCH RFC 1/7] util: asyncio-related helpers John Snow
2021-04-13 15:55 ` [PATCH RFC 2/7] error: Error classes and so on John Snow
2021-04-13 15:55 ` [PATCH RFC 3/7] protocol: generic async message-based protocol loop John Snow
2021-04-13 20:00   ` Stefan Hajnoczi
2021-04-14 17:29     ` John Snow
2021-04-15  9:14       ` Stefan Hajnoczi
2021-04-13 15:55 ` [PATCH RFC 4/7] message: add QMP Message type John Snow
2021-04-13 20:07   ` Stefan Hajnoczi
2021-04-14 17:39     ` John Snow
2021-04-13 15:55 ` [PATCH RFC 5/7] models: Add well-known QMP objects John Snow
2021-04-13 15:55 ` [PATCH RFC 6/7] qmp_protocol: add QMP client implementation John Snow
2021-04-14  5:44   ` Stefan Hajnoczi
2021-04-14 17:50     ` John Snow
2021-04-15  9:23       ` Stefan Hajnoczi
2021-04-13 15:55 ` [PATCH RFC 7/7] linter config John Snow
2021-04-14  6:38 ` Stefan Hajnoczi [this message]
2021-04-14 19:17   ` [PATCH RFC 0/7] RFC: Asynchronous QMP Draft John Snow
2021-04-15  9:52     ` Stefan Hajnoczi
2021-04-20  2:26       ` John Snow
2021-04-20  2:47         ` John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHaN5JPvjK2Wq6su@stefanha-x1.localdomain \
    --to=stefanha@redhat.com \
    --cc=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.