[libvirt] [PATCH 1/3] 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 | 23 ++++++++++++++++++++--- tests/virnetsockettest.c | 11 ++++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index e4289b1..ea653da 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -634,9 +634,26 @@ 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); + + 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;" + "fi;" + "%s $ARG -U %s'", + netcat ? netcat : "nc", + netcat ? netcat : "nc", + path); return virNetSocketNewConnectCommand(cmd, retsock); } diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index fae15a3..b3a2705 100644 --- a/tests/virnetsockettest.c +++ 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", }; if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0) ret = -1; @@ -509,7 +509,7 @@ 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;fi;netcat $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0) ret = -1; @@ -522,7 +522,7 @@ 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;fi;netcat $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0) ret = -1; @@ -538,7 +538,8 @@ 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;fi;nc $ARG -U /tmp/socket'\n", + .dieEarly = true, }; if (virtTestRun("SSH test 5", 1, testSocketSSH, &sshData5) < 0) @@ -549,7 +550,7 @@ 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;fi;nc $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 6", 1, testSocketSSH, &sshData6) < 0) ret = -1; -- 1.7.6.3
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. 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"; 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 ..."; (or whatever it takes to match any changes you make above). ACK with nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
On 10/13/2011 03:02 PM, Guido Günther wrote:
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(-)
New version attached.
Good job addressing my comments; ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Guido Günther