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

[Sameeh Jubran]

Ping.

On Fri, Oct 27, 2017 at 10:08 AM, Sameeh Jubran <address@hidden> wrote:

>
>
> On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <address@hidden>
> wrote:
>
>> 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.
>>
> I can relate to your concerns, it is somehow an edge case but I think that
> this
> is the elegant way to handle it instead of just polling forever. This
> patch series
> is more related to Windows than Linux as this edge case is much more common
> on Windows since when the virtio-serial driver is installed sometimes
> usually
> it requires a post-installation reboot and when the system is up, qemu-ga
> runs before
> the virtio-serial driver is fully configured and it fails to load and then
> another reboot is needed.
>
>
>>
>> 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 think we can use the --retry with linux clients and use the device
> notifications
> API provided by Windows as it is supported since xp.
>
>>
>> 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!
>>
> I haven't tested it yet, but I think it might solve the issue. Your series
> is much
> simpler and less intrusive to the code but I don't think this is the right
> approach.
>
>>
>> >
>> >  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
>> >
>>
>>
>
>
> --
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*