[RFC net-next 0/3] bonding: new option API

[RFC net-next 0/3] bonding: new option API

[Nikolay Aleksandrov]

Hi,
This patchset aims to introduce a new option API that can be easily
extended if necessary and which attempts to remove some common problems
and code. In the beginning there was support for inter-option dependencies,
but that turned out to be unnecessary as the only 2 options that _enforce_
another option to be set prior to setting are up/down delay and they can be
easily re-worked to not require miimon to be set, so we can spare ourselves
100+ lines of checks, dealing with complex dependency errors and such.
In case this becomes necessary I've kept the old version of this patch-set
which has it, and can easily re-work it at any time.
There're still a lot of things to fix/clean but I've done some limited testing
with the options that are converted and it seems to work.
The main exported functions (as can be seen) are:
__bond_opt_set() - to be used when a string is passed which needs to be
                   converted in the case of BOND_OPTVAL_INTEGER. (sysfs)
__bond_opt_intset() - to be used when a value is passed to
                      BOND_OPTVAL_INTEGER (netlink), this function can't
                      be used for BOND_OPTVAL_STRING options
These two can be used from inside other options to stop them (e.g., arp_interval
stopping miimon and vice versa).
I've also added bond_opt_tryset_rtnl() mostly for sysfs use.
See the description of patch 01 and the comments inside for more information.

Value tables of converted options are no longer exported, and can be accessed
through the API (bond_opt_get_val() & bond_opt_get_flags).
Another good side-effect is that the error codes are standard for all options
for the common errors at least.

When/if this patchset is posted for inclusion, I'll have all options converted.
I actually had them before but while on vacation during December a lot of code
went in changing the bonding options and have to re-work most of the patches.

Some of the future plans for this are:
 Verbose outputting of dependencies (done, just have to polish it)
 Automatic sysfs generation from the bond_opts[].
 Use of the API in bond_check_params() and thus cleaning it up
 Structure for accessor fn parameter passing so we can implement get/set
 in a more general manner

Sending it with 2 options converted which illustrate the use of different
features of the API. I've tested them via sysfs.

Any thoughts, comments and suggestions are very welcome.

Thanks,
 Nik

CC: Andy Gospodarek <andy@greyhouse.net>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Veaceslav Falico <vfalico@redhat.com>
CC: David S. Miller <davem@davemloft.net>


Nikolay Aleksandrov (3):
  bonding: add infrastructure for an option API
  bonding: convert mode setting to use the new option API
  bonding: convert packets_per_slave to use the new option API

 drivers/net/bonding/bond_main.c    |  17 +-
 drivers/net/bonding/bond_netlink.c |   6 +-
 drivers/net/bonding/bond_options.c | 401 +++++++++++++++++++++++++++++++++----
 drivers/net/bonding/bond_options.h |  87 ++++++++
 drivers/net/bonding/bond_sysfs.c   |  44 ++--
 drivers/net/bonding/bonding.h      |   5 +-
 6 files changed, 473 insertions(+), 87 deletions(-)
 create mode 100644 drivers/net/bonding/bond_options.h

-- 
1.8.1.4

[RFC net-next 1/3] bonding: add infrastructure for an option API

[Nikolay Aleksandrov]

This patch adds the necessary basic infrastructure to support
centralized and unified option manipulation API for the bonding. The new
structure bond_option will be used to describe each option with its
dependencies on modes which will be checked automatically thus removing a
lot of duplicated code. Also automatic range checking is added for
non-string options. Currently the option setting function requires RTNL to
be acquired prior to calling it, since many options already rely on RTNL
it seemed like the best choice to protect all against common race
conditions.
In order to add an option the following steps need to be done:
1. Add an entry BOND_OPT_<option> to bond_options.h so it gets a unique id
   and a bit corresponding to the id
2. Add a bond_option entry to the bond_opts[] array in bond_options.c which
   describes the option, its dependencies and its manipulation function
3. Add code to export the option through sysfs and/or as a module parameter
   (the sysfs export will be made automatically in the future)

The current available option types are:
BOND_OPTVAL_INTEGER - range option, the flags should be used to define
                      it properly
BOND_OPTVAL_STRING - string option or an option which wants to do its
                     own validation and conversion since the buffer is
                     passed unmodified to the option manipulation function

The options can have different flags set, currently the following are
supported:
BOND_OPTFLAG_NOSLAVES - require that the bond device has no slaves prior
                        to setting the option
BOND_OPTFLAG_IFDOWN - require that the bond device is down prior to
                      setting the option

There's a new value structure to describe different types of values
which can have the following flags:
BOND_VALFLAG_DEFAULT - marks the default option (permanent string alias
                       to this option is "default")
BOND_VALFLAG_MIN - the minimum value that this option can have
BOND_VALFLAG_MAX - the maximum value that this option can have

An example would be nice here, so if we have an option which can have
the values "off"(2), "special"(4, default) and supports a range, say
16 - 32, it should be defined as follows:
"off", 2,
"special", 4, BOND_VALFLAG_DEFAULT,
"rangemin", 16, BOND_VALFLAG_MIN,
"rangemax", 32, BOND_VALFLAG_MAX
So we have the valid intervals: [2, 2], [4, 4], [16, 32]

BOND_VALFLAG_(MIN|MAX) can be used to specify a valid range for an
option, if MIN is omitted then 0 is considered as a minimum. If an
exact match is found in the values[] table it will be returned,
otherwise the range is tried (if available).
The values[] table which describes the allowed values for an option is
not mandatory for BOND_OPTVAL_STRING, but if it's present it will be
used.

For BOND_OPTVAL_INTEGER the values[] table is mandatory.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_options.c | 317 +++++++++++++++++++++++++++++++++++++
 drivers/net/bonding/bond_options.h |  82 ++++++++++
 drivers/net/bonding/bonding.h      |   1 +
 3 files changed, 400 insertions(+)
 create mode 100644 drivers/net/bonding/bond_options.h

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 945a666..7765efb 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -17,8 +17,325 @@
 #include <linux/rwlock.h>
 #include <linux/rcupdate.h>
 #include <linux/reciprocal_div.h>
+#include <linux/ctype.h>
 #include "bonding.h"
 
