[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] Autodetect if the remote nc command supports the -q option



Hi Eric,

On Wed, Oct 12, 2011 at 05:03:45PM -0600, Eric Blake wrote:
> On 10/12/2011 04:39 PM, Guido Günther wrote:
> >Based on a patch by Marc Deslauriers<marc deslauriers ubuntu com>
> >
> >RH: https://bugzilla.redhat.com/show_bug.cgi?id=562176
> >Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/517478
> >Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=573172
> >---
> >  src/rpc/virnetsocket.c   |   23 ++++++++++++++++++++---
> >  tests/virnetsockettest.c |   11 ++++++-----
> >  2 files changed, 26 insertions(+), 8 deletions(-)
> >
> >+    virCommandAddArgList(cmd, nodename, "sh", "-c", NULL);
> >+    /*
> >+     * This ugly thing is a shell script to detect availability of
> >+     * the -q option for 'nc': debian and suse based distros need this
> >+     * flag to ensure the remote nc will exit on EOF, so it will go away
> >+     * when we close the connection tunnel. If it doesn't go away, subsequent
> >+     * connection attempts will hang.
> >+     *
> >+     * Fedora's 'nc' doesn't have this option, and defaults to the desired
> >+     * behavior.
> >+     */
> 
> The comment is essential :)
> 
> >+    virCommandAddArgFormat(cmd,
> >+         "'if %s -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then"
> >+         "     ARG=-q0;"
> >+         "fi;"
> >+         "%s $ARG -U %s'",
> 
> This relies on ARG not being inherited from the environment.
> Probably safe, but just out of paranoia, and in a desire to compress
> things a bit, I'd go with either a pre-initialization:
> 
> s/'if %s/'ARG=;if %s/
> 
> or an else clause to the if-then-fi.

I went for the else clause since I think it's best for readability.

> 
> Also, since we aren't using any space after ;, why do we need four
> spaces before ARG=-q0?  We need at least one space (or a newline)
> after 'then', but either we should use newline after each part of
> the command (to match how it is listed in the source) or compress
> things to minimal size.
> 
> >+         netcat ? netcat : "nc",
> >+         netcat ? netcat : "nc",
> 
> Micro-optimization: prior to the virCommandAddArgFormat, I would have done:
> 
> if (!netcat)
>     netcat = "nc";

Done.

> 
> then just directly used netcat here instead of dual ?:.  But that's
> a nit that you don't have to worry about (patch 3/3 does the same
> thing).
> 
> >+++ b/tests/virnetsockettest.c
> >@@ -496,7 +496,7 @@ mymain(void)
> >      struct testSSHData sshData1 = {
> >          .nodename = "somehost",
> >          .path = "/tmp/socket",
> >-        .expectOut = "somehost nc -U /tmp/socket\n",
> >+        .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then     ARG=-q0;fi;nc $ARG -U /tmp/socket'\n",
> 
> Feel free to break this into multiple string literals; along
> expected newline boundaries might be nice:
> 
> .expectOut = "somehost sh -c "
>     "'if ...; then\n"
>     "    ARG=-q0\n"
>     "fi\n"
>     "nc $ARG ...";

Done.

> 
> (or whatever it takes to match any changes you make above).
> 
> ACK with nits fixed.

New version attached.
Cheers,
 -- Guido
>From 52bcc244aa521147918e5a0ed8391676c6e17aa1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx sigxcpu org>
Date: Fri, 8 Jul 2011 21:07:29 +0200
Subject: [PATCH 1/4] Autodetect if the remote nc command supports the -q
 option

Based on a patch by Marc Deslauriers <marc deslauriers ubuntu com>

RH: https://bugzilla.redhat.com/show_bug.cgi?id=562176
Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/517478
Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=573172
---
 src/rpc/virnetsocket.c   |   26 +++++++++++++++++++++++---
 tests/virnetsockettest.c |   39 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index e4289b1..2a9bca0 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -634,9 +634,29 @@ int virNetSocketNewConnectSSH(const char *nodename,
                              "-e", "none", NULL);
     if (noVerify)
         virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
-    virCommandAddArgList(cmd, nodename,
-                         netcat ? netcat : "nc",
-                         "-U", path, NULL);
+
+    if (!netcat)
+        netcat = "nc";
+
+    virCommandAddArgList(cmd, nodename, "sh", "-c", NULL);
+    /*
+     * This ugly thing is a shell script to detect availability of
+     * the -q option for 'nc': debian and suse based distros need this
+     * flag to ensure the remote nc will exit on EOF, so it will go away
+     * when we close the connection tunnel. If it doesn't go away, subsequent
+     * connection attempts will hang.
+     *
+     * Fedora's 'nc' doesn't have this option, and defaults to the desired
+     * behavior.
+     */
+    virCommandAddArgFormat(cmd,
+         "'if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+             "ARG=-q0;"
+         "else "
+             "ARG=;"
+         "fi;"
+         "%s $ARG -U %s'",
+         netcat, netcat, path);
 
     return virNetSocketNewConnectCommand(cmd, retsock);
 }
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index fae15a3..75cc9c0 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -496,7 +496,12 @@ mymain(void)
     struct testSSHData sshData1 = {
         .nodename = "somehost",
         .path = "/tmp/socket",
-        .expectOut = "somehost nc -U /tmp/socket\n",
+        .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                                         "ARG=-q0;"
+                                     "else "
+                                         "ARG=;"
+                                     "fi;"
+                                     "nc $ARG -U /tmp/socket'\n",
     };
     if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0)
         ret = -1;
@@ -509,7 +514,13 @@ mymain(void)
         .noTTY = true,
         .noVerify = false,
         .path = "/tmp/socket",
-        .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost netcat -U /tmp/socket\n",
+        .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh -c '"
+                     "if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "netcat $ARG -U /tmp/socket'\n",
     };
     if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0)
         ret = -1;
@@ -522,7 +533,13 @@ mymain(void)
         .noTTY = false,
         .noVerify = true,
         .path = "/tmp/socket",
-        .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost netcat -U /tmp/socket\n",
+        .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c '"
+                     "if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "netcat $ARG -U /tmp/socket'\n",
     };
     if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0)
         ret = -1;
@@ -538,7 +555,13 @@ mymain(void)
     struct testSSHData sshData5 = {
         .nodename = "crashyhost",
         .path = "/tmp/socket",
-        .expectOut = "crashyhost nc -U /tmp/socket\n",
+        .expectOut = "crashyhost sh -c "
+                     "'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "nc $ARG -U /tmp/socket'\n",
         .dieEarly = true,
     };
     if (virtTestRun("SSH test 5", 1, testSocketSSH, &sshData5) < 0)
@@ -549,7 +572,13 @@ mymain(void)
         .path = "/tmp/socket",
         .keyfile = "/root/.ssh/example_key",
         .noVerify = true,
-        .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com nc -U /tmp/socket\n",
+        .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com sh -c '"
+                     "if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "nc $ARG -U /tmp/socket'\n",
     };
     if (virtTestRun("SSH test 6", 1, testSocketSSH, &sshData6) < 0)
         ret = -1;
-- 
1.7.6.3


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]