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.