Libguestfs - Mar 2016 - Re: [PATCH v3 05/11] conn: Pretend to be a serial terminal, so sgabios doesn't hang.

On Tuesday 22 March 2016 19:05:24 Richard W.M. Jones
wrote:
> This tedious workaround avoids a 0.26 second pause when using sgabios
> (the Serial Graphics Adapter).  It's basically a workaround for buggy
> code in sgabios, but much easier than fixing the assembler.
> ---
>  src/conn-socket.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conn-socket.c b/src/conn-socket.c
> index 5b6b80e..13cb39b 100644
> --- a/src/conn-socket.c
> +++ b/src/conn-socket.c
> @@ -33,6 +33,8 @@
>  #include <assert.h>
>  #include <libintl.h>
>  
> +#include "ignore-value.h"
> +
>  #include "guestfs.h"
>  #include "guestfs-internal.h"
>  
> @@ -314,6 +316,9 @@ handle_log_message (guestfs_h *g,
>  {
>    CLEANUP_FREE char *buf = safe_malloc (g, BUFSIZ);
>    ssize_t n;
> +  const char dsr_request[] = "\033[6n";
> +  const char dsr_reply[] = "\033[24;80R";
> +  const char dsr_reply_padding[] =
"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b";
>  
>    /* Carried over from ancient proto.c code.  The comment there was:
>     *
> @@ -341,7 +346,32 @@ handle_log_message (guestfs_h *g,
>      return -1;
>    }
>  
> -  /* It's an actual log message, send it upwards. */
> +  /* It's an actual log message. */
> +
> +  /* SGABIOS tries to query the "serial console" for its size
using the
> +   * ISO/IEC 6429 Device Status Report (ESC [ 6 n).  If it doesn't
> +   * read anything back, then it unfortunately hangs for 0.26 seconds.
> +   * Therefore we detect this situation and send back a fake console
> +   * size.
> +   */
> +  if (memmem (buf, n, dsr_request, sizeof dsr_request - 1) != NULL) {
> +    /* Ignore any error from this write, as it's just an optimization.
> +     * We can't even be sure that console_sock is a socket or that
> +     * it's writable.
> +     */
> +    ignore_value (write (conn->console_sock, dsr_reply,
> +                         sizeof dsr_reply - 1));
> +    /* Additionally, because of a bug in sgabios, it will still pause
> +     * unless you write at least 14 bytes, so we have to pad the
> +     * reply.  We can't pad with NULs since sgabios's input
routine
> +     * ignores these, so we have to use some other safe padding
> +     * characters.  Backspace seems innocuous.
> +     */
> +    ignore_value (write (conn->console_sock, dsr_reply_padding,
> +                         sizeof dsr_reply_padding - 1));
> +  }
> +
> +  /* Send it upwards. */
>    guestfs_int_log_message_callback (g, buf, n);
>  
>    return 1;
If I read this correctly, the newly added memmem would be executed for
every read data from the daemon socket:
  read_data -> handle_log_message -> memmem

Wouldn't be better to limit it in accept_connection if possible? More
than the overhead of memmem, I'm worried that this check might
misdetect other kind of binary data coming from the daemon (e.g.
download APIs) with the sgabios output.

-- 
Pino Toscano

On Wed, Mar 23, 2016 at 10:39:43AM +0100, Pino Toscano
wrote:
> If I read this correctly, the newly added memmem would be executed for
> every read data from the daemon socket:
>   read_data -> handle_log_message -> memmem
I think only from the console socket (not the daemon socket, and hence
not download APIs).  However I will see if it's possible to limit this
to the accept phase anyway, since that seems like a good idea.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Wed, Mar 23, 2016 at 12:59:51PM +0000, Richard W.M. Jones
wrote:
> On Wed, Mar 23, 2016 at 10:39:43AM +0100, Pino Toscano wrote:
> > If I read this correctly, the newly added memmem would be executed for
> > every read data from the daemon socket:
> >   read_data -> handle_log_message -> memmem
> 
> I think only from the console socket (not the daemon socket, and hence
> not download APIs).  However I will see if it's possible to limit this
> to the accept phase anyway, since that seems like a good idea.
Not easily ...  It turns out that we accept the daemon connection very
early on, just after qemu starts up.  (This surprised me, but makes
sense given that virtio-serial is connectionless).

Do you agree that this won't affect downloads and other data paths
from libguestfs?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/