[IPROUTE]: Fix up "ip link" help text

[IPROUTE]: Fix up "ip link" help text

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

With this patch, "ip link add help" shows:

Usage: ip link add [ link DEV ] [ name ] NAME
                    [ txqueuelen PACKETS ]
                    [ address LLADDR ]
                    [ broadcast LLADDR ]
                    [ mtu MTU ]
                    type TYPE [ ARGS ]

        ip link set DEVICE [ { up | down } ]
                           [ arp { on | off } ]
                           [ dynamic { on | off } ]
                           [ multicast { on | off } ]
                           [ allmulticast { on | off } ]
                           [ promisc { on | off } ]
                           [ trailers { on | off } ]
                           [ txqueuelen PACKETS ]
                           [ name NEWNAME ]
                           [ address LLADDR ]
                           [ broadcast LLADDR ]
                           [ mtu MTU ]
                           [ netns PID ]
        ip link show [ DEVICE ]

TYPE := { vlan | veth | dummy | ifb | macvlan }

[-- Attachment #2: 03.diff --]
[-- Type: text/x-patch, Size: 5527 bytes --]

commit 5e52e54c147b631e555294e6446d9c2d609207f2
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue Oct 7 17:15:13 2008 +0200

    [IPROUTE]: Fix up "ip link" help text
    
    Show help for "ip link add" in case its supported by the kernel. Also fix up
    the "ip link set" help text:
    
    - the flags are not mutually exclusive as suggested by the current text,
      up/down states and on/off are mutually exclusive.
    
    - txqueuelen, name etc. have nothing to do with the flags and on/off states
      at all and are certainly not mutually exclusive, fix formatting and make
      it conform the other helptexts better.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/ip/iplink.c b/ip/iplink.c
index 6e9ac71..a99115a 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -37,23 +37,85 @@
 #define LIBDIR "/usr/lib/"
 #endif
 
+#if IPLINK_IOCTL_COMPAT
+static int have_rtnl_newlink = -1;
+
+static int accept_msg(const struct sockaddr_nl *who,
+		      struct nlmsghdr *n, void *arg)
+{
+	struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(n);
+
+	if (n->nlmsg_type == NLMSG_ERROR &&
+	    (err->error == -EOPNOTSUPP || err->error == -EINVAL))
+		have_rtnl_newlink = 0;
+	else
+		have_rtnl_newlink = 1;
+	return -1;
+}
+
+static int iplink_have_newlink(void)
+{
+	struct {
+		struct nlmsghdr		n;
+		struct ifinfomsg	i;
+		char			buf[1024];
+	} req;
+
+	if (have_rtnl_newlink < 0) {
+		memset(&req, 0, sizeof(req));
+
+		req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
+		req.n.nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
+		req.n.nlmsg_type = RTM_NEWLINK;
+		req.i.ifi_family = AF_UNSPEC;
+
+		rtnl_send(&rth, (char *)&req.n, req.n.nlmsg_len);
+		rtnl_listen(&rth, accept_msg, NULL);
+	}
+	return have_rtnl_newlink;
+}
+#else /* IPLINK_IOCTL_COMPAT */
+static int iplink_have_newlink(void)
+{
+	return 1;
+}
+#endif /* ! IPLINK_IOCTL_COMPAT */
+
 static void usage(void) __attribute__((noreturn));
 
 void iplink_usage(void)
 {
-	fprintf(stderr, "Usage: ip link set DEVICE { up | down |\n");
-	fprintf(stderr, "	                     arp { on | off } |\n");
-	fprintf(stderr, "	                     dynamic { on | off } |\n");
-	fprintf(stderr, "	                     multicast { on | off } |\n");
-	fprintf(stderr, "	                     allmulticast { on | off } |\n");
-	fprintf(stderr, "	                     promisc { on | off } |\n");
-	fprintf(stderr, "	                     trailers { on | off } |\n");
-	fprintf(stderr, "	                     txqueuelen PACKETS |\n");
-	fprintf(stderr, "	                     name NEWNAME |\n");
-	fprintf(stderr, "	                     address LLADDR | broadcast LLADDR |\n");
-	fprintf(stderr, "	                     mtu MTU }\n");
-	fprintf(stderr, "	                     netns PID }\n");
+	if (iplink_have_newlink()) {
+		fprintf(stderr, "Usage: ip link add [ link DEV ] [ name ] NAME\n");
+		fprintf(stderr, "                   [ txqueuelen PACKETS ]\n");
+		fprintf(stderr, "                   [ address LLADDR ]\n");
+		fprintf(stderr, "                   [ broadcast LLADDR ]\n");
+		fprintf(stderr, "                   [ mtu MTU ]\n");
+		fprintf(stderr, "                   type TYPE [ ARGS ]\n");
+		fprintf(stderr, "\n");
+		fprintf(stderr, "       ip link set DEVICE [ { up | down } ]\n");
+	} else
+		fprintf(stderr, "Usage: ip link set DEVICE [ { up | down } ]\n");
+
+	fprintf(stderr, "	                  [ arp { on | off } ]\n");
+	fprintf(stderr, "	                  [ dynamic { on | off } ]\n");
+	fprintf(stderr, "	                  [ multicast { on | off } ]\n");
+	fprintf(stderr, "	                  [ allmulticast { on | off } ]\n");
+	fprintf(stderr, "	                  [ promisc { on | off } ]\n");
+	fprintf(stderr, "	                  [ trailers { on | off } ]\n");
+	fprintf(stderr, "	                  [ txqueuelen PACKETS ]\n");
+	fprintf(stderr, "	                  [ name NEWNAME ]\n");
+	fprintf(stderr, "	                  [ address LLADDR ]\n");
+	fprintf(stderr, "	                  [ broadcast LLADDR ]\n");
+	fprintf(stderr, "	                  [ mtu MTU ]\n");
+	fprintf(stderr, "	                  [ netns PID ]\n");
 	fprintf(stderr, "       ip link show [ DEVICE ]\n");
+
+	if (iplink_have_newlink()) {
+		fprintf(stderr, "\n");
+		fprintf(stderr, "TYPE := { vlan | veth | dummy | ifb | macvlan }\n");
+	}
+
 	exit(-1);
 }
 
@@ -103,50 +165,6 @@ struct link_util *get_link_kind(const char *id)
 	return l;
 }
 