+static struct bond_option bond_opts[] = {
+	{ }
+};
+
+/* Searches for a value in opt's values[] table */
+struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val)
+{
+	struct bond_option *opt;
+	int i;
+
+	opt = bond_opt_get(option);
+	if (WARN_ON(!opt))
+		return NULL;
+	for (i = 0; opt->values && opt->values[i].name; i++)
+		if (opt->values[i].value == val)
+			return &opt->values[i];
+
+	return NULL;
+}
+
+/* Searches for a value in opt's values[] table which matches the flagmask */
+static struct bond_value_tbl *bond_opt_get_flags(const struct bond_option *opt,
+						 u32 flagmask)
+{
+	int i;
+
+	for (i = 0; opt->values && opt->values[i].name; i++)
+		if (opt->values[i].flags & flagmask)
+			return &opt->values[i];
+
+	return NULL;
+}
+
+static bool bond_opt_check_range(const struct bond_option *opt, int val)
+{
+	struct bond_value_tbl *minval, *maxval;
+
+	minval = bond_opt_get_flags(opt, BOND_VALFLAG_MIN);
+	maxval = bond_opt_get_flags(opt, BOND_VALFLAG_MAX);
+	if (val < 0 || (minval && val < minval->value) ||
+	    (maxval && val > maxval->value))
+		return false;
+
+	return true;
+}
+
+/**
+ * bond_opt_parse - parse option value
+ * @tbl: an option's values[] table to parse against
+ * @bufarg: value to parse
+ * @retval: pointer where to store the parsed value
+ * @string: how to treat bufarg (as string or integer)
+ *
+ * This functions tries to extract the value from bufarg and check if it's
+ * a possible match for the option. It shouldn't be possible to have a non-NULL
+ * return with @retval being < 0.
+ */
+struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
+				      void *bufarg, int *retval, bool string)
+{
+	char *buf, *p, valstr[BOND_MAX_MODENAME_LEN + 1] = { 0, };
+	struct bond_value_tbl *tbl, *maxval = NULL, *ret = NULL;
+	int valint = -1, i, rv;
+
+	tbl = opt->values;
+	if (!tbl)
+		goto out;
+
+	if (string) {
+		buf = bufarg;
+		for (p = (char *)buf; *p; p++)
+			if (!(isdigit(*p) || isspace(*p)))
+				break;
+
+		if (*p)
+			rv = sscanf(buf, "%20s", valstr);
+		else
+			rv = sscanf(buf, "%d", &valint);
+
+		if (!rv)
+			goto out;
+	} else {
+		valint = *(int *)bufarg;
+	}
+
+	for (i = 0; tbl[i].name; i++) {
+		/* Obtain maxval in case we don't match anything */
+		if (tbl[i].flags & BOND_VALFLAG_MAX)
+			maxval = &tbl[i];
+
+		/* Check for exact match */
+		if (!strcmp(valstr, "default") &&
+		    (tbl[i].flags & BOND_VALFLAG_DEFAULT))
+			ret = &tbl[i];
+		else if (valint == tbl[i].value)
+			ret = &tbl[i];
+		else if (!strcmp(valstr, tbl[i].name))
+			ret = &tbl[i];
+
+		/* Exact match */
+		if (ret) {
+			valint = ret->value;
+			goto out;
+		}
+	}
+	/* Possible range match */
+	if (bond_opt_check_range(opt, valint))
+		ret = maxval;
+out:
+	if (ret)
+		*retval = valint;
+	else
+		*retval = -ENOENT;
+
+	return ret;
+}
+
+/* Check opt's dependencies against bond mode and currently set options */
+static int bond_opt_check_deps(struct bonding *bond,
+			       const struct bond_option *opt)
+{
+	struct bond_params *params = &bond->params;
+
+	if (test_bit(params->mode, &opt->unsuppmodes))
+		return -EACCES;
+	if ((opt->flags & BOND_OPTFLAG_NOSLAVES) && bond_has_slaves(bond))
+		return -ENOTEMPTY;
+	if ((opt->flags & BOND_OPTFLAG_IFDOWN) && (bond->dev->flags & IFF_UP))
+		return -EBUSY;
+
+	return 0;
+}
+
+static void bond_opt_dep_print(struct bonding *bond,
+			       const struct bond_option *opt)
+{
+	struct bond_params *params;
+
+	params = &bond->params;
+	if (test_bit(params->mode, &opt->unsuppmodes))
+		pr_err("%s: option %s: mode dependency failed\n",
+		       bond->dev->name, opt->name);
+}
+
+static void bond_opt_error_interpret(struct bonding *bond,
+				     const struct bond_option *opt,
+				     int error, char *buf)
+{
+	struct bond_value_tbl *minval, *maxval;
+	char *p;
+
+	p = strchr(buf, '\n');
+	if (p)
+		*p = '\0';
+	switch (error) {
+	case -EINVAL:
+		pr_err("%s: option %s: invalid value (%s).\n",
+		       bond->dev->name, opt->name, buf);
+		if (opt->valtype == BOND_OPTVAL_STRING)
+			break;
+		minval = bond_opt_get_flags(opt, BOND_VALFLAG_MIN);
+		maxval = bond_opt_get_flags(opt, BOND_VALFLAG_MAX);
+		if (!maxval)
+			break;
+		pr_err("%s: option %s: allowed values %d - %d.\n",
+		       bond->dev->name, opt->name, minval ? minval->value : 0,
+		       maxval->value);
+		break;
+	case -EACCES:
+		bond_opt_dep_print(bond, opt);
+		break;
+	case -ENOTEMPTY:
+		pr_err("%s: option %s: unable to set because the bond has slaves.\n",
+		       bond->dev->name, opt->name);
+		break;
+	case -EBUSY:
+		pr_err("%s: option %s: unable to set because the bond device is up.\n",
+		       bond->dev->name, opt->name);
+		break;
+	default:
+		break;
+	}
+}
+
+static int bond_opt_call_intset(struct bonding *bond,
+				const struct bond_option *opt,
+				int newval)
+{
+	struct bond_value_tbl *valptr;
+	int value = newval;
+
+	if (opt->valtype != BOND_OPTVAL_INTEGER)
+		return -EINVAL;
+	/* Check value and store the result in it */
+	valptr = bond_opt_parse(opt, &value, &value, false);
+	if (!valptr)
+		return -EINVAL;
+
+	return opt->set(bond, &value);
+}
+
+/**
+ * bond_opt_call_set - convert buf and check the value based on opt->valtype
+ * @bond: bond device to set on
+ * @opt: option to set
+ * @buf: value to set the option to
+ *
+ * This function converts buf to the appropriate value based on the opt's
+ * valtype.
+ */
+static int bond_opt_call_set(struct bonding *bond,
+			     const struct bond_option *opt,
+			     char *buf)
+{
+	struct bond_value_tbl *valptr;
+	int value = -1;
+
+	if (opt->valtype == BOND_OPTVAL_INTEGER) {
+		valptr = bond_opt_parse(opt, buf, &value, true);
+		if (!valptr)
+			return -EINVAL;
+		return opt->set(bond, &value);
+	} else {
+		return opt->set(bond, buf);
+	}
+}
+
+/**
+ * __bond_opt_set - set a bonding option
+ * @bond: target bond device
+ * @option: option to set
+ * @buf: value to set it to
+ *
+ * This function is used to change the bond's option value to newval, it can be
+ * used for both enabling/changing an option and for disabling it. RTNL lock
+ * must be obtained before calling this function.
+ */
+int __bond_opt_set(struct bonding *bond, unsigned int option, char *buf)
+{
+	const struct bond_option *opt;
+	int ret = -ENOENT;
+
+	ASSERT_RTNL();
+
+	opt = bond_opt_get(option);
+	if (WARN_ON(!buf) || WARN_ON(!opt))
+		goto out;
+	ret = bond_opt_check_deps(bond, opt);
+	if (ret)
+		goto out;
+	ret = bond_opt_call_set(bond, opt, buf);
+out:
+	if (ret)
+		bond_opt_error_interpret(bond, opt, ret, buf);
+
+	return ret;
+}
+
+/* See comment for __bond_opt_set. This function is used to set a
+ * BOND_OPTVAL_INTEGER option to a value directly without string parsing. The
+ * value is still checked against the option's values[] table.
+ */
+int __bond_opt_intset(struct bonding *bond, unsigned int option, int value)
+{
+	const struct bond_option *opt;
+	int ret = -ENOENT;
+
+	ASSERT_RTNL();
+
+	opt = bond_opt_get(option);
+	if (WARN_ON(!opt))
+		return ret;
+	ret = bond_opt_check_deps(bond, opt);
+	if (ret)
+		return ret;
+	ret = bond_opt_call_intset(bond, opt, value);
+
+	return ret;
+}
+
+/**
+ * bond_opt_tryset_rtnl - try to acquire rtnl and call __bond_opt_set
+ * @bond: target bond device
+ * @option: option to set
+ * @buf: value to set it to
+ *
+ * This function tries to acquire RTNL without blocking and if successful
+ * calls __bond_opt_set, otherwise returns -EAGAIN to notify the caller.
+ */
+int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf)
+{
+	int ret;
+
+	if (!rtnl_trylock())
+		return -EAGAIN;
+	ret = __bond_opt_set(bond, option, buf);
+	rtnl_unlock();
+
+	return ret;
+}
+
+/**
+ * bond_opt_get - get a pointer to an option
+ * @option: option for which to return a pointer
+ *
+ * This function checks if option is valid and if so returns a pointer
+ * to its entry in the bond_opts[] option array.
+ */
+struct bond_option *bond_opt_get(unsigned int option)
+{
+	if (!BOND_OPT_VALID(option))
+		return NULL;
+
+	return &bond_opts[option];
+}
+
 int bond_option_mode_set(struct bonding *bond, int mode)
 {
 	if (bond_parm_tbl_lookup(mode, bond_mode_tbl) < 0) {
diff --git a/drivers/net/bonding/bond_options.h b/drivers/net/bonding/bond_options.h
new file mode 100644
index 0000000..c72ecec
--- /dev/null
+++ b/drivers/net/bonding/bond_options.h
@@ -0,0 +1,82 @@
+/*
+ * drivers/net/bond/bond_options.h - bonding options
+ * Copyright (c) 2013 Nikolay Aleksandrov <nikolay@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _BOND_OPTIONS_H
+#define _BOND_OPTIONS_H
+
+#define BOND_OPT_VALID(opt) ((opt) < BOND_OPT_LAST)
+#define BOND_MODE_ALL_EX(x) (~(x))
+
+/* Option value types */
+enum {
+	BOND_OPTVAL_INTEGER,
+	BOND_OPTVAL_STRING
+};
+
+/* Option flags:
+ * BOND_OPTFLAG_NOSLAVES - check if the bond device is empty before setting
+ * BOND_OPTFLAG_IFDOWN - check if the bond device is down before setting
+ */
+enum {
+	BOND_OPTFLAG_NOSLAVES	= BIT(0),
+	BOND_OPTFLAG_IFDOWN	= BIT(1)
+};
+
+/* Value type flags:
+ * BOND_VALFLAG_DEFAULT - mark the value as default
+ * BOND_VALFLAG_(MIN|MAX) - mark the value as min/max
+ */
+enum {
+	BOND_VALFLAG_DEFAULT	= BIT(0),
+	BOND_VALFLAG_MIN	= BIT(1),
+	BOND_VALFLAG_MAX	= BIT(2)
+};
+
+/* Option IDs, their bit positions correspond to their IDs */
+enum {
+	BOND_OPT_LAST
+};
+
+struct bond_value_tbl {
+	char *name;
+	int value;
+	u32 flags;
+};
+
+struct bonding;
+
+struct bond_option {
+	int id;
+	char *name;
+	char *desc;
+	u32 flags;
+	u8 valtype;
+
+	/* unsuppmodes is used to denote modes in which the option isn't
+	 * supported.
+	 */
+	unsigned long unsuppmodes;
+	/* supported values which this option can have, can be a subset of
+	 * BOND_OPTVAL_RANGE's value range
+	 */
+	struct bond_value_tbl *values;
+
+	int (*set)(struct bonding *bond, void *newval);
+};
+
+void bond_opt_init(void);
+int __bond_opt_set(struct bonding *bond, unsigned int option, char *buf);
+int __bond_opt_intset(struct bonding *bond, unsigned int option, int value);
+int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf);
+struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
+				      void *bufarg, int *retval, bool string);
+struct bond_option *bond_opt_get(unsigned int option);
+struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val);
+#endif /* _BOND_OPTIONS_H */
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 955dc48..71e751a 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -25,6 +25,7 @@
 #include <linux/etherdevice.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
