[v1] Python patches

[PULL 04/19] python: backport 'protocol: adjust logging name when changing client name'

Posted by John Snow 4 months, 3 weeks ago

The client name is mutable, so the logging name should also change to
reflect it when it changes.

Signed-off-by: John Snow <jsnow@redhat.com>
cherry picked from commit python-qemu-qmp@e10b73c633ce138ba30bc8beccd2ab31989eaf3d
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 python/qemu/qmp/protocol.py | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
index 86e588881b7..ec4762c567b 100644
--- a/python/qemu/qmp/protocol.py
+++ b/python/qemu/qmp/protocol.py
@@ -217,10 +217,8 @@ class AsyncProtocol(Generic[T]):
     # -------------------------
 
     def __init__(self, name: Optional[str] = None) -> None:
-        #: The nickname for this connection, if any.
-        self.name: Optional[str] = name
-        if self.name is not None:
-            self.logger = self.logger.getChild(self.name)
+        self._name: Optional[str]
+        self.name = name
 
         # stream I/O
         self._reader: Optional[StreamReader] = None
@@ -257,6 +255,24 @@ def __repr__(self) -> str:
         tokens.append(f"runstate={self.runstate.name}")
         return f"<{cls_name} {' '.join(tokens)}>"
 
+    @property
+    def name(self) -> Optional[str]:
+        """
+        The nickname for this connection, if any.
+
+        This name is used for differentiating instances in debug output.
+        """
+        return self._name
+
+    @name.setter
+    def name(self, name: Optional[str]) -> None:
+        logger = logging.getLogger(__name__)
+        if name:
+            self.logger = logger.getChild(name)
+        else:
+            self.logger = logger
+        self._name = name
+
     @property  # @upper_half
     def runstate(self) -> Runstate:
         """The current `Runstate` of the connection."""
-- 
2.51.0

Re: [PULL 04/19] python: backport 'protocol: adjust logging name when changing client name'

Posted by Thomas Huth 3 months, 3 weeks ago

On 16/09/2025 18.23, John Snow wrote:
> The client name is mutable, so the logging name should also change to
> reflect it when it changes.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> cherry picked from commit python-qemu-qmp@e10b73c633ce138ba30bc8beccd2ab31989eaf3d
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   python/qemu/qmp/protocol.py | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)

  Hi John,

there is a regression when running "scripts/device-crash-test -q": It now 
prints:

ERROR: Negotiation failed: EOFError
ERROR: Failed to establish session: EOFError

all the time. See also for example:

https://gitlab.com/qemu-project/qemu/-/jobs/11715477453#L145

Bisecting the issue pointed me to this patch here. Could you please have a look?

  Thanks,
   Thomas


> diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
> index 86e588881b7..ec4762c567b 100644
> --- a/python/qemu/qmp/protocol.py
> +++ b/python/qemu/qmp/protocol.py
> @@ -217,10 +217,8 @@ class AsyncProtocol(Generic[T]):
>       # -------------------------
>   
>       def __init__(self, name: Optional[str] = None) -> None:
> -        #: The nickname for this connection, if any.
> -        self.name: Optional[str] = name
> -        if self.name is not None:
> -            self.logger = self.logger.getChild(self.name)
> +        self._name: Optional[str]
> +        self.name = name
>   
>           # stream I/O
>           self._reader: Optional[StreamReader] = None
> @@ -257,6 +255,24 @@ def __repr__(self) -> str:
>           tokens.append(f"runstate={self.runstate.name}")
>           return f"<{cls_name} {' '.join(tokens)}>"
>   
> +    @property
> +    def name(self) -> Optional[str]:
> +        """
> +        The nickname for this connection, if any.
> +
> +        This name is used for differentiating instances in debug output.
> +        """
> +        return self._name
> +
> +    @name.setter
> +    def name(self, name: Optional[str]) -> None:
> +        logger = logging.getLogger(__name__)
> +        if name:
> +            self.logger = logger.getChild(name)
> +        else:
> +            self.logger = logger
> +        self._name = name
> +
>       @property  # @upper_half
>       def runstate(self) -> Runstate:
>           """The current `Runstate` of the connection."""

Re: [PULL 04/19] python: backport 'protocol: adjust logging name when changing client name'

Posted by John Snow 3 months, 2 weeks ago

On Wed, Oct 15, 2025, 3:11 AM Thomas Huth <thuth@redhat.com> wrote:

> On 16/09/2025 18.23, John Snow wrote:
> > The client name is mutable, so the logging name should also change to
> > reflect it when it changes.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > cherry picked from commit
> python-qemu-qmp@e10b73c633ce138ba30bc8beccd2ab31989eaf3d
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   python/qemu/qmp/protocol.py | 24 ++++++++++++++++++++----
> >   1 file changed, 20 insertions(+), 4 deletions(-)
>
>   Hi John,
>
> there is a regression when running "scripts/device-crash-test -q": It now
> prints:
>
> ERROR: Negotiation failed: EOFError
> ERROR: Failed to establish session: EOFError
>
> all the time. See also for example:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/11715477453#L145
>
> Bisecting the issue pointed me to this patch here. Could you please have a
> look?
>
>   Thanks,
>    Thomas
>
>
> > diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
> > index 86e588881b7..ec4762c567b 100644
> > --- a/python/qemu/qmp/protocol.py
> > +++ b/python/qemu/qmp/protocol.py
> > @@ -217,10 +217,8 @@ class AsyncProtocol(Generic[T]):
> >       # -------------------------
> >
> >       def __init__(self, name: Optional[str] = None) -> None:
> > -        #: The nickname for this connection, if any.
> > -        self.name: Optional[str] = name
> > -        if self.name is not None:
> > -            self.logger = self.logger.getChild(self.name)
> > +        self._name: Optional[str]
> > +        self.name = name
> >
> >           # stream I/O
> >           self._reader: Optional[StreamReader] = None
> > @@ -257,6 +255,24 @@ def __repr__(self) -> str:
> >           tokens.append(f"runstate={self.runstate.name}")
> >           return f"<{cls_name} {' '.join(tokens)}>"
> >
> > +    @property
> > +    def name(self) -> Optional[str]:
> > +        """
> > +        The nickname for this connection, if any.
> > +
> > +        This name is used for differentiating instances in debug output.
> > +        """
> > +        return self._name
> > +
> > +    @name.setter
> > +    def name(self, name: Optional[str]) -> None:
> > +        logger = logging.getLogger(__name__)
> > +        if name:
> > +            self.logger = logger.getChild(name)
> > +        else:
> > +            self.logger = logger
> > +        self._name = name
> > +
> >       @property  # @upper_half
> >       def runstate(self) -> Runstate:
> >           """The current `Runstate` of the connection."""
>

... odd that it's this patch. I will have a look and issue a fix shortly.

Thanks for the report.