[BusyBox] vlanconfig - alternative to vconfig applet

[Manuel Novoa III]

Hello,

On Sun, Jan 26, 2003 at 05:48:35PM +0200, Nick Fedchik wrote:
> On Sat, 25 Jan 2003, Nick Fedchik wrote:
> 
> ...
> > so much more changes and have made a decision to make new
> > applet - vlanconfig.
> > The patch attached.
> Sorry, that previous patch was very buggy! I was very tired, so
> made a lot of mistakes. I hope it was'nt critical to anyone.
> Here the new patch instead of previous.
> Also this version has heavy redesigned, so I don't made a patch
> over previous patch, but post completelly new patch.

While I commend your motivation and efforts, I suspect that anyone
using vconfig (I don't btw) isn't likely to want to change their
scripts, etc. to use your different command line args.  I certainly
agree though that the current vconfig.c applet is a "bloated pig"
and I've already made several comments to Erik about it.

That being said, I thought I'd give some constructive feedback
on your applet, as well as include a vconfig applet that I did a
few size-optimization iterations on.  As a comparison,
on i386 with gcc (GCC) 3.2.2 20030109 (Debian prerelease), I get

  text    data     bss     dec     hex filename
  2835       0       0    2835     b13 vconfig-dist.o  (current vconfig)
  1089       0       0    1089     441 vlanconfig.o    (your alternate)
   798       0       0     798     31e vconfig.o       (my vconfig)

As I said, I don't use vconfig myself, so I only tested that the
arg processing was correct.

I hope you and others find the following comments and code example useful.

Manuel

-------------------------------------------------------------------
(your applet... minus headers)

> +#define VLAN_GROUP_ARRAY_LEN 4096
> +#define SIOCSIFVLAN	0x8983	/* Set 802.1Q VLAN options  */
> +
> +int vlanconfig_main(int argc, char **argv)
> +{
> +	int fd;
> +	struct vlan_ioctl_args ifr;
> +	int cmd, set_opt;
> +	char *endptr = NULL;

There's no need to initialize endptr.

> +	char *conf_file_name = "/proc/net/vlan/config";
> +	char *int_len_msg = "Interface name must be less than";

Since these never change, it would be smaller to do

static const char conf_file_name[] = ...
static const char int_len_msg[] = ...

> +
> +/*
> +Usage: add      <iface-name> <vid>
> +       del      <vlan-name>
> +       set	nametype  {1|2|3|4}
> +       set	flag	<vlan-name>
> +       set	egress  <vlan-name>	<skb_priority>   <vlan_qos> 0-7
> +       set	ingress <vlan-name>	<skb_priority>   <vlan_qos>
> +
> +*/
> +
> +	enum vlan_cmd_num { cmd_add, cmd_del, cmd_set };
> +
> +#define VLAN_CMD_HIGHEST 3
> +	char *vlan_cmd[VLAN_CMD_HIGHEST] = {
> +		"add",
> +		"del",
> +		"set"
> +	};
> +
> +	enum vlan_cmd_set_opt_num { set_nametype, set_flag, set_egress,
> +		set_ingress
> +	};
> +
> +#define VLAN_CMD_SET_OPT_HIGHEST 4
> +	char *vlan_cmd_set_opt[VLAN_CMD_SET_OPT_HIGHEST] = {
> +		"nametype",
> +		"flag",
> +		"egress",
> +		"ingress"
> +	};

Similarly, it would be smaller if these were static const as well.
Also, if you were writing library code and compiling with -fPIC, there
would be some overhead in initializing the arrays regardless.

> +
> +	if ((argc < 3) || (argc > 6))

The test

   if (((unsigned int)(argc - 3)) > 3)

is equivalent, but smaller.

> +		show_usage();
> +
> +	if ((fd = open(conf_file_name, O_RDONLY)) < 0)	// Open up the /proc/vlan/config
> +		error_msg_and_die("open %s: %m\n", conf_file_name);
> +	close(fd);

It isn't really necessary to close fd.  Hence, storing fd isn't necessary
either.

> +
> +	if ((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0)	/* We use sockets now, instead of the file descriptor */
> +		error_msg_and_die("socket: %m\n");

It may generate smaller code if the socket call/fd assignment is closer
to the ioctl call.

> +
> +	memset(&ifr, 0, sizeof(struct vlan_ioctl_args));
> +
> +	for (cmd = 0; cmd < VLAN_CMD_HIGHEST; cmd++)
> +		if (!strcasecmp(vlan_cmd[cmd], argv[1]))
> +			break;

Since we're guaranteed to go through the loop at least once, it is
sometimes smaller to use a do {} while, depending on the test, etc.
Your mileage may vary but it is worth looking at.

> +
> +	if (cmd == VLAN_CMD_HIGHEST)
> +		show_usage();
> +
> +	if (cmd == cmd_set) {
> +
> +		if (argc < 4)
> +			show_usage();
> +		for (set_opt = 0; set_opt < VLAN_CMD_SET_OPT_HIGHEST; set_opt++)
> +			if (!strcasecmp(vlan_cmd_set_opt[set_opt], argv[2]))
> +				break;

By creating a "set" arg prefixing the nametype/flag/egress/ingress args,
you've made it necessary to use a second loop.  Consider removing "set".

> +
> +		if (set_opt == VLAN_CMD_SET_OPT_HIGHEST)
> +			show_usage();
> +		if (set_opt == set_nametype) {
> +			ifr.u.name_type = strtoul(argv[3], &endptr, 10);
> +			if ((ifr.u.name_type > VLAN_NAME_TYPE_HIGHEST)
> +				|| (*endptr != '\0'))
> +				show_usage();
> +			ifr.cmd = SET_VLAN_NAME_TYPE_CMD;
> +		} else {
> +			if (strlen(argv[3]) > IF_NAMESIZE)	// For those cases argv[3] is interface (physical or vlan-aware)
> +				error_msg_and_die("%s %d\n", int_len_msg, IF_NAMESIZE);
> +			strcpy(ifr.device1, argv[3]);
> +
> +			if (set_opt == set_flag) {
> +/*
> +				if (argc < 5)
> +					show_usage();
> +				ifr.u.flag = strtoul(argv[4], &endptr, 2);
> +				if ((ifr.u.flag > 1) || (*endptr != '\0'))
> +					show_usage();

The argc test, strtoul call, range check is repeated code.  In my
version, the needed argc is stored in the command table and I only
need to check twice... once to make sure there are at least 3 args,
and then once when the command is determined.

I also use a static function that does the conversion and range check
to consolidate things.

> +*/
> +				ifr.u.flag = 1;
> +				ifr.cmd = SET_VLAN_FLAG_CMD;
> +			} else {
> +				if (argc < 6)
> +					show_usage();
> +				ifr.u.skb_priority = strtoul(argv[4], &endptr, 10);
> +				if ((ifr.u.skb_priority > 0xFFFF) || (*endptr != '\0'))
> +					show_usage();
> +				ifr.vlan_qos = strtoul(argv[5], &endptr, 8);
> +				if ((ifr.vlan_qos > 7) || (*endptr != '\0'))
> +					show_usage();
> +				if (set_opt == set_egress)
> +					ifr.cmd = SET_VLAN_EGRESS_PRIORITY_CMD;
> +				if (set_opt == set_ingress)
> +					ifr.cmd = SET_VLAN_INGRESS_PRIORITY_CMD;

I also store the ifr.cmd value in the command table.  Hence I only need
one 'ifr.cmd = <value>;' statment.

> +			}
> +		}
> +	} else {
> +		// general check here, argv[2] is ifname    
> +		if (strlen(argv[2]) > IF_NAMESIZE)
> +			error_msg_and_die("%s %d\n", int_len_msg, IF_NAMESIZE);
> +		strcpy(ifr.device1, argv[2]);
> +		if (cmd == cmd_add) {
> +			ifr.cmd = ADD_VLAN_CMD;
> +			if (argc < 4)
> +				show_usage();
> +			ifr.u.VID = strtoul(argv[3], &endptr, 10);	// argv[3] is VID (0-4096)
> +			if ((ifr.u.VID >= VLAN_GROUP_ARRAY_LEN) || (ifr.u.VID < 0)
> +				|| (*endptr != '\0'))
> +				show_usage();
> +		}
> +		if (cmd == cmd_del)
> +			ifr.cmd = DEL_VLAN_CMD;
> +	};
> +	if (ioctl(fd, SIOCSIFVLAN, &ifr) < 0)
> +		error_msg_and_die("ioctl error when %s %s: %m\n", vlan_cmd[cmd],
> +						  argv[2]);
> +
> +	return 0;
> +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vconfig.c
Type: text/x-csrc
Size: 3766 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20030127/6360dc1e/attachment.c