+#include "bond_options.h"
 
 #define DRV_VERSION	"3.7.1"
 #define DRV_RELDATE	"April 27, 2011"
-- 
1.8.1.4

[RFC net-next 2/3] bonding: convert mode setting to use the new option API

[Nikolay Aleksandrov]

This patch makes the bond's mode setting use the new option API and
adds support for dependency printing which relies on having an entry for
the mode option in the bond_opts[] array.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c    | 17 ++++----------
 drivers/net/bonding/bond_netlink.c |  2 +-
 drivers/net/bonding/bond_options.c | 45 ++++++++++++++++++++++----------------
 drivers/net/bonding/bond_options.h |  3 +++
 drivers/net/bonding/bond_sysfs.c   | 27 +++++++----------------
 drivers/net/bonding/bonding.h      |  2 --
 6 files changed, 42 insertions(+), 54 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..e102096 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -215,17 +215,6 @@ const struct bond_parm_tbl bond_lacp_tbl[] = {
 {	NULL,		-1},
 };
 
-const struct bond_parm_tbl bond_mode_tbl[] = {
-{	"balance-rr",		BOND_MODE_ROUNDROBIN},
-{	"active-backup",	BOND_MODE_ACTIVEBACKUP},
-{	"balance-xor",		BOND_MODE_XOR},
-{	"broadcast",		BOND_MODE_BROADCAST},
-{	"802.3ad",		BOND_MODE_8023AD},
-{	"balance-tlb",		BOND_MODE_TLB},
-{	"balance-alb",		BOND_MODE_ALB},
-{	NULL,			-1},
-};
-
 const struct bond_parm_tbl xmit_hashtype_tbl[] = {
 {	"layer2",		BOND_XMIT_POLICY_LAYER2},
 {	"layer3+4",		BOND_XMIT_POLICY_LAYER34},
@@ -3982,14 +3971,16 @@ int bond_parse_parm(const char *buf, const struct bond_parm_tbl *tbl)
 static int bond_check_params(struct bond_params *params)
 {
 	int arp_validate_value, fail_over_mac_value, primary_reselect_value, i;
+	struct bond_value_tbl *valptr;
 	int arp_all_targets_value;
 
 	/*
 	 * Convert string parameters.
 	 */
 	if (mode) {
-		bond_mode = bond_parse_parm(mode, bond_mode_tbl);
-		if (bond_mode == -1) {
+		valptr = bond_opt_parse(bond_opt_get(BOND_OPT_MODE), mode,
+					&bond_mode, true);
+		if (!valptr) {
 			pr_err("Error: Invalid bonding mode \"%s\"\n",
 			       mode == NULL ? "NULL" : mode);
 			return -EINVAL;
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 555c783..51a9be3 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -72,7 +72,7 @@ static int bond_changelink(struct net_device *bond_dev,
 	if (data[IFLA_BOND_MODE]) {
 		int mode = nla_get_u8(data[IFLA_BOND_MODE]);
 
-		err = bond_option_mode_set(bond, mode);
+		err = __bond_opt_intset(bond, BOND_OPT_MODE, mode);
 		if (err)
 			return err;
 	}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 7765efb..4aa752f 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -20,7 +20,27 @@
 #include <linux/ctype.h>
 #include "bonding.h"
 
+static struct bond_value_tbl bond_mode_tbl[] = {
+	{ "balance-rr",    BOND_MODE_ROUNDROBIN,   BOND_VALFLAG_DEFAULT},
+	{ "active-backup", BOND_MODE_ACTIVEBACKUP, 0},
+	{ "balance-xor",   BOND_MODE_XOR,          0},
+	{ "broadcast",     BOND_MODE_BROADCAST,    0},
+	{ "802.3ad",       BOND_MODE_8023AD,       0},
+	{ "balance-tlb",   BOND_MODE_TLB,          0},
+	{ "balance-alb",   BOND_MODE_ALB,          0},
+	{ NULL,            -1,                     0},
+};
+
 static struct bond_option bond_opts[] = {
+	[BOND_OPT_MODE] = {
+		.id = BOND_OPT_MODE,
+		.name = "mode",
+		.desc = "bond device mode",
+		.flags = BOND_OPTFLAG_NOSLAVES | BOND_OPTFLAG_IFDOWN,
+		.valtype = BOND_OPTVAL_INTEGER,
+		.values = bond_mode_tbl,
+		.set = bond_option_mode_set
+	},
 	{ }
 };
 
@@ -336,29 +356,15 @@ struct bond_option *bond_opt_get(unsigned int option)
 	return &bond_opts[option];
 }
 
-int bond_option_mode_set(struct bonding *bond, int mode)
+int bond_option_mode_set(struct bonding *bond, void *newval)
 {
-	if (bond_parm_tbl_lookup(mode, bond_mode_tbl) < 0) {
-		pr_err("%s: Ignoring invalid mode value %d.\n",
-		       bond->dev->name, mode);
-		return -EINVAL;
-	}
-
-	if (bond->dev->flags & IFF_UP) {
-		pr_err("%s: unable to update mode because interface is up.\n",
-		       bond->dev->name);
-		return -EPERM;
-	}
-
-	if (bond_has_slaves(bond)) {
-		pr_err("%s: unable to update mode because bond has slaves.\n",
-			bond->dev->name);
-		return -EPERM;
-	}
+	int mode = *(int *)newval;
+	struct bond_option *opt;
 
 	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
+		opt = bond_opt_get(BOND_OPT_MODE);
 		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
-			bond->dev->name, bond_mode_tbl[mode].modename);
+			bond->dev->name, opt->values[mode].name);
 		/* disable arp monitoring */
 		bond->params.arp_interval = 0;
 		/* set miimon to default value */
@@ -370,6 +376,7 @@ int bond_option_mode_set(struct bonding *bond, int mode)
 	/* don't cache arp_validate between modes */
 	bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
 	bond->params.mode = mode;
+
 	return 0;
 }
 
diff --git a/drivers/net/bonding/bond_options.h b/drivers/net/bonding/bond_options.h
index c72ecec..12c4d4f 100644
--- a/drivers/net/bonding/bond_options.h
+++ b/drivers/net/bonding/bond_options.h
@@ -41,6 +41,7 @@ enum {
 
 /* Option IDs, their bit positions correspond to their IDs */
 enum {
+	BOND_OPT_MODE,
 	BOND_OPT_LAST
 };
 
@@ -79,4 +80,6 @@ struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
 				      void *bufarg, int *retval, bool string);
 struct bond_option *bond_opt_get(unsigned int option);
 struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val);
+
+int bond_option_mode_set(struct bonding *bond, void *newval);
 #endif /* _BOND_OPTIONS_H */
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 011f163..8ce5f4f 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -264,37 +264,26 @@ static ssize_t bonding_show_mode(struct device *d,
 				 struct device_attribute *attr, char *buf)
 {
 	struct bonding *bond = to_bond(d);
+	struct bond_value_tbl *val;
 
-	return sprintf(buf, "%s %d\n",
-			bond_mode_tbl[bond->params.mode].modename,
-			bond->params.mode);
+	val = bond_opt_get_val(BOND_OPT_MODE, bond->params.mode);
+
+	return sprintf(buf, "%s %d\n", val->name, bond->params.mode);
 }
 
 static ssize_t bonding_store_mode(struct device *d,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
 {
-	int new_value, ret;
 	struct bonding *bond = to_bond(d);
+	int ret;
 
-	new_value = bond_parse_parm(buf, bond_mode_tbl);
-	if (new_value < 0)  {
-		pr_err("%s: Ignoring invalid mode value %.*s.\n",
-		       bond->dev->name, (int)strlen(buf) - 1, buf);
-		return -EINVAL;
-	}
-	if (!rtnl_trylock())
+	ret = bond_opt_tryset_rtnl(bond, BOND_OPT_MODE, (char *)buf);
+	if (ret == -EAGAIN)
 		return restart_syscall();
-
-	ret = bond_option_mode_set(bond, new_value);
-	if (!ret) {
-		pr_info("%s: setting mode to %s (%d).\n",
-			bond->dev->name, bond_mode_tbl[new_value].modename,
-			new_value);
+	else if (!ret)
 		ret = count;
-	}
 
-	rtnl_unlock();
 	return ret;
 }
 static DEVICE_ATTR(mode, S_IRUGO | S_IWUSR,
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 71e751a..50bbd28 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -439,7 +439,6 @@ void bond_setup(struct net_device *bond_dev);
 unsigned int bond_get_num_tx_queues(void);
 int bond_netlink_init(void);
 void bond_netlink_fini(void);
-int bond_option_mode_set(struct bonding *bond, int mode);
 int bond_option_active_slave_set(struct bonding *bond, struct net_device *slave_dev);
 int bond_option_miimon_set(struct bonding *bond, int miimon);
 int bond_option_updelay_set(struct bonding *bond, int updelay);
@@ -549,7 +548,6 @@ static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
 /* exported from bond_main.c */
 extern int bond_net_id;
 extern const struct bond_parm_tbl bond_lacp_tbl[];
-extern const struct bond_parm_tbl bond_mode_tbl[];
 extern const struct bond_parm_tbl xmit_hashtype_tbl[];
 extern const struct bond_parm_tbl arp_validate_tbl[];
 extern const struct bond_parm_tbl arp_all_targets_tbl[];
-- 
1.8.1.4

[RFC net-next 3/3] bonding: convert packets_per_slave to use the new option API

[Nikolay Aleksandrov]

This patch adds the necessary changes so packets_per_slave would use the
new bonding option API.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_netlink.c |  4 ++--
 drivers/net/bonding/bond_options.c | 49 +++++++++++++++++++++-----------------
 drivers/net/bonding/bond_options.h |  2 ++
 drivers/net/bonding/bond_sysfs.c   | 17 ++++---------
 drivers/net/bonding/bonding.h      |  2 --
 5 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 51a9be3..ee01d8f 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -250,8 +250,8 @@ static int bond_changelink(struct net_device *bond_dev,
 		int packets_per_slave =
 			nla_get_u32(data[IFLA_BOND_PACKETS_PER_SLAVE]);
 
-		err = bond_option_packets_per_slave_set(bond,
-							packets_per_slave);
+		err = __bond_opt_intset(bond, BOND_OPT_PACKETS_PER_SLAVE,
+					packets_per_slave);
 		if (err)
 			return err;
 	}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 4aa752f..88b5338 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -31,6 +31,12 @@ static struct bond_value_tbl bond_mode_tbl[] = {
 	{ NULL,            -1,                     0},
 };
 
+static struct bond_value_tbl bond_pps_tbl[] = {
+	{ "default", 1,         BOND_VALFLAG_DEFAULT},
+	{ "maxval",  USHRT_MAX, BOND_VALFLAG_MAX},
+	{ NULL,      -1,        0},
+};
+
 static struct bond_option bond_opts[] = {
 	[BOND_OPT_MODE] = {
 		.id = BOND_OPT_MODE,
@@ -41,6 +47,15 @@ static struct bond_option bond_opts[] = {
 		.values = bond_mode_tbl,
 		.set = bond_option_mode_set
 	},
+	[BOND_OPT_PACKETS_PER_SLAVE] = {
+		.id = BOND_OPT_PACKETS_PER_SLAVE,
+		.name = "packets_per_slave",
+		.desc = "Packets to send per slave in RR mode",
+		.valtype = BOND_OPTVAL_INTEGER,
+		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ROUNDROBIN)),
+		.values = bond_pps_tbl,
+		.set = bond_option_pps_set
+	},
 	{ }
 };
 
@@ -982,28 +997,6 @@ int bond_option_lp_interval_set(struct bonding *bond, int lp_interval)
 	return 0;
 }
 
-int bond_option_packets_per_slave_set(struct bonding *bond,
-				      int packets_per_slave)
-{
-	if (packets_per_slave < 0 || packets_per_slave > USHRT_MAX) {
-		pr_err("%s: packets_per_slave must be between 0 and %u\n",
-		       bond->dev->name, USHRT_MAX);
-		return -EINVAL;
-	}
-
-	if (bond->params.mode != BOND_MODE_ROUNDROBIN)
-		pr_warn("%s: Warning: packets_per_slave has effect only in balance-rr mode\n",
-			bond->dev->name);
-
-	if (packets_per_slave > 1)
-		bond->params.packets_per_slave =
-			reciprocal_value(packets_per_slave);
-	else
-		bond->params.packets_per_slave = packets_per_slave;
-
-	return 0;
-}
-
 int bond_option_lacp_rate_set(struct bonding *bond, int lacp_rate)
 {
 	if (bond_parm_tbl_lookup(lacp_rate, bond_lacp_tbl) < 0) {
@@ -1054,3 +1047,15 @@ int bond_option_ad_select_set(struct bonding *bond, int ad_select)
 
 	return 0;
 }
+
+int bond_option_pps_set(struct bonding *bond, void *newval)
+{
+	int *new_value = newval;
+
+	if (*new_value > 1)
+		bond->params.packets_per_slave = reciprocal_value(*new_value);
+	else
+		bond->params.packets_per_slave = *new_value;
+
+	return 0;
+}
diff --git a/drivers/net/bonding/bond_options.h b/drivers/net/bonding/bond_options.h
index 12c4d4f..9a3a647 100644
--- a/drivers/net/bonding/bond_options.h
+++ b/drivers/net/bonding/bond_options.h
@@ -42,6 +42,7 @@ enum {
 /* Option IDs, their bit positions correspond to their IDs */
 enum {
 	BOND_OPT_MODE,
+	BOND_OPT_PACKETS_PER_SLAVE,
 	BOND_OPT_LAST
 };
 
@@ -82,4 +83,5 @@ struct bond_option *bond_opt_get(unsigned int option);
 struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val);
 
 int bond_option_mode_set(struct bonding *bond, void *newval);
+int bond_option_pps_set(struct bonding *bond, void *newval);
 #endif /* _BOND_OPTIONS_H */
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 8ce5f4f..2de1bab 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1375,22 +1375,15 @@ static ssize_t bonding_store_packets_per_slave(struct device *d,
 					       const char *buf, size_t count)
 {
 	struct bonding *bond = to_bond(d);
-	int new_value, ret;
-
-	if (sscanf(buf, "%d", &new_value) != 1) {
-		pr_err("%s: no packets_per_slave value specified.\n",
-		       bond->dev->name);
-		return -EINVAL;
-	}
+	int ret;
 
-	if (!rtnl_trylock())
+	ret = bond_opt_tryset_rtnl(bond, BOND_OPT_PACKETS_PER_SLAVE,
+				   (char *)buf);
+	if (ret == -EAGAIN)
 		return restart_syscall();
-
-	ret = bond_option_packets_per_slave_set(bond, new_value);
-	if (!ret)
+	else if (!ret)
 		ret = count;
 
-	rtnl_unlock();
 	return ret;
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 50bbd28..88cd7d7 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -463,8 +463,6 @@ int bond_option_all_slaves_active_set(struct bonding *bond,
 				      int all_slaves_active);
 int bond_option_min_links_set(struct bonding *bond, int min_links);
 int bond_option_lp_interval_set(struct bonding *bond, int min_links);
-int bond_option_packets_per_slave_set(struct bonding *bond,
-				      int packets_per_slave);
 int bond_option_lacp_rate_set(struct bonding *bond, int lacp_rate);
 int bond_option_ad_select_set(struct bonding *bond, int ad_select);
 struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
-- 
1.8.1.4

Re: [RFC net-next 1/3] bonding: add infrastructure for an option API

[Stephen Hemminger]

On Fri, 10 Jan 2014 14:11:33 +0100
Nikolay Aleksandrov <nikolay@redhat.com> wrote:

> This patch adds the necessary basic infrastructure to support
> centralized and unified option manipulation API for the bonding. The new
> structure bond_option will be used to describe each option with its
> dependencies on modes which will be checked automatically thus removing a
> lot of duplicated code. Also automatic range checking is added for
> non-string options. Currently the option setting function requires RTNL to
> be acquired prior to calling it, since many options already rely on RTNL
> it seemed like the best choice to protect all against common race
> conditions.
> In order to add an option the following steps need to be done:
> 1. Add an entry BOND_OPT_<option> to bond_options.h so it gets a unique id
>    and a bit corresponding to the id
> 2. Add a bond_option entry to the bond_opts[] array in bond_options.c which
>    describes the option, its dependencies and its manipulation function
> 3. Add code to export the option through sysfs and/or as a module parameter
>    (the sysfs export will be made automatically in the future)
> 
> The current available option types are:
> BOND_OPTVAL_INTEGER - range option, the flags should be used to define
>                       it properly
> BOND_OPTVAL_STRING - string option or an option which wants to do its
>                      own validation and conversion since the buffer is
>                      passed unmodified to the option manipulation function
> 
> The options can have different flags set, currently the following are
> supported:
> BOND_OPTFLAG_NOSLAVES - require that the bond device has no slaves prior
>                         to setting the option
> BOND_OPTFLAG_IFDOWN - require that the bond device is down prior to
>                       setting the option
> 
> There's a new value structure to describe different types of values
> which can have the following flags:
> BOND_VALFLAG_DEFAULT - marks the default option (permanent string alias
>                        to this option is "default")
> BOND_VALFLAG_MIN - the minimum value that this option can have
> BOND_VALFLAG_MAX - the maximum value that this option can have
> 
> An example would be nice here, so if we have an option which can have
> the values "off"(2), "special"(4, default) and supports a range, say
> 16 - 32, it should be defined as follows:
> "off", 2,
> "special", 4, BOND_VALFLAG_DEFAULT,
> "rangemin", 16, BOND_VALFLAG_MIN,
> "rangemax", 32, BOND_VALFLAG_MAX
> So we have the valid intervals: [2, 2], [4, 4], [16, 32]
> 
> BOND_VALFLAG_(MIN|MAX) can be used to specify a valid range for an
> option, if MIN is omitted then 0 is considered as a minimum. If an
> exact match is found in the values[] table it will be returned,
> otherwise the range is tried (if available).
> The values[] table which describes the allowed values for an option is
> not mandatory for BOND_OPTVAL_STRING, but if it's present it will be
> used.
> 
> For BOND_OPTVAL_INTEGER the values[] table is mandatory.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Please add new features via netlink not sysfs.
Sysfs is awkward API for command line, and not accessible through ip link.
Also it provides no notification of changes or configuration back to switches
and other things using netlink.

Re: [RFC net-next 1/3] bonding: add infrastructure for an option API

[Scott Feldman]

On Jan 10, 2014, at 11:19 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Fri, 10 Jan 2014 14:11:33 +0100
> Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> 
>> This patch adds the necessary basic infrastructure to support
>> centralized and unified option manipulation API for the bonding. The new
>> structure bond_option will be used to describe each option with its
>> dependencies on modes which will be checked automatically thus removing a
>> lot of duplicated code. Also automatic range checking is added for
>> non-string options. Currently the option setting function requires RTNL to
>> be acquired prior to calling it, since many options already rely on RTNL
>> it seemed like the best choice to protect all against common race
>> conditions.
> 
> Please add new features via netlink not sysfs.
> Sysfs is awkward API for command line, and not accessible through ip link.
> Also it provides no notification of changes or configuration back to switches
> and other things using netlink.

I don’t think Nikolay is adding a new bonding feature.  This patch series is a (very nice!) cleanup of option handling in the bonding driver.  The recently added netlink interface to bonding options is still intact.  This patch moves all the parsing/setting/getting/locking/bounds_checking/dependency_checking into one nice clean API.

I’m still reviewing patch set and might have more comments, but the first comment is: can this be generalized for more than just bonding?  It seems lots of drivers which maintain an option set could benefit from a similar API.

-scott

Re: [RFC net-next 3/3] bonding: convert packets_per_slave to use the new option API

[Scott Feldman]

On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:

> This patch adds the necessary changes so packets_per_slave would use the
> new bonding option API.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
> +	[BOND_OPT_PACKETS_PER_SLAVE] = {
> +		.id = BOND_OPT_PACKETS_PER_SLAVE,
> +		.name = "packets_per_slave",
> +		.desc = "Packets to send per slave in RR mode",
> +		.valtype = BOND_OPTVAL_INTEGER,
> +		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ROUNDROBIN)),

This .unsuppmodes would be hard to generalize as-is since it’s bond-specific.

(*set)() could have done the check also.  Maybe there is a (*check)() func to do pre-screening of value before (*set) is called, but after general checks are made?

> +		.values = bond_pps_tbl,
> +		.set = bond_option_pps_set
> +	},
> 	{ }
> };
> 
> @@ -982,28 +997,6 @@ int bond_option_lp_interval_set(struct bonding *bond, int lp_interval)
> 	return 0;
> }
> 
> -int bond_option_packets_per_slave_set(struct bonding *bond,
> -				      int packets_per_slave)
> -{
> -	if (packets_per_slave < 0 || packets_per_slave > USHRT_MAX) {
> -		pr_err("%s: packets_per_slave must be between 0 and %u\n",
> -		       bond->dev->name, USHRT_MAX);
> -		return -EINVAL;
> -	}
> -
> -	if (bond->params.mode != BOND_MODE_ROUNDROBIN)
> -		pr_warn("%s: Warning: packets_per_slave has effect only in balance-rr mode\n",
> -			bond->dev->name);
> -
> -	if (packets_per_slave > 1)
> -		bond->params.packets_per_slave =
> -			reciprocal_value(packets_per_slave);
> -	else
> -		bond->params.packets_per_slave = packets_per_slave;
> -
> -	return 0;
> -}
> -

