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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160323/77a43674/attachment.sig>