Skip to contentSkip to conversation

: Fix up "ip link" help text

IPROUTE Patrick McHardy 3 participants 6 messages Oct 7, 2008 – Oct 13, 2008 2 messages not in the archive

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Patrick McHardy

author
#0 lore raw

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 }

commit 5e52e54c147b631e555294e6446d9c2d609207f2

Author: Patrick McHardy <[email protected]>
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 <[email protected]>
ip/iplink.c +74 -56
index 6e9ac71..a99115a 100644
@@ -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;

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw
Quoted 2 lines · Arkadiusz Miskiewicz

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 :)

commit 8c6321466df9a452ac58e09d32edd6c5b4ea0d17

Author: Patrick McHardy <[email protected]>
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 <[email protected]>
ip/iplink.c +23 -12
index 6e9ac71..781fbd2 100644
@@ -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);
 }
#3 lore raw
Quoted 10 lines · On Tue, 07 Oct 2008 23:02:17 +0200 Patrick McHardy <[email protected]> wrote:
Quoted 4 lines · Arkadiusz Miskiewicz

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

#4 lore raw
In reply to Patrick McHardy
Quoted 10 lines · On Tue, 07 Oct 2008 23:02:17 +0200 Patrick McHardy <[email protected]> wrote:
Quoted 4 lines · Arkadiusz Miskiewicz

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.

#5 lore raw
Quoted 15 lines · Stephen Hemminger
Quoted 10 lines · On Tue, 07 Oct 2008 23:02:17 +0200 Patrick McHardy <[email protected]> wrote:
Quoted 3 lines · Arkadiusz Miskiewicz

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 :)

Quoted 4 lines

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

Quoted 5 lines

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

Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc  http://www.candelatech.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

#7 lore raw
Quoted 16 lines · On Mon, 13 Oct 2008 19:17:00 +0200 Arkadiusz Miskiewicz <[email protected]> wrote:
Quoted 8 lines · Stephen Hemminger

On Tue, 07 Oct 2008 23:02:17 +0200

Quoted 5 lines · Patrick McHardy
Quoted 3 lines · Arkadiusz Miskiewicz

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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 7 lines

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.