-#if IPLINK_IOCTL_COMPAT
-static int have_rtnl_newlink = -1;
-
-static int accept_msg(const struct sockaddr_nl *who,
-		      struct nlmsghdr *n, void *arg)
-{
-	struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(n);
-
-	if (n->nlmsg_type == NLMSG_ERROR &&
-	    (err->error == -EOPNOTSUPP || err->error == -EINVAL))
-		have_rtnl_newlink = 0;
-	else
-		have_rtnl_newlink = 1;
-	return -1;
-}
-
-static int iplink_have_newlink(void)
-{
-	struct {
-		struct nlmsghdr		n;
-		struct ifinfomsg	i;
-		char			buf[1024];
-	} req;
-
-	if (have_rtnl_newlink < 0) {
-		memset(&req, 0, sizeof(req));
-
-		req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-		req.n.nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
-		req.n.nlmsg_type = RTM_NEWLINK;
-		req.i.ifi_family = AF_UNSPEC;
-
-		rtnl_send(&rth, (char *)&req.n, req.n.nlmsg_len);
-		rtnl_listen(&rth, accept_msg, NULL);
-	}
-	return have_rtnl_newlink;
-}
-#else /* IPLINK_IOCTL_COMPAT */
-static int iplink_have_newlink(void)
-{
-	return 1;
-}
-#endif /* ! IPLINK_IOCTL_COMPAT */
-
 struct iplink_req {
 	struct nlmsghdr		n;
 	struct ifinfomsg	i;

Re: [IPROUTE]: Fix up "ip link" help text

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

Arkadiusz Miskiewicz wrote:
> Runtime dependand help doesn't look sane. Help should always show all possible 
> options/command while trying to use unsupported option should display error 
> message.
>   

That makes sense. But don't complain to me about the quality
of the error messages afterwards please :)



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3127 bytes --]

commit 8c6321466df9a452ac58e09d32edd6c5b4ea0d17
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue Oct 7 23:00:59 2008 +0200

    [IPROUTE]: Fix up "ip link" help text
    
    Add help for "ip link add". Also fix up the "ip link set" help text:
    
    - the flags are not mutually exclusive as suggested by the current text,
      up/down states and on/off are mutually exclusive.
    
    - txqueuelen, name etc. have nothing to do with the flags and on/off states
      at all and are certainly not mutually exclusive, fix formatting and make
      it conform the other helptexts better.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/ip/iplink.c b/ip/iplink.c
