Re: [libvirt] [PATCH 1/3] Autodetect if the remote nc command supports the -q option
On 10/12/2011 04:39 PM, Guido Günther wrote:
Based on a patch by Marc
Deslauriers<marc.deslauriers(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org