Nice!  Good bye rambling code.

Re: [RFC net-next 2/3] bonding: convert mode setting to use the new option API

[Scott Feldman]

On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:

> This patch makes the bond's mode setting use the new option API and
> adds support for dependency printing which relies on having an entry for
> the mode option in the bond_opts[] array.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

> -int bond_option_mode_set(struct bonding *bond, int mode)
> +int bond_option_mode_set(struct bonding *bond, void *newval)
> {
> -	if (bond_parm_tbl_lookup(mode, bond_mode_tbl) < 0) {
> -		pr_err("%s: Ignoring invalid mode value %d.\n",
> -		       bond->dev->name, mode);
> -		return -EINVAL;
> -	}
> -
> -	if (bond->dev->flags & IFF_UP) {
> -		pr_err("%s: unable to update mode because interface is up.\n",
> -		       bond->dev->name);
> -		return -EPERM;
> -	}
> -
> -	if (bond_has_slaves(bond)) {
> -		pr_err("%s: unable to update mode because bond has slaves.\n",
> -			bond->dev->name);
> -		return -EPERM;
> -	}
> +	int mode = *(int *)newval;
> +	struct bond_option *opt;
> 
> 	if (BOND_NO_USES_ARP(mode) && bond->params.arp_interval) {
> +		opt = bond_opt_get(BOND_OPT_MODE);
> 		pr_info("%s: %s mode is incompatible with arp monitoring, start mii monitoring\n",
> -			bond->dev->name, bond_mode_tbl[mode].modename);
> +			bond->dev->name, opt->values[mode].name);

Can this be a name accessor func?          ^^^^^^^^^^^^^^^^^^^^^^

> static ssize_t bonding_store_mode(struct device *d,
> 				  struct device_attribute *attr,
> 				  const char *buf, size_t count)
> {
> -	int new_value, ret;
> 	struct bonding *bond = to_bond(d);
> +	int ret;
> 
> -	new_value = bond_parse_parm(buf, bond_mode_tbl);
> -	if (new_value < 0)  {
> -		pr_err("%s: Ignoring invalid mode value %.*s.\n",
> -		       bond->dev->name, (int)strlen(buf) - 1, buf);
> -		return -EINVAL;
> -	}
> -	if (!rtnl_trylock())
> +	ret = bond_opt_tryset_rtnl(bond, BOND_OPT_MODE, (char *)buf);
> +	if (ret == -EAGAIN)
> 		return restart_syscall();

I wonder if the restart_syscall() should be kept close to rtnl_trylock() in bond_opt_tryset_rtnl?

Re: [RFC net-next 1/3] bonding: add infrastructure for an option API

[Scott Feldman]

On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:

> This patch adds the necessary basic infrastructure to support
> centralized and unified option manipulation API for the bonding. The new
> structure bond_option will be used to describe each option with its
> dependencies on modes which will be checked automatically thus removing a
> lot of duplicated code. Also automatic range checking is added for
> non-string options. Currently the option setting function requires RTNL to
> be acquired prior to calling it, since many options already rely on RTNL
> it seemed like the best choice to protect all against common race
> conditions.
> 


> +/**
> + * bond_opt_parse - parse option value
> + * @tbl: an option's values[] table to parse against
> + * @bufarg: value to parse
> + * @retval: pointer where to store the parsed value
> + * @string: how to treat bufarg (as string or integer)
> + *
> + * This functions tries to extract the value from bufarg and check if it's
> + * a possible match for the option. It shouldn't be possible to have a non-NULL
> + * return with @retval being < 0.
> + */
> +struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
> +				      void *bufarg, int *retval, bool string)
> +{
> +	char *buf, *p, valstr[BOND_MAX_MODENAME_LEN + 1] = { 0, };

Did you mean to use BOND_MAX_MODENAME_LEN here?  bond_options.h should have it’s own define max value string len, I think.

> +	struct bond_value_tbl *tbl, *maxval = NULL, *ret = NULL;
> +	int valint = -1, i, rv;
> +
> +	tbl = opt->values;
> +	if (!tbl)
> +		goto out;
> +
> +	if (string) {
> +		buf = bufarg;
> +		for (p = (char *)buf; *p; p++)
> +			if (!(isdigit(*p) || isspace(*p)))
> +				break;

I think we lost a comment here from old code explaining eating whitespace and trailing newline.

> 
> +struct bond_value_tbl {
> +	char *name;
> +	int value;

What if want a u64 value?

> +	u32 flags;
> +};

struct bond_value_tbl_entry ?  Or just struct bond_value?  tbl is a collection of these, so it’s weird to have tbl in name of item.

-scott

Re: [RFC net-next 0/3] bonding: new option API

[Scott Feldman]

On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:

> Hi,
> This patchset aims to introduce a new option API that can be easily
> extended if necessary and which attempts to remove some common problems
> and code. In the beginning there was support for inter-option dependencies,
> but that turned out to be unnecessary as the only 2 options that _enforce_
> another option to be set prior to setting are up/down delay and they can be
> easily re-worked to not require miimon to be set, so we can spare ourselves
> 100+ lines of checks, dealing with complex dependency errors and such.
> In case this becomes necessary I've kept the old version of this patch-set
> which has it, and can easily re-work it at any time.
> There're still a lot of things to fix/clean but I've done some limited testing
> with the options that are converted and it seems to work.
> The main exported functions (as can be seen) are:
> __bond_opt_set() - to be used when a string is passed which needs to be
>                   converted in the case of BOND_OPTVAL_INTEGER. (sysfs)
> __bond_opt_intset() - to be used when a value is passed to
>                      BOND_OPTVAL_INTEGER (netlink), this function can't
>                      be used for BOND_OPTVAL_STRING options
> These two can be used from inside other options to stop them (e.g., arp_interval
> stopping miimon and vice versa).
> I've also added bond_opt_tryset_rtnl() mostly for sysfs use.
> See the description of patch 01 and the comments inside for more information.
> 
> Value tables of converted options are no longer exported, and can be accessed
> through the API (bond_opt_get_val() & bond_opt_get_flags).
> Another good side-effect is that the error codes are standard for all options
> for the common errors at least.

Nice!

> When/if this patchset is posted for inclusion, I'll have all options converted.
> I actually had them before but while on vacation during December a lot of code
> went in changing the bonding options and have to re-work most of the patches.

Oops, sorry about that ;)

> Some of the future plans for this are:
> Verbose outputting of dependencies (done, just have to polish it)
> Automatic sysfs generation from the bond_opts[].

I had a patch in my queue to do something similar, but yours is so much nicer.  

For sysfs nodes, there is a file permission. I wonder if bond_opts should have a sense of RO, RW, or WO?  Then automatic sysfs generation is even easier.  Hmmm, actually I think the answer is no.  Nevermind.

> Use of the API in bond_check_params() and thus cleaning it up
> Structure for accessor fn parameter passing so we can implement get/set
> in a more general manner
> 
> Sending it with 2 options converted which illustrate the use of different
> features of the API. I've tested them via sysfs.
> 
> Any thoughts, comments and suggestions are very welcome.

Nice job Nik, well done.

-scott

Re: [RFC net-next 0/3] bonding: new option API

[Nikolay Aleksandrov]

On 01/10/2014 09:32 PM, Scott Feldman wrote:
> 
> On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> 
>> Hi,
>> This patchset aims to introduce a new option API that can be easily
>> extended if necessary and which attempts to remove some common problems
>> and code. In the beginning there was support for inter-option dependencies,
>> but that turned out to be unnecessary as the only 2 options that _enforce_
>> another option to be set prior to setting are up/down delay and they can be
>> easily re-worked to not require miimon to be set, so we can spare ourselves
>> 100+ lines of checks, dealing with complex dependency errors and such.
>> In case this becomes necessary I've kept the old version of this patch-set
>> which has it, and can easily re-work it at any time.
>> There're still a lot of things to fix/clean but I've done some limited testing
>> with the options that are converted and it seems to work.
>> The main exported functions (as can be seen) are:
>> __bond_opt_set() - to be used when a string is passed which needs to be
>>                   converted in the case of BOND_OPTVAL_INTEGER. (sysfs)
>> __bond_opt_intset() - to be used when a value is passed to
>>                      BOND_OPTVAL_INTEGER (netlink), this function can't
>>                      be used for BOND_OPTVAL_STRING options
>> These two can be used from inside other options to stop them (e.g., arp_interval
>> stopping miimon and vice versa).
>> I've also added bond_opt_tryset_rtnl() mostly for sysfs use.
>> See the description of patch 01 and the comments inside for more information.
>>
>> Value tables of converted options are no longer exported, and can be accessed
>> through the API (bond_opt_get_val() & bond_opt_get_flags).
>> Another good side-effect is that the error codes are standard for all options
>> for the common errors at least.
> 
> Nice!
> 
>> When/if this patchset is posted for inclusion, I'll have all options converted.
>> I actually had them before but while on vacation during December a lot of code
>> went in changing the bonding options and have to re-work most of the patches.
> 
> Oops, sorry about that ;)
> 
>> Some of the future plans for this are:
>> Verbose outputting of dependencies (done, just have to polish it)
>> Automatic sysfs generation from the bond_opts[].
> 
> I had a patch in my queue to do something similar, but yours is so much nicer.  
> 
> For sysfs nodes, there is a file permission. I wonder if bond_opts should have a sense of RO, RW, or WO?  Then automatic sysfs generation is even easier.  Hmmm, actually I think the answer is no.  Nevermind.
> 
>> Use of the API in bond_check_params() and thus cleaning it up
>> Structure for accessor fn parameter passing so we can implement get/set
>> in a more general manner
>>
>> Sending it with 2 options converted which illustrate the use of different
>> features of the API. I've tested them via sysfs.
>>
>> Any thoughts, comments and suggestions are very welcome.
> 
> Nice job Nik, well done.
> 
> -scott
> 
Hi Scott,
Thank you for the review, I'll take care of the comments for the first version.
I saw your first comment about generalizing this to more than just the
bonding, I'll look into that because I know I'm reinventing the wheel here
as there are already network drivers which have similar APIs, but such
project would have much larger set of requirements (e.g. a much better and
more descriptive inter-dependency checks, custom errors, custom
permissions, more opaque objects etc.). Maybe we can leave it as the next
step, I have to give it some more thought :-)

Cheers,
 Nik