Re: [Qemu-devel] qemu-ga behavior on virtio-serial unplug

[mdroth]

On Wed, Jun 19, 2013 at 01:17:57PM +0200, Laszlo Ersek wrote:
> Hello Michael,
> 
> this is with reference to
> <https://bugzilla.redhat.com/show_bug.cgi?id=907733>.
> 
> Ever since the initial qemu-ga commit AFAICS an exception for
> virtio-serial has existed, when reading EOF from the channel.
> 
> For isa-serial, EOF results in the client connection being closed. I
> assume this exits the glib main loop somehow (otherwise qemu-ga would
> just sit there and do nothing, as no further connections are accepted I
> think).

I think it would actually do the latter, unfortunately. It's distinct
from virtio-serial handling in that we remove the GSource by return false
in the event handler (qga/main.c:channel_event_cb), but I think we'd
need to drop in a g_main_loop_quit() to actually get qemu-ga to exit in
that scenario.

This doesn't normally get triggered though, as isa-serial does not send
EOF when nothing is connected to the chardev backend, but instead just
blocks. Might till make sense to make qemu-ga exit in this case though
since it won't be doing anything useful and wrapper scripts would at
least have some indication that something is up.

> 
> For a unix domain socket, we can continue accepting new connections
> after reading EOF.
> 
> For virtio-serial, EOF means "no host-side process is connected". In
> this case we sleep a bit and go back to reading from the same fd (and
> this turns into a sleep loop until the next host-side process connects).
> 
> 
> Can we tell "virtio-serial port unplug" from "no host-side process"?
> Because in the former case qemu-ga should really close the channel (and
> maybe exit (*)), otherwise the unplug won't complete in the guest kernel.
> 
> According to Amit's comments in the RHBZ, at unplug a SIGIO is
> delivered, and a POLLHUP event is reported. However,
> 
> (a) I think the glib iochannel abstraction doesn't allow us to tell
> POLLHUP apart from reading EOF;

AFAICT we can actually access the POLLHUP event via the 'condition' param
that gets passed to the cb, but the issue is we also get POLLUP when
the chardev backend isn't open.

> 
> (b) delivery of an unhandled SIGIO seems to terminate the victim
> process. qemu-ga doesn't seem to either catch or block SIGIO, which is
> why I think a SIGIO signal is not sent in reality (maybe we should ask
> for it first?)
> 
> ... Actually I'm confused about this as well. The virtio-serial port
> *is* opened with O_ASYNC (and on Solaris, it is replaced with an
> I_SETSIG ioctl()). What's the reason for this? g_io_channel_unix_new()
> doesn't seem to list it as a requirement, and qemu-ga doesn't seem to
> handle SIGIO.

At some point I played around with trying to use SIGIO to handle channel
resets and whatnot (since we're also supposed to get one when someone
opens the chardev backend and causes VIRTIO_CONSOLE_PORT_OPEN to get
sent). I don't think I ever got it working, SIGIO doesn't seem to get
sent, so that O_ASYNC might just be a relic from that.

I tried installing a handler retested host-connect as well as hot
unplug and still don't seem to be getting the signal. Not sure if i'm
doing something wrong or if there's an issue with the guest driver.

I did notice something interesting though:

1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF
1371740628.395726: debug: cb: condition: 17, status: 2
1371740628.395760: debug: received EOF
1371740628.496035: debug: cb: condition: 17, status: 2
1371740628.496072: debug: received EOF
1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF

<host opened chardev backend>

1371740634.195524: debug: cb: condition: 1, status: 1
1371740634.195556: debug: read data, count: 25, data:
{'execute':'guest-ping'}

1371740634.195634: debug: process_event: called
1371740634.195660: debug: processing command
1371740634.196007: debug: sending data, count: 15

<virtio-serial unplugged>

1371740644.113346: debug: cb: condition: 16, status: 2
1371740644.113379: debug: received EOF
1371740644.213694: debug: cb: condition: 16, status: 2
1371740644.213725: debug: received EOF
1371740644.314041: debug: cb: condition: 16, status: 2
1371740644.314168: debug: received EOF

i.e. we got the POLLHUP if we read from an
unconnected-but-present port, and we *don't* get the POLLHUP
if the port has been unplugged.

And in none of these cases do the SIGIO seem to be sent.

Here's the debug stuff i added:

diff --git a/qga/main.c b/qga/main.c
index 0e04e73..7f9a628 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -140,6 +140,11 @@ static void quit_handler(int sig)
     }
 }
 
+static void sigio_handler(int sig)
+{
+    g_debug("got sigio: %d", sig);
+}
+
 #ifndef _WIN32
 static gboolean register_signal_handlers(void)
 {
@@ -158,6 +163,13 @@ static gboolean register_signal_handlers(void)
         g_error("error configuring signal handler: %s", strerror(errno));
     }
 
+    memset(&sigact, 0, sizeof(struct sigaction));
+    sigact.sa_handler = sigio_handler;
+    ret = sigaction(SIGIO, &sigact, NULL);
+    if (ret == -1) {
+        g_error("error configuring signal handler: %s", strerror(errno));
+    }
+
     return true;
 }
 
@@ -627,6 +639,7 @@ static gboolean channel_event_cb(GIOCondition condition, 
gpointer data)
     gsize count;
     GError *err = NULL;
     GIOStatus status = ga_channel_read(s->channel, buf, 
QGA_READ_COUNT_DEFAULT, &count);
+    g_debug("cb: condition: %d, status: %d", condition, status);
     if (err != NULL) {
         g_warning("error reading channel: %s", err->message);
         g_error_free(err);

> 
> In any case we'd need a way to tell "host side close" from "port unplug".
> 

Poking around a bit it seems that the SIGIO can be tied to a specific
kind of event we can extract via siginfo_t. Right now it looks like
virtio-serial is hard-coded to set POLL_OUT, but with some driver
changes we could maybe tie unplug to POLL_HUP or something.

So with some driver changes qemu-ga can be made to handle this
gracefully, but otherwise I don't have any good ideas.

The only that comes to mind is adding a 'quit' command to qemu-ga that
libvirt could call prior to unplug, and once we get around to integrating
qemu-ga into qmp qemu could issue it internally as part of the unplug.
This isn't consumable for other stuff uses virtio-serial though so I
think working SIGIO into doing what we want is probably the best
approach.

There's also taking advantage of the above behavior (EOF and no POLLHUP
means we were hot-unplugged) but based I what I've read that's not the
intended behavior.

> (*) Then, there's the question what to do after unplug. Should we keep
> reopening the same virtio-serial port (and sleeping a bit in-between)?
> Or exit and let udev / systemd restart qemu-ga on any new virtio-serial
> port, passing -p accordingly?

Event-based would be pretty spiffy, but that doesn't preclude us from
adding a "--wait-for-channel" type of option that plays a little more
nicely with a watchdog-style deployment.

> 
> Thanks
> Laszlo
>