Libguestfs - Feb 2017 - [PATCH 0/4] p2v: Send ping packets, document timeout problems.

Fix and/or document issues raised in this thread:

https://www.redhat.com/archives/libguestfs/2017-February/msg00010.html

Rich.

This is a simple macro with no dependencies.  Allow it to be used from
any program.
---
 daemon/daemon.h            | 12 ------------
 lib/guestfs-internal-all.h | 17 ++++++++++++++++-
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 3af5c0e..8097bfc 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -340,18 +340,6 @@ is_zero (const char *buffer, size_t size)
   return 1;
 }
 
-/* Helper for building up short lists of arguments.  Your code has to
- * define MAX_ARGS to a suitable value.
- */
-#define ADD_ARG(argv,i,v)                                               \
-  do {                                                                  \
-    if ((i) >= MAX_ARGS) {                                              \
-      fprintf (stderr, "%s: %d: internal error: exceeded MAX_ARGS (%zu)
when constructing the command line\n", __FILE__, __LINE__, (size_t)
MAX_ARGS); \
-      abort ();                                                         \
-    }                                                                   \
-    (argv)[(i)++] = (v);                                                \
-  } while (0)
-
 /* Helper for functions that need a root filesystem mounted.
  * NB. Cannot be used for FileIn functions.
  */
diff --git a/lib/guestfs-internal-all.h b/lib/guestfs-internal-all.h
index dbf9735..0c841ff 100644
--- a/lib/guestfs-internal-all.h
+++ b/lib/guestfs-internal-all.h
@@ -1,5 +1,5 @@
 /* libguestfs
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2017 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -58,6 +58,21 @@
 #define STRCASEPREFIX(a,b) (strncasecmp((a),(b),strlen((b))) == 0)
 #define STRSUFFIX(a,b) (strlen((a)) >= strlen((b)) &&
STREQ((a)+strlen((a))-strlen((b)),(b)))
 
+/* A simple (indeed, simplistic) way to build up short lists of
+ * arguments.  Your code must define MAX_ARGS to a suitable "larger
+ * than could ever be needed" value.  (If the value is exceeded then
+ * your code will abort).  For more complex needs, use something else
+ * more suitable.
+ */
+#define ADD_ARG(argv,i,v)                                               \
+  do {                                                                  \
+    if ((i) >= MAX_ARGS) {                                              \
+      fprintf (stderr, "%s: %d: internal error: exceeded MAX_ARGS (%zu)
when constructing the command line\n", __FILE__, __LINE__, (size_t)
MAX_ARGS); \
+      abort ();                                                         \
+    }                                                                   \
+    (argv)[(i)++] = (v);                                                \
+  } while (0)
+
 #ifndef SOCK_CLOEXEC
 #define SOCK_CLOEXEC 0
 #endif
-- 
2.10.2

No functional change.
---
 p2v/ssh.c | 134 +++++++++++++++++++++++++++++---------------------------------
 1 file changed, 62 insertions(+), 72 deletions(-)

diff --git a/p2v/ssh.c b/p2v/ssh.c
index 4c1d64c..817048c 100644
--- a/p2v/ssh.c
+++ b/p2v/ssh.c
@@ -306,6 +306,15 @@ cache_ssh_identity (struct config *config)
   return 0;
 }
 
+/* GCC complains about the argv array in the next function which it
+ * thinks might grow to an unbounded size.  Since we control
+ * extra_args, this is not in fact a problem.
+ */
+#if defined(__GNUC__) && GUESTFS_GCC_VERSION >= 40800 /* gcc >=
4.8.0 */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstack-usage="
+#endif
+
 /**
  * Start ssh subprocess with the standard arguments and possibly some
  * optional arguments.  Also handles authentication.
@@ -314,15 +323,18 @@ static mexp_h *
 start_ssh (unsigned spawn_flags, struct config *config,
            char **extra_args, int wait_prompt)
 {
-  size_t i, j, nr_args, count;
+  size_t i = 0;
+  const size_t MAX_ARGS +    64 + (extra_args != NULL ?
guestfs_int_count_strings (extra_args) : 0);
+  const char *argv[MAX_ARGS];
   char port_str[64];
   char connect_timeout_str[128];
-  CLEANUP_FREE /* [sic] */ const char **args = NULL;
   mexp_h *h;
   const int ovecsize = 12;
   int ovector[ovecsize];
   int saved_timeout;
   int using_password_auth;
