Re: [Qemu-devel] [PATCH 0/3]

[Michael Roth]

Quoting Sameeh Jubran (2017-08-13 10:58:46)
> From: Sameeh Jubran <address@hidden>
> 
> This series fixes qemu-ga's behaviour upon facing a missing serial/serial
> driver by listening to the serial device's events.
> 
> For more info on why this series is needed checkout the commit message
> of the third patch and the following bugzilla: 
> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
> 
> Sameeh Jubran (3):
>   qga: Channel: Add functions for checking serial status
>   qga: main: make qga config and socket activation global
>   qga: Prevent qemu-ga exit if serial doesn't exist

Hi Sameeh,

The event handling stuff is spiffy and could be useful for other use-cases
(e.g. cpu/mem hotplug events that can be consumed by management), but since
the actual bug here is somewhat of an edge case (we *could* just tell
people that installing the agent before virtio-serial drivers is a bug,
or that unplugging the agent's communication channel is a bad idea),
I'm not too comfortable with adding this much complexity unless there's
a stronger argument for it.

There's also a couple issues I had with this series as it stands, namely
the lack of a ./configure check for udev (which could cause build
breakage in some environments), and a lot of spillage of GAConfig into
qga/channel-*, which I think could be avoided.

I've sent an alternative series that I think we should consider as it
uses a much simpler mechanism to implement this support (basically
just periodically retrying the channel if it doesn't exist, or if it
disappears for whatever reason). I've tested it on Windows, but would
be good to confirm that it adequately addresses the use-case you were
looking at. Thanks!

> 
>  Makefile            |   4 +
>  qga/channel-posix.c |  54 ++++++++++
>  qga/channel-win32.c |  60 +++++++++++
>  qga/channel.h       |   9 ++
>  qga/main.c          | 284 
> ++++++++++++++++++++++++++++++++++++++++++++++------
>  qga/service-win32.h |   4 +
>  6 files changed, 385 insertions(+), 30 deletions(-)
> 
> -- 
> 2.9.4
>