[v2,6/6] xl: use xenconsole startup protocol

Commit Message

Wei Liu Aug. 5, 2016, 5:41 p.m. UTC

If user asks xl to automatically connect to console when creating a
guest, use the new startup protocol before trying to unpause domain so
that we don't lose any console output.

Signed-off-by: Wei Liu <[e-mail]>
---
v2: properly handle read(2) errors
---
 tools/libxl/xl_cmdimpl.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

Comments

Ian Jackson Aug. 8, 2016, 10:17 a.m. UTC | #1

Wei Liu writes ("[PATCH v2 6/6] xl: use xenconsole startup protocol"):
> If user asks xl to automatically connect to console when creating a
> guest, use the new startup protocol before trying to unpause domain so
> that we don't lose any console output.
...
>      if ( dom_info->console_autoconnect ) {
> +        if (libxl_pipe(ctx, notify_pipe)) {
> +            fprintf(stderr,
> +                    "Failed to create console notification pipe, errno %d\n",
> +                    errno);

Is it really necessary to print to stderr, when libxl_pipe has already
logged a message ?  That seems the compelling advantage of
libxl_pipe...

> +        else if (r == 1 && buf[0] != 0x00)
> +            fprintf(stderr, "Got unexpected response from xenconsole: %x\n",
> +                    buf[0]);

I would generally prefer %#x, but up to you.

Acked-by: Ian Jackson <[e-mail]>

Thanks,
Ian.

Wei Liu Aug. 8, 2016, 10:32 a.m. UTC | #2

On Mon, Aug 08, 2016 at 11:17:20AM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v2 6/6] xl: use xenconsole startup protocol"):
> > If user asks xl to automatically connect to console when creating a
> > guest, use the new startup protocol before trying to unpause domain so
> > that we don't lose any console output.
> ...
> >      if ( dom_info->console_autoconnect ) {
> > +        if (libxl_pipe(ctx, notify_pipe)) {
> > +            fprintf(stderr,
> > +                    "Failed to create console notification pipe, errno %d\n",
> > +                    errno);
> 
> Is it really necessary to print to stderr, when libxl_pipe has already
> logged a message ?  That seems the compelling advantage of
> libxl_pipe...
> 

OK. I can delete this fprintf.

> > +        else if (r == 1 && buf[0] != 0x00)
> > +            fprintf(stderr, "Got unexpected response from xenconsole: %x\n",
> > +                    buf[0]);
> 
> I would generally prefer %#x, but up to you.
> 

Done.

> Acked-by: Ian Jackson <[e-mail]>
> 

Thanks.

> Thanks,
> Ian.

Patch

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 180fc8d..92002dc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2728,6 +2728,7 @@  static void autoconnect_console(libxl_ctx *ctx_ignored,
                                 libxl_event *ev, void *priv)
 {
     uint32_t bldomid = ev->domid;
+    int notify_fd = *(int*)priv; /* write end of the notification pipe */
 
     libxl_event_free(ctx, ev);
 
@@ -2740,7 +2741,7 @@  static void autoconnect_console(libxl_ctx *ctx_ignored,
     postfork();
 
     sleep(1);
-    libxl_primary_console_exec(ctx, bldomid);
+    libxl_primary_console_exec(ctx, bldomid, notify_fd);
     /* Do not return. xl continued in child process */
     perror("xl: unable to exec console client");
     _exit(1);
@@ -2810,6 +2811,7 @@  static int create_domain(struct domain_create *dom_info)
     int restore_fd_to_close = -1;
     int send_back_fd = -1;
     const libxl_asyncprogress_how *autoconnect_console_how;
+    int notify_pipe[2] = { -1, -1 };
     struct save_file_header hdr;
     uint32_t domid_soft_reset = INVALID_DOMID;
 
@@ -2997,7 +2999,15 @@  start:
 
     libxl_asyncprogress_how autoconnect_console_how_buf;
     if ( dom_info->console_autoconnect ) {
+        if (libxl_pipe(ctx, notify_pipe)) {
+            fprintf(stderr,
+                    "Failed to create console notification pipe, errno %d\n",
+                    errno);
+            ret = ERROR_FAIL;
+            goto error_out;
+        }
         autoconnect_console_how_buf.callback = autoconnect_console;
+        autoconnect_console_how_buf.for_callback = &notify_pipe[1];
         autoconnect_console_how = &autoconnect_console_how_buf;
     }else{
         autoconnect_console_how = 0;
@@ -3047,6 +3057,33 @@  start:
         restore_fd_to_close = -1;
     }
 
+    if (autoconnect_console_how) {
+        char buf[1];
+        int r;
+
+        /* Try to get notification from xenconsole. Just move on if
+         * error occurs -- it's only minor annoyance if console
+         * doesn't show up.
+         */
+        do {
+            r = read(notify_pipe[0], buf, 1);
+        } while (r == -1 && errno == EINTR);
+
+        if (r == -1)
+            fprintf(stderr,
+                    "Failed to get notification from xenconsole: %s\n",
+                    strerror(errno));
+        else if (r == 0)
+            fprintf(stderr, "Got EOF from xenconsole notification fd\n");
+        else if (r == 1 && buf[0] != 0x00)
+            fprintf(stderr, "Got unexpected response from xenconsole: %x\n",
+                    buf[0]);
+
+        close(notify_pipe[0]);
+        close(notify_pipe[1]);
+        notify_pipe[0] = notify_pipe[1] = -1;
+    }
+
     if (!paused)
         libxl_domain_unpause(ctx, domid);
 
@@ -3754,9 +3791,9 @@  int main_console(int argc, char **argv)
 
     domid = find_domain(argv[optind]);
     if (!type)
-        libxl_primary_console_exec(ctx, domid);
+        libxl_primary_console_exec(ctx, domid, -1);
     else
-        libxl_console_exec(ctx, domid, num, type);
+        libxl_console_exec(ctx, domid, num, type, -1);
     fprintf(stderr, "Unable to attach console\n");
     return EXIT_FAILURE;
 }