[Qemu-devel] [PATCH 0/3]

Sameeh Jubran posted 3 patches 6 years, 7 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
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(-)
[Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 7 months ago
From: Sameeh Jubran <sjubran@redhat.com>

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

 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


Re: [Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 7 months ago
Ping.

On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> From: Sameeh Jubran <sjubran@redhat.com>
>
> 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
>
>  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>.*
Re: [Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 6 months ago
Ping.

On Tue, Aug 22, 2017 at 2:18 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> Ping.
>
> On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>
>> From: Sameeh Jubran <sjubran@redhat.com>
>>
>> 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
>>
>>  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>.*
Re: [Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 5 months ago
Pong.

On Mon, Sep 4, 2017 at 4:48 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> Ping.
>
> On Tue, Aug 22, 2017 at 2:18 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>
>> Ping.
>>
>> On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>>
>>> From: Sameeh Jubran <sjubran@redhat.com>
>>>
>>> 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
>>>
>>>  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>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*
Re: [Qemu-devel] [PATCH 0/3]
Posted by Michael Roth 6 years, 5 months ago
Quoting Sameeh Jubran (2017-08-13 10:58:46)
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> 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
> 


Re: [Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 5 months ago
On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
wrote:

> Quoting Sameeh Jubran (2017-08-13 10:58:46)
> > From: Sameeh Jubran <sjubran@redhat.com>
> >
> > 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>.*
Re: [Qemu-devel] [PATCH 0/3]
Posted by Sameeh Jubran 6 years, 2 months ago
Ping.

On Fri, Oct 27, 2017 at 10:08 AM, Sameeh Jubran <sameeh@daynix.com> wrote:

>
>
> On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
> wrote:
>
>> Quoting Sameeh Jubran (2017-08-13 10:58:46)
>> > From: Sameeh Jubran <sjubran@redhat.com>
>> >
>> > 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>.*
Re: [Qemu-devel] [PATCH 0/3]
Posted by Michael Roth 6 years, 2 months ago
Quoting Sameeh Jubran (2018-01-22 08:24:29)
> Ping.

I think I would still prefer the alternative series I sent. Your
approach may be more correct/elegant but the asynchronous event-handling
nature of it makes it inherently more different to debug than the
boring synchronous loop in my series. There's also more work needed to
get this in shape and I honestly don't think it's worth the extra effort
it will take.

But if you feel strongly enough about it I wouldn't be opposed to a hybrid
of the 2 approaches that basically boils down to the following:

1) Base your code on top of my full series here:

     https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg06157.html

2) Drop the udev side of your code and only implement the
   win32 side of your code.

3) Modify your handle_serial_device_events() logic to not actually
   manage closing/opening the Channel, but to simply set a global flag,
   e.g. ga_state->channel_path_available appropriately based on
   DBT_DEVICEARRIVAL/DBT_DEVICEREMOVECOMPLETE. And since it's a
   win32-only optimization make sure it's just set to true permanently
   for !win32.

   Then use that flag as an optimization to the retry loop
   in run_agent() that was introduced in patch 4 of my series:
   
   static int run_agent(GAState *s)
   {
       int ret = EXIT_SUCCESS;
   
       s->force_exit = false;
   
       do {
           ret = run_agent_once(s);
           if (s->config->retry_path && !s->force_exit) {
               g_warning("agent stopped unexpectedly, restarting...");
               sleep(QGA_RETRY_INTERVAL);
           }
       } while (s->config->retry_path && !s->force_exit);
   
       return ret;
   }
   
   by having your code changes introduce something like this:

   static int run_agent(GAState *s)
   {
       int ret = EXIT_SUCCESS;
   
       s->force_exit = false;
   
       do {
           ret = run_agent_once(s);
           if (s->config->retry_path && !s->force_exit) {
               g_warning("agent stopped unexpectedly, restarting...");
               while (!ga_state->channel_path_available) {
                 g_warning("waiting for channel path...");
                 sleep(QGA_RETRY_INTERVAL);
               }
           }
       } while (s->config->retry_path && !s->force_exit);
   
       return ret;
   }

   It's still a loop but at least it's not trial-and-error based. If
   you want to get fancy with it though you can use something based
   around WaitForSingleObject or SetEvent to avoid gratuitous looping.

I think that would be much easier to make sense of, and it also lets us
handle this for POSIX without necessarily commiting to udev event loops
(though it also leaves that option open if we really wanted to).

It should also allow you to drop all your changes to the Channel code
since it's based around the changes I made to have Channel
implementations exit cleanly if their underlying device dies.

> 
> On Fri, Oct 27, 2017 at 10:08 AM, Sameeh Jubran <sameeh@daynix.com> wrote:
> 
> 
> 
>     On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
>     wrote:
> 
>         Quoting Sameeh Jubran (2017-08-13 10:58:46)
>         > From: Sameeh Jubran <sjubran@redhat.com>
>         >
>         > 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
>     Software Engineer @ Daynix.
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.