Re: [PATCH v3 06/25] python/aqmp: add runstate state machine to AsyncProtocol

[Eric Blake]

On Tue, Aug 03, 2021 at 02:29:22PM -0400, John Snow wrote:
> This serves a few purposes:
> 
> 1. Protect interfaces when it's not safe to call them (via @require)
> 
> 2. Add an interface by which an async client can determine if the state
> has changed, for the purposes of connection management.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/__init__.py |   6 +-
>  python/qemu/aqmp/protocol.py | 159 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 160 insertions(+), 5 deletions(-)
> 

> +++ b/python/qemu/aqmp/protocol.py
> +
> +F = TypeVar('F', bound=Callable[..., Any])  # pylint: disable=invalid-name
> +
> +
> +# Don't Panic.
> +def require(required_state: Runstate) -> Callable[[F], F]:

Technically, the definition of F is integral to the definition of
require, so I might have put the 'Don't Panic' note a bit sooner.  And
it took me a while before I noticed...

> +    """
> +    Decorator: protect a method so it can only be run in a certain 
> `Runstate`.
> +
> +    :param required_state: The `Runstate` required to invoke this method.
> +    :raise StateError: When the required `Runstate` is not met.
> +    """
> +    def _decorator(func: F) -> F:
> +        # _decorator is the decorator that is built by calling the
> +        # require() decorator factory; e.g.:
> +        #
> +        # @require(Runstate.IDLE) def # foo(): ...

Is that second # intentional?

> +        # will replace 'foo' with the result of '_decorator(foo)'.
> +
> +        @wraps(func)
> +        def _wrapper(proto: 'AsyncProtocol[Any]',
> +                     *args: Any, **kwargs: Any) -> Any:
> +            # _wrapper is the function that gets executed prior to the
> +            # decorated method.
> +
> +            name = type(proto).__name__
> +
> +            if proto.runstate != required_state:
> +                if proto.runstate == Runstate.CONNECTING:
> +                    emsg = f"{name} is currently connecting."
> +                elif proto.runstate == Runstate.DISCONNECTING:
> +                    emsg = (f"{name} is disconnecting."
> +                            " Call disconnect() to return to IDLE state.")
> +                elif proto.runstate == Runstate.RUNNING:
> +                    emsg = f"{name} is already connected and running."
> +                elif proto.runstate == Runstate.IDLE:
> +                    emsg = f"{name} is disconnected and idle."
> +                else:
> +                    assert False
> +                raise StateError(emsg, proto.runstate, required_state)
> +            # No StateError, so call the wrapped method.
> +            return func(proto, *args, **kwargs)
> +
> +        # Return the decorated method;
> +        # Transforming Func to Decorated[Func].
> +        return cast(F, _wrapper)
> +
> +    # Return the decorator instance from the decorator factory. Phew!
> +    return _decorator

...that it paired with this comment, explaining what looks like a
monstrosity, but in reality is a fairly typical and boilerplate
implementation of a function decorator (not that you write one every
day - the whole point of the decorator is to have a one-word
abstraction already implemented so you DON'T have to reimplement the
decoration yourself ;).

> +
> +
>  class AsyncProtocol(Generic[T]):
>      """
>      AsyncProtocol implements a generic async message-based protocol.
> @@ -118,7 +202,24 @@ def __init__(self) -> None:
>          #: exit.
>          self._dc_task: Optional[asyncio.Future[None]] = None
>  
> +        self._runstate = Runstate.IDLE
> +        self._runstate_changed: Optional[asyncio.Event] = None
> +
> +    @property  # @upper_half

Is it intentional to leave the @upper_half decorator commented out?

But that's minor enough that I trust you to handle it as you see fit.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org