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

[Michael Roth]

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 <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
>     Software Engineer @ Daynix.
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.