qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Robert Foley <robert.foley@linaro.org>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: fam@euphon.net, "Peter Puhov" <peter.puhov@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v1 06/14] tests/vm: Add logging of console to file.
Date: Mon, 10 Feb 2020 13:21:23 -0500	[thread overview]
Message-ID: <CAEyhzFv_W3cEFQYxpsrDDkbYawwGe2WFgi1SokLz9GO8xZ-w-A@mail.gmail.com> (raw)
In-Reply-To: <CAEyhzFs6+Lssj8a5QckmDaqi41E8_WemueSYuZrqtr=tVbYOjA@mail.gmail.com>

On Fri, 7 Feb 2020 at 17:20, Robert Foley <robert.foley@linaro.org> wrote:
>
> On Fri, 7 Feb 2020 at 12:12, Alex Bennée <alex.bennee@linaro.org> wrote:
> > > +
> > > +    def join(self, timeout=None):
> > > +        """Time to destroy the thread.
> > > +           Clear the event to stop the thread, and wait for
> > > +           it to complete."""
> > > +        self.alive.clear()
> > > +        threading.Thread.join(self, timeout)
> > > +        self.log_file.close()
> >
> > I'm note sure about this - introducing threading into Python seems very
> > un-pythonic. I wonder if the python experts have any view on a better
> > way to achieve what we want which I think is:
> >
> >   - a buffer that properly drains output from QEMU
> >   - which can optionally be serialised onto disk for logging
> >
> > What else are we trying to achieve here?
>
> I think that covers what we are trying to achieve here.
> The logging to file is a nice to have, but
> the draining of the output from QEMU is the main objective here.
> We will do a bit more research here to seek out a cleaner way to achieve this,
> but certainly we would also be interested if any python experts have a
> view on a cleaner approach.
>
We have a few more ideas on how to make this a bit cleaner.

We could create a new class "ConsoleSocket", based on asyncore.dispatcher.
The asyncore python library allows for overriding certain functionality of
an underlying socket.
We could override the handle_read() method in order to get a callback
when data is available to be read from the console.  This is much cleaner
than us simply waiting with a read() on the socket in a thread.
Note that even asyncore requires a thread around asyncore.loop(), but we would
only be responsible for start/stop of the thread and the thread body
is defined for us (literally just asyncore.loop()).
The thread starting/stopping would be completely transparent
to the users of this new ConsoleSocket.  The thread would be created on
initialization of ConsoleSocket, and destroyed upon close() of the socket.

As the console bytes get consumed we can dump them to an in memory stream,
rather than dumping them to a file right away.
We could perhaps use an io.StringIO text stream.  This stream will get
the bytes of the console and the test will consume the bytes directly
from this stream.
In addition, we could optionally dump the bytes to a file as well.

How does this sound?  Just looking for a bit of feedback before we
head in this direction.

Thanks & Regards,
-Rob


  reply	other threads:[~2020-02-10 18:22 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-05 21:29 [PATCH v1 00/14] tests/vm: Add support for aarch64 VMs Robert Foley
2020-02-05 21:29 ` [PATCH v1 01/14] tests/vm: use $(PYTHON) consistently Robert Foley
2020-02-07 11:42   ` Alex Bennée
2020-02-05 21:29 ` [PATCH v1 02/14] tests/vm: Debug mode shows ssh output Robert Foley
2020-02-05 21:29 ` [PATCH v1 03/14] tests/vm: increased max timeout for vm boot Robert Foley
2020-02-07 12:01   ` Alex Bennée
2020-02-05 21:29 ` [PATCH v1 04/14] tests/vm: give wait_ssh() option to wait for root Robert Foley
2020-02-07 12:01   ` Alex Bennée
2020-02-05 21:29 ` [PATCH v1 05/14] tests/vm: Added gen_cloud_init_iso() to basevm.py Robert Foley
2020-02-07 12:22   ` Alex Bennée
2020-02-05 21:29 ` [PATCH v1 06/14] tests/vm: Add logging of console to file Robert Foley
2020-02-07 17:12   ` Alex Bennée
2020-02-07 22:20     ` Robert Foley
2020-02-10 18:21       ` Robert Foley [this message]
2020-02-05 21:29 ` [PATCH v1 07/14] tests/vm: Add configuration to basevm.py Robert Foley
2020-02-05 21:29 ` [PATCH v1 08/14] tests/vm: Added configuration file support Robert Foley
2020-02-14 16:53   ` Alex Bennée
2020-02-14 18:00     ` Robert Foley
2020-02-05 21:29 ` [PATCH v1 09/14] tests/vm: add --boot-console switch Robert Foley
2020-02-05 21:29 ` [PATCH v1 10/14] tests/vm: Add ability to select QEMU from current build Robert Foley
2020-02-05 21:29 ` [PATCH v1 11/14] tests/vm: allow wait_ssh() to specify command Robert Foley
2020-02-05 21:29 ` [PATCH v1 12/14] tests/vm: Added a new script for ubuntu.aarch64 Robert Foley
2020-02-05 21:29 ` [PATCH v1 13/14] tests/vm: Added a new script for centos.aarch64 Robert Foley
2020-02-05 21:29 ` [PATCH v1 14/14] tests/vm: change scripts to use self._config Robert Foley
2020-02-07 16:50 ` [PATCH v1 00/14] tests/vm: Add support for aarch64 VMs Alex Bennée

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=CAEyhzFv_W3cEFQYxpsrDDkbYawwGe2WFgi1SokLz9GO8xZ-w-A@mail.gmail.com \
    --to=robert.foley@linaro.org \
    --cc=alex.bennee@linaro.org \
    --cc=fam@euphon.net \
    --cc=peter.puhov@linaro.org \
    --cc=philmd@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).