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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <7F4A3DF6-7DD5-4E11-986A-C090F02506A5@cumulusnetworks.com>
Date:	Fri, 10 Jan 2014 12:21:22 -0800
From:	Scott Feldman <sfeldma@...ulusnetworks.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	Netdev <netdev@...r.kernel.org>
Subject: Re: [RFC net-next 1/3] bonding: add infrastructure for an option API


On Jan 10, 2014, at 5:11 AM, Nikolay Aleksandrov <nikolay@...hat.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

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