Regarding the second patch, my thought is that if a user forgets the
'--', the current behaviour will either crash as described in my bug
report, or try to use the following arguments as the name of the
container to open. That is, I believe that
lxc-netstat -n <container-name> -n -a
will result in trying to do netstat on container "-a". I think my patch
brings lxc-netstat's behaviour into a more consistent form, given that
it seems
lxc-netstat -n <container-name> -a -n
will result in calling netstat on container-name with "-a -n" as the
arguments (see the last line of the case statement; line 81 of the
unpatched source). The behaviour currently depends on the order of the
arguments, as far as I can tell.
Perhaps a better approach would be to consistently die cleanly if no --
is provided, unless there are no further arguments? Or maybe it doesn't
matter if behaviour is undefined under these circumstances.
--Andrew
On 06/26/2013 06:57 AM, Serge Hallyn wrote:
> Quoting Andrew Gilbert (andrewg...@gmail.com):
>> Hi,
>> I think I've discovered a bug in lxc-netstat when trying to have it run
>> netstat with the '-n' option. I've attached a patch for this below
>> (patch 1). I've also attached a patch that tries to Do The Right Thing
>> if someone uses '-n' without the '--' before it (patch 2). I'd be happy
>> to resend the patches as separate emails if that would be better.
>> --Andrew G
> Hi,
>
> thanks, first patch confirmed and
>
> Acked-by: Serge E. Hallyn<serge.hal...@ubuntu.com>
>
> from me. But yes, please do resend as separate patches (threaded,
> 1/2 and 2/2) and (more importantly) do include a signed-off-by.
> (If there were more than 2 emails I'd suggest using git-send-email,
> but with two that's a bit over the top)
>
> (Question below on second patch)
>
>> S T E P S T O R E P R O D U C E:
>> 1. Start a container.
>> 2. Use lxc-netstat -n <container-name> -- -n -a to try to get numeric
>> IPs in netstat output.
>> 3. Get error "./lxc-netstat: 75: shift: can't shift that many."
>>
>> It appears that after the execution of lxc-unshare, the call to
>> lxc-netstat does not include the '--' separator to prevent the netstat
>> arguments being interpreted as lxc-netstat arguments. This only would
>> manifest as a bug if the option is one that is valid for lxc-netstat as
>> well as netstat, as the lone -n option is.
>>
>> P A T C H 1:
>> From 78ea721f2a6b0cc01cffba93109bd8d8202eeb98 Mon Sep 17 00:00:00 2001
>> From: Andrew Gilbert<andrewg...@gmail.com>
>> Date: Fri, 21 Jun 2013 11:24:37 -0700
>> Subject: [PATCH] Add double-dash to lxc-netstat re-call arguments
>>
>> When lxc-netstat was called by lxc-unshare, it would be given the
>> arguments intended for netstat from the first invocation, but without
>> anything to separate them from the arguments intended for lxc-netstat.
>> This meant that netstat arguments like -n would result in lxc-netstat
>> trying to process them
>> ---
>> src/lxc/lxc-netstat.in | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/lxc/lxc-netstat.in b/src/lxc/lxc-netstat.in
>> index 2fa2d23..229c214 100644
>> --- a/src/lxc/lxc-netstat.in
>> +++ b/src/lxc/lxc-netstat.in
>> @@ -93,7 +93,7 @@ if [ -z "$name" ]; then
>> fi
>>
>> if [ -z "$exec" ]; then
>> - exec @BINDIR@/lxc-unshare -s MOUNT -- $0 -n $name --exec "$@"
>> + exec @BINDIR@/lxc-unshare -s MOUNT -- $0 -n $name --exec -- "$@"
>> fi
>>
>> if lxc-info -n $name --state-is 'STOPPED'; then
>> --
>> 1.8.1.2
>>
>> P A T C H 2:
>> From e45138027e08d291f4fad1357bcf5cb1b610c402 Mon Sep 17 00:00:00 2001
>> From: Andrew Gilbert<andrewg...@gmail.com>
>> Date: Fri, 21 Jun 2013 14:36:28 -0700
>> Subject: [PATCH 2/2] Add -n differentiation to lxc-netstat
>>
>> lxc-netstat now only processes an -n argument if it has not previously
>> received a value for $name from --name or -n. If it _has_ received such
>> a value, it stops processing arguments and leaves the -n for netstat.
>> This does not apply to the use of --name after a name has been provided
>> by --name or -n; the current behaviour continues.
> Can you please give a test case here? It sounds to me like you're
> supporting
>
> lxc-netstat -n <container-name> -n -a
>
> without the '--'. I'm not sure we want to do that - the rules end
> up more ambiguous than a simple 'use -- to pass arguments on'.
>
>> ---
>> src/lxc/lxc-netstat.in | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/lxc/lxc-netstat.in b/src/lxc/lxc-netstat.in
>> index 229c214..4a239d9 100644
>> --- a/src/lxc/lxc-netstat.in
>> +++ b/src/lxc/lxc-netstat.in
>> @@ -66,12 +66,23 @@ get_parent_cgroup()
>> }
>>
>> exec=""
>> +name=""
>>
>> while true; do
>> case $1 in
>> -h|--help)
>> help; exit 1;;
>> - -n|--name)
>> + -n)
>> + # If we already have a value for $name, treat -n as being an
>> + # argument for netstat
>> + if [ -n "$name" ]
>> + then
>> + break
>> + else
>> + name="$2"; shift 2;
>> + fi
>> + ;;
>> + --name)
>> name=$2; shift 2;;
>> --exec)
>> exec="exec"; shift;;
>> --
>> 1.8.1.2
>>
>>
>>
>> ------------------------------------------------------------------------------
>> This SF.net email is sponsored by Windows:
>>
>> Build for Windows Store.
>>
>> http://p.sf.net/sfu/windows-dev2dev
>> _______________________________________________
>> Lxc-devel mailing list
>> Lxc-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/lxc-devel
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
