Re: [RFC PATCH ethtool] ethtool: Improve compatibility between netlink and ioctl interfaces

[Ido Schimmel <idosch@idosch.org>]

On Mon, Nov 02, 2020 at 11:58:03PM +0100, Michal Kubecek wrote:
> On Mon, Nov 02, 2020 at 08:40:36PM +0200, Ido Schimmel wrote:
> > +static int linkmodes_reply_adver_all_cb(const struct nlmsghdr *nlhdr,
> 
>                               ^^^^^ advert?
> 
> > +					void *data)
> > +{
> > +	const struct nlattr *bitset_tb[ETHTOOL_A_BITSET_MAX + 1] = {};
> > +	const struct nlattr *tb[ETHTOOL_A_LINKMODES_MAX + 1] = {};
> > +	DECLARE_ATTR_TB_INFO(bitset_tb);
> > +	struct nl_context *nlctx = data;
> > +	struct nl_msg_buff *msgbuff;
> > +	DECLARE_ATTR_TB_INFO(tb);
> > +	struct nl_socket *nlsk;
> > +	struct nlattr *nest;
> > +	int ret;
> > +
> > +	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (!tb[ETHTOOL_A_LINKMODES_OURS])
> > +		return -EINVAL;
> > +
> > +	ret = mnl_attr_parse_nested(tb[ETHTOOL_A_LINKMODES_OURS], attr_cb,
> > +				    &bitset_tb_info);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (!bitset_tb[ETHTOOL_A_BITSET_SIZE] ||
> > +	    !bitset_tb[ETHTOOL_A_BITSET_VALUE] ||
> > +	    !bitset_tb[ETHTOOL_A_BITSET_MASK])
> > +		return -EINVAL;
> > +
> > +	ret = netlink_init_ethnl2_socket(nlctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	nlsk = nlctx->ethnl2_socket;
> > +	msgbuff = &nlsk->msgbuff;
> > +
> > +	ret = msg_init(nlctx, msgbuff, ETHTOOL_MSG_LINKMODES_SET,
> > +		       NLM_F_REQUEST | NLM_F_ACK);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ethnla_fill_header(msgbuff, ETHTOOL_A_LINKMODES_HEADER,
> > +			       nlctx->devname, 0))
> > +		return -EMSGSIZE;
> > +
> > +	if (ethnla_put_u8(msgbuff, ETHTOOL_A_LINKMODES_AUTONEG, AUTONEG_ENABLE))
> > +		return -EMSGSIZE;
> > +
> > +	/* Use the size and mask from the reply and set the value to the mask,
> > +	 * so that all supported link modes will be advertised.
> > +	 */
> > +	ret = -EMSGSIZE;
> > +	nest = ethnla_nest_start(msgbuff, ETHTOOL_A_LINKMODES_OURS);
> > +	if (!nest)
> > +		return -EMSGSIZE;
> > +
> > +	if (ethnla_put_u32(msgbuff, ETHTOOL_A_BITSET_SIZE,
> > +			   mnl_attr_get_u32(bitset_tb[ETHTOOL_A_BITSET_SIZE])))
> > +		goto err;
> > +
> > +	if (ethnla_put(msgbuff, ETHTOOL_A_BITSET_VALUE,
> > +		       mnl_attr_get_payload_len(bitset_tb[ETHTOOL_A_BITSET_MASK]),
> > +		       mnl_attr_get_payload(bitset_tb[ETHTOOL_A_BITSET_MASK])))
> > +		goto err;
> > +
> > +	if (ethnla_put(msgbuff, ETHTOOL_A_BITSET_MASK,
> > +		       mnl_attr_get_payload_len(bitset_tb[ETHTOOL_A_BITSET_MASK]),
> > +		       mnl_attr_get_payload(bitset_tb[ETHTOOL_A_BITSET_MASK])))
> > +		goto err;
> > +
> > +	ethnla_nest_end(msgbuff, nest);
> 
> To fully replicate ioctl code behaviour, we should only set the bits
> corresponding to "real" link modes, not "special" ones (e.g.
> ETHTOOL_LINK_MODE_TP_BIT).

Michal,

I have the changes you requested here:
https://github.com/idosch/ethtool/commit/b34d15839f2662808c566c04eda726113e20ee59

Do you want to integrate it with your nl_parse() rework or should I?

Thanks