Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 28 Mar 2019 15:04:28 +0100
From:   Michal Kubecek <mkubecek@suse.cz>
To:     Jiri Pirko <jiri@resnulli.us>
Cc:     Florian Fainelli <f.fainelli@gmail.com>,
        David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        Andrew Lunn <andrew@lunn.ch>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 12/22] ethtool: provide string sets with
 GET_STRSET request
Message-ID: <20190328140428.GG26076@unicorn.suse.cz>
References: <cover.1553532199.git.mkubecek@suse.cz>
 <f5af581e33b4976b84eae7c7788e3fde9adfa83c.1553532199.git.mkubecek@suse.cz>
 <2c29310b-a2a0-3867-a09f-51f2dc47ecd3@gmail.com>
 <20190328071853.GY26076@unicorn.suse.cz>
 <20190328134313.GO14297@nanopsycho>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190328134313.GO14297@nanopsycho>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 28, 2019 at 02:43:13PM +0100, Jiri Pirko wrote:
> 
> I don't like this. This should not be bitfield/set. This should be
> simply nested array of enum values:
> 
> enum ethtool_link_mode {
> 	ETHTOOL_LINK_MODE_10baseT_Half,
> 	ETHTOOL_LINK_MODE_10baseT_Full,
> 	ETHTOOL_LINK_MODE_100baseT_Half,
> 	ETHTOOL_LINK_MODE_100baseT_Full,
> 	ETHTOOL_LINK_MODE_1000baseT_Full,
> };

We already have such enum. The problem with your "no string" approach is
that it requires all userspace applications to (1) keep this enum in
sync with kernel and (2) maintain their our tables of names. Experience
shows we are not very good and satisfying these conditions even for the
one which should be best at keeping up.

> and then there should be 2 attrs:
> ETHTOOL_A_LINK_MODE_LIST_OUR	/* nest */
> ETHTOOL_A_LINK_MODE_LIST_PEER	/* nest */
> ETHTOOL_A_LINK_MODE		/* u32 */
> 
> and then the message should look like:
> 
>    ETHTOOL_A_LINK_MODE_LIST_OUR start nest
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
>    ETHTOOL_A_LINK_MODE_LIST_OUR end nest
>    ETHTOOL_A_LINK_MODE_LIST_PEER start nest
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
>      ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
>    ETHTOOL_A_LINK_MODE_LIST_PEER end nest
> 
> Nice and simple. No bits, no strings.

A bit too simple, actually. You would need third nest to distinguish
supported and advertised modes. And for setting, you would also need two
arrays if you want to set only some of the modes (unless you introduce
something that would be similar to mine except for omitting the names).

More important: you still didn't explain how is your "no strings"
approach supposed to work for bit sets where userspace cannot possibly
know the set of available flags (e.g. the private flags).

Michal