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

[Eric Blake]

On 10/12/2011 04:39 PM, Guido Günther wrote:
> Based on a patch by Marc Deslauriers<marc.deslauriers at 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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org