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

From: John Snow <jsnow@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: Willian Rampazzo <wrampazz@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Wainer dos Santos Moschetta <wainersm@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	"Niteesh G . S ." <niteesh.gs@gmail.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Cleber Rosa <crosa@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v3 06/25] python/aqmp: add runstate state machine to AsyncProtocol
Date: Thu, 5 Aug 2021 17:30:16 -0400	[thread overview]
Message-ID: <CAFn=p-arW63EzrcTo=8Ufcx7ppyBKmBR21F-AjVqr2f_BXL8cQ@mail.gmail.com> (raw)
In-Reply-To: <20210805210241.ey36xvb57clvl3r5@redhat.com>

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

On Thu, Aug 5, 2021 at 5:02 PM Eric Blake <eblake@redhat.com> wrote:

> 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?
>
>
No, that one is a mistake. Comment reflow mishap.

> > +        # 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?
>
>
In this case yes, because you can't decorate a property in Python. So for
the sake of leaving it greppable, especially when those decorators are just
a "notation only" thing anyway, I figured I'd stuff it behind a comment.

> But that's minor enough that I trust you to handle it as you see fit.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>
Thanks!

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

[-- Attachment #2: Type: text/html, Size: 6916 bytes --]