index 6e9ac71..781fbd2 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -41,19 +41,30 @@ static void usage(void) __attribute__((noreturn));
 
 void iplink_usage(void)
 {
-	fprintf(stderr, "Usage: ip link set DEVICE { up | down |\n");
-	fprintf(stderr, "	                     arp { on | off } |\n");
-	fprintf(stderr, "	                     dynamic { on | off } |\n");
-	fprintf(stderr, "	                     multicast { on | off } |\n");
-	fprintf(stderr, "	                     allmulticast { on | off } |\n");
-	fprintf(stderr, "	                     promisc { on | off } |\n");
-	fprintf(stderr, "	                     trailers { on | off } |\n");
-	fprintf(stderr, "	                     txqueuelen PACKETS |\n");
-	fprintf(stderr, "	                     name NEWNAME |\n");
-	fprintf(stderr, "	                     address LLADDR | broadcast LLADDR |\n");
-	fprintf(stderr, "	                     mtu MTU }\n");
-	fprintf(stderr, "	                     netns PID }\n");
+	fprintf(stderr, "Usage: ip link add [ name ] NAME\n");
+	fprintf(stderr, "                   [ link DEV ]\n");
+	fprintf(stderr, "                   [ txqueuelen PACKETS ]\n");
+	fprintf(stderr, "                   [ address LLADDR ]\n");
+	fprintf(stderr, "                   [ broadcast LLADDR ]\n");
+	fprintf(stderr, "                   [ mtu MTU ]\n");
+	fprintf(stderr, "                   type TYPE [ ARGS ]\n");
+	fprintf(stderr, "\n");
+	fprintf(stderr, "       ip link set DEVICE [ { up | down } ]\n");
+	fprintf(stderr, "	                  [ arp { on | off } ]\n");
+	fprintf(stderr, "	                  [ dynamic { on | off } ]\n");
+	fprintf(stderr, "	                  [ multicast { on | off } ]\n");
+	fprintf(stderr, "	                  [ allmulticast { on | off } ]\n");
+	fprintf(stderr, "	                  [ promisc { on | off } ]\n");
+	fprintf(stderr, "	                  [ trailers { on | off } ]\n");
+	fprintf(stderr, "	                  [ txqueuelen PACKETS ]\n");
+	fprintf(stderr, "	                  [ name NEWNAME ]\n");
+	fprintf(stderr, "	                  [ address LLADDR ]\n");
+	fprintf(stderr, "	                  [ broadcast LLADDR ]\n");
+	fprintf(stderr, "	                  [ mtu MTU ]\n");
+	fprintf(stderr, "	                  [ netns PID ]\n");
 	fprintf(stderr, "       ip link show [ DEVICE ]\n");
+	fprintf(stderr, "\n");
+	fprintf(stderr, "TYPE := { vlan | veth | dummy | ifb | macvlan }\n");
 	exit(-1);
 }
 

Re: [IPROUTE]: Fix up "ip link" help text

[Stephen Hemminger]

On Tue, 07 Oct 2008 23:02:17 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Arkadiusz Miskiewicz wrote:
> > Runtime dependand help doesn't look sane. Help should always show all possible 
> > options/command while trying to use unsupported option should display error 
> > message.
> >   
> 
> That makes sense. But don't complain to me about the quality
> of the error messages afterwards please :)
> 
> 

I had a better message set in works. will merge mine and patrick's

Re: [IPROUTE]: Fix up "ip link" help text

[Stephen Hemminger]

On Tue, 07 Oct 2008 23:02:17 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Arkadiusz Miskiewicz wrote:
> > Runtime dependand help doesn't look sane. Help should always show all possible 
> > options/command while trying to use unsupported option should display error 
> > message.
> >   
> 
> That makes sense. But don't complain to me about the quality
> of the error messages afterwards please :)
> 
> 

I applied a variant of Patrick's original patch. I added text for delete, and
avoided some code movement.  The help text should only show what is supported
otherwise there will be user complaints.

Re: [IPROUTE]: Fix up "ip link" help text

[Ben Greear]

Stephen Hemminger wrote:
> On Tue, 07 Oct 2008 23:02:17 +0200
> Patrick McHardy <kaber@trash.net> wrote:
>
>   
>> Arkadiusz Miskiewicz wrote:
>>     
>>> Runtime dependand help doesn't look sane. Help should always show all possible 
>>> options/command while trying to use unsupported option should display error 
>>> message.
>>>   
>>>       
>> That makes sense. But don't complain to me about the quality
>> of the error messages afterwards please :)
>>
>>
>>     
>
> I applied a variant of Patrick's original patch. I added text for delete, and
> avoided some code movement.  The help text should only show what is supported
> otherwise there will be user complaints.
>   
I personally dislike that.  I think the tool show should everything that 
it *could* do assuming
proper kernel, etc.  Then, if it turns out the current running kernel 
can't do that, it could just
give an error:  "This feature not supported in your current kernel.  See 
[url] for info on when
features were added to the kernel."

That way, the user can see all the possible goodness out there, and 
perhaps be inclined to upgrade
to a later kernel if that gives them a feature they'd like.

Thanks,
Ben

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [IPROUTE]: Fix up "ip link" help text

[Stephen Hemminger]

On Mon, 13 Oct 2008 19:17:00 +0200
Arkadiusz Miskiewicz <arekm@maven.pl> wrote:

> On Monday 13 of October 2008, Stephen Hemminger wrote:
> > On Tue, 07 Oct 2008 23:02:17 +0200
> >
> > Patrick McHardy <kaber@trash.net> wrote:
> > > Arkadiusz Miskiewicz wrote:
> > > > Runtime dependand help doesn't look sane. Help should always show all
> > > > possible options/command while trying to use unsupported option should
> > > > display error message.
> > >
> > > That makes sense. But don't complain to me about the quality
> > > of the error messages afterwards please :)
> >
> > I applied a variant of Patrick's original patch. I added text for delete,
> > and avoided some code movement.  The help text should only show what is
> > supported otherwise there will be user complaints.
> 
> Too bad. 
> 
> Help should show all options while trying to use unsupported ones should 
> yeld "unsupported"/some error message.
> 
> Will ipv6 options etc, etc also be hidden when ipv6 not available?
> 


The options won't show on older kernels. It is not a kernel config option
difference, it is old legacy code, like back to 2.4 where the API's just
aren't there.