+  size_t count;
 
   if (cache_ssh_identity (config) == -1)
     return NULL;
@@ -330,63 +342,48 @@ start_ssh (unsigned spawn_flags, struct config *config,
   /* Are we using password or identity authentication? */
   using_password_auth = config->identity_file == NULL;
 
-  /* Create the ssh argument array. */
-  nr_args = 0;
-  if (extra_args != NULL)
-    nr_args = guestfs_int_count_strings (extra_args);
-
-  if (using_password_auth)
-    nr_args += 13;
-  else
-    nr_args += 15;
-  args = malloc (sizeof (char *) * nr_args);
-  if (args == NULL)
-    error (EXIT_FAILURE, errno, "malloc");
-
-  j = 0;
-  args[j++] = "ssh";
-  args[j++] = "-p";             /* Port. */
+  ADD_ARG (argv, i, "ssh");
+  ADD_ARG (argv, i, "-p");      /* Port. */
   snprintf (port_str, sizeof port_str, "%d", config->port);
-  args[j++] = port_str;
-  args[j++] = "-l";             /* Username. */
-  args[j++] = config->username ? config->username : "root";
-  args[j++] = "-o";             /* Host key will always be novel. */
-  args[j++] = "StrictHostKeyChecking=no";
-  args[j++] = "-o";             /* ConnectTimeout */
+  ADD_ARG (argv, i, port_str);
+  ADD_ARG (argv, i, "-l");      /* Username. */
+  ADD_ARG (argv, i, config->username ? config->username :
"root");
+  ADD_ARG (argv, i, "-o");      /* Host key will always be novel. */
+  ADD_ARG (argv, i, "StrictHostKeyChecking=no");
+  ADD_ARG (argv, i, "-o");      /* ConnectTimeout */
   snprintf (connect_timeout_str, sizeof connect_timeout_str,
             "ConnectTimeout=%d", SSH_TIMEOUT);
-  args[j++] = connect_timeout_str;
+  ADD_ARG (argv, i, connect_timeout_str);
   if (using_password_auth) {
     /* Only use password authentication. */
-    args[j++] = "-o";
-    args[j++] =
"PreferredAuthentications=keyboard-interactive,password";
+    ADD_ARG (argv, i, "-o");
+    ADD_ARG (argv, i,
"PreferredAuthentications=keyboard-interactive,password");
   }
   else {
     /* Use identity file (private key). */
-    args[j++] = "-o";
-    args[j++] = "PreferredAuthentications=publickey";
-    args[j++] = "-i";
-    args[j++] = config->identity_file;
+    ADD_ARG (argv, i, "-o");
+    ADD_ARG (argv, i, "PreferredAuthentications=publickey");
+    ADD_ARG (argv, i, "-i");
+    ADD_ARG (argv, i, config->identity_file);
   }
   if (extra_args != NULL) {
-    for (i = 0; extra_args[i] != NULL; ++i)
-      args[j++] = extra_args[i];
+    for (size_t j = 0; extra_args[j] != NULL; ++j)
+      ADD_ARG (argv, i, extra_args[j]);
   }
-  args[j++] = config->server;   /* Conversion server. */
-  args[j++] = NULL;
-  assert (j == nr_args);
+  ADD_ARG (argv, i, config->server); /* Conversion server. */
+  ADD_ARG (argv, i, NULL);
 
 #if DEBUG_STDERR
   fputs ("ssh command: ", stderr);
-  for (i = 0; i < nr_args - 1; ++i) {
+  for (i = 0; argv[i] != NULL; ++i) {
     if (i > 0) fputc (' ', stderr);
-    fputs (args[i], stderr);
+    fputs (argv[i], stderr);
   }
   fputc ('\n', stderr);
 #endif
 
   /* Create the miniexpect handle. */
-  h = mexp_spawnvf (spawn_flags, "ssh", (char **) args);
+  h = mexp_spawnvf (spawn_flags, "ssh", (char **) argv);
   if (h == NULL) {
     set_ssh_internal_error ("ssh: mexp_spawnvf: %m");
     return NULL;
@@ -566,6 +563,10 @@ start_ssh (unsigned spawn_flags, struct config *config,
   return h;
 }
 
+#if defined(__GNUC__) && GUESTFS_GCC_VERSION >= 40800 /* gcc >=
4.8.0 */
+#pragma GCC diagnostic pop
+#endif
+
 /**
  * Upload a file to remote using L<scp(1)>.
  *
@@ -574,16 +575,16 @@ start_ssh (unsigned spawn_flags, struct config *config,
 int
 scp_file (struct config *config, const char *localfile, const char *remotefile)
 {
-  size_t j, nr_args;
+  size_t i = 0;
+  const size_t MAX_ARGS = 64;
+  const char *argv[MAX_ARGS];
   char port_str[64];
   char connect_timeout_str[128];
   CLEANUP_FREE char *remote = NULL;
-  CLEANUP_FREE /* [sic] */ const char **args = NULL;
   mexp_h *h;
   const int ovecsize = 12;
   int ovector[ovecsize];
   int using_password_auth;
-  size_t i;
 
   if (cache_ssh_identity (config) == -1)
     return -1;
@@ -591,58 +592,47 @@ scp_file (struct config *config, const char *localfile,
const char *remotefile)
   /* Are we using password or identity authentication? */
   using_password_auth = config->identity_file == NULL;
 
-  /* Create the scp argument array. */
-  if (using_password_auth)
-    nr_args = 12;
-  else
-    nr_args = 14;
-  args = malloc (sizeof (char *) * nr_args);
-  if (args == NULL)
-    error (EXIT_FAILURE, errno, "malloc");
-
-  j = 0;
-  args[j++] = "scp";
-  args[j++] = "-P";             /* Port. */
+  ADD_ARG (argv, i, "scp");
+  ADD_ARG (argv, i, "-P");      /* Port. */
   snprintf (port_str, sizeof port_str, "%d", config->port);
-  args[j++] = port_str;
-  args[j++] = "-o";             /* Host key will always be novel. */
-  args[j++] = "StrictHostKeyChecking=no";
-  args[j++] = "-o";             /* ConnectTimeout */
+  ADD_ARG (argv, i, port_str);
+  ADD_ARG (argv, i, "-o");      /* Host key will always be novel. */
+  ADD_ARG (argv, i, "StrictHostKeyChecking=no");
+  ADD_ARG (argv, i, "-o");      /* ConnectTimeout */
   snprintf (connect_timeout_str, sizeof connect_timeout_str,
             "ConnectTimeout=%d", SSH_TIMEOUT);
-  args[j++] = connect_timeout_str;
+  ADD_ARG (argv, i, connect_timeout_str);
   if (using_password_auth) {
     /* Only use password authentication. */
-    args[j++] = "-o";
-    args[j++] =
"PreferredAuthentications=keyboard-interactive,password";
+    ADD_ARG (argv, i, "-o");
+    ADD_ARG (argv, i,
"PreferredAuthentications=keyboard-interactive,password");
   }
   else {
     /* Use identity file (private key). */
-    args[j++] = "-o";
-    args[j++] = "PreferredAuthentications=publickey";
-    args[j++] = "-i";
-    args[j++] = config->identity_file;
+    ADD_ARG (argv, i, "-o");
+    ADD_ARG (argv, i, "PreferredAuthentications=publickey");
+    ADD_ARG (argv, i, "-i");
+    ADD_ARG (argv, i, config->identity_file);
   }
-  args[j++] = localfile;
+  ADD_ARG (argv, i, localfile);
   if (asprintf (&remote, "%s@%s:%s",
                 config->username ? config->username : "root",
                 config->server, remotefile) == -1)
     error (EXIT_FAILURE, errno, "asprintf");
-  args[j++] = remote;
-  args[j++] = NULL;
-  assert (j == nr_args);
+  ADD_ARG (argv, i, remote);
+  ADD_ARG (argv, i, NULL);
 
 #if DEBUG_STDERR
   fputs ("scp command: ", stderr);
-  for (i = 0; i < nr_args - 1; ++i) {
+  for (i = 0; argv[i] != NULL; ++i) {
     if (i > 0) fputc (' ', stderr);
-    fputs (args[i], stderr);
+    fputs (argv[i], stderr);
   }
   fputc ('\n', stderr);
 #endif
 
   /* Create the miniexpect handle. */
-  h = mexp_spawnv ("scp", (char **) args);
+  h = mexp_spawnv ("scp", (char **) argv);
   if (h == NULL) {
     set_ssh_internal_error ("scp: mexp_spawnv: %m");
     return -1;
-- 
2.10.2

Also drop the connection if no response at all has been received after
30 minutes.

This action should prevent firewall timeouts from causing virt-p2v to
fail (see this thread:
https://www.redhat.com/archives/libguestfs/2017-February/msg00010.html).

Thanks: Tomáš Golembiovský
---
 p2v/ssh.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/p2v/ssh.c b/p2v/ssh.c
index 817048c..4dd4c44 100644
--- a/p2v/ssh.c
+++ b/p2v/ssh.c
@@ -354,6 +354,10 @@ start_ssh (unsigned spawn_flags, struct config *config,
   snprintf (connect_timeout_str, sizeof connect_timeout_str,
             "ConnectTimeout=%d", SSH_TIMEOUT);
   ADD_ARG (argv, i, connect_timeout_str);
+  ADD_ARG (argv, i, "-o");      /* Send ping packets every 5 mins to
sshd. */
+  ADD_ARG (argv, i, "ServerAliveInterval=300");
+  ADD_ARG (argv, i, "-o");
+  ADD_ARG (argv, i, "ServerAliveCountMax=6");
   if (using_password_auth) {
     /* Only use password authentication. */
     ADD_ARG (argv, i, "-o");
-- 
2.10.2

---
 p2v/virt-p2v.pod | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/p2v/virt-p2v.pod b/p2v/virt-p2v.pod
index e04c1f1..98763f6 100644
--- a/p2v/virt-p2v.pod
+++ b/p2v/virt-p2v.pod
@@ -568,6 +568,44 @@ L<virt-p2v-make-disk(1)/ADDING AN SSH IDENTITY>
 
 L<virt-p2v-make-kickstart(1)/ADDING AN SSH IDENTITY>
 
+=head1 COMMON PROBLEMS
+
+=head2 Timeouts
+
+As described below (see L</HOW VIRT-P2V WORKS>) virt-p2v makes several
+long-lived ssh connections to the conversion server.  If these
+connections time out then virt-p2v will fail.
+
+To test if a timeout might be causing problems, open an XTerm on the
+virt-p2v machine, C<ssh root@I<conversion-server>>, and leave it
for
+at least an hour.  If the session disconnects without you doing
+anything, then there is a timeout which you should turn off.
+
+Timeouts happen because:
+
+=over 4
+
+=item C<TIMEOUT> or C<TMOUT> environment variable
+
+Check if one of these environment variables is set in the root shell
+on the conversion server.
+
+=item sshd C<ClientAlive*> setting
+
+Check for C<ClientAlive*> settings in C</etc/ssh/sshd_config> on
the
+conversion server.
+
+=item Firewall or NAT settings
+
+Check if there is a firewall or NAT box between virt-p2v and the
+conversion server, and if this firewall drops idle connections after a
+too-short time.
+
+virt-p2v E<ge> 1.36 attempts to work around firewall timeouts by
+sending ssh keepalive messages every 5 minutes.
+
+=back
+
 =head1 OPTIONS
 
 =over 4
-- 
2.10.2