Re: [ethtool PATCH 6/6] Update documentation for -u/-U operations

[Alexander Duyck <alexander.h.duyck@intel.com>]

On 4/27/2011 11:23 AM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
>> This patch updates the documentation for the -u/-U operations to include
>> the recent changes made to support addition/deletion/display of network
>> flow classifier rules.
>
> This should be included in the same patch.

I'll combine the two patches for the next release if that is what is 
preferred.  I was just trying to keep the overall size down by splitting 
them since this is documentation only.

>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> ---
>>
>>   ethtool.8.in |  185 +++++++++++++++++++++++++++++-----------------------------
>>   ethtool.c    |   32 ++++++----
>>   2 files changed, 111 insertions(+), 106 deletions(-)
>>
>> diff --git a/ethtool.8.in b/ethtool.8.in
>> index 12a1d1d..8908351 100644
>> --- a/ethtool.8.in
>> +++ b/ethtool.8.in
>> @@ -42,10 +42,20 @@
>>   [\\fB\\$1\\fP\ \\fIN\\fP]
>>   ..
>>   .\"
>> +.\"	.BM - same as above but has a mask field for format "[value N [value-mask N]]"
>> +.\"
>> +.de BM
>> +[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fB\\$1\-mask\\fP\ \\fIN\\fP]]
>
> You've changed the code to accept 'm' as an alternative to
> <field>  '-mask', so this should be changed accordingly.

What would be the preferred way of stating that?  For now I just 
replaced the \\$1\-mask with m.  However I am assuming that probably 
isn't the best approach either.  Should I state somewhere that m can be 
replaced with "field name"-mask?

> [...]
>> @@ -236,9 +252,9 @@ ethtool \- query or control network driver and hardware settings
>>   .HP
>>   .B ethtool \-N
>>   .I ethX
>> -.RB [ rx\-flow\-hash \ \*(FL
>> -.RB \ \*(HO]
>> +.RB [ rx-flow-hash \ \*(FL \  \*(HO]
>>   .HP
>> +
>
> This looks like an unintentional reversion of part of commit
> db6c0cee6cd956767e1c39109fe81104cc4694cb.

Yeah, my bad.  I will have it updated for the next patch.  I only really 
meant to combine this into one line, I didn't intend to drop the "\-" 
formatting fixes you added.

>>   .B ethtool \-x|\-\-show\-rxfh\-indir
>>   .I ethX
>>   .HP
>> @@ -257,54 +273,28 @@ ethtool \- query or control network driver and hardware settings
>>   .HP
>>   .B ethtool \-u|\-\-show\-ntuple
>>   .I ethX
>> -.TP
>> +.BN class-rule
>> +.HP
>> +
>>   .BI ethtool\ \-U|\-\-config\-ntuple \ ethX
>> -.RB {
>> -.A3 flow\-type tcp4 udp4 sctp4
>> -.RB [ src\-ip
>> -.IR addr
>> -.RB [ src\-ip\-mask
>> -.IR mask ]]
>> -.RB [ dst\-ip
>> -.IR addr
>> -.RB [ dst\-ip\-mask
>> -.IR mask ]]
>> -.RB [ src\-port
>> -.IR port
>> -.RB [ src\-port\-mask
>> -.IR mask ]]
>> -.RB [ dst\-port
>> -.IR port
>> -.RB [ dst\-port\-mask
>> -.IR mask ]]
>> -.br
>> -.RB | \ flow\-type\ ether
>> -.RB [ src
>> -.IR mac\-addr
>> -.RB [ src\-mask
>> -.IR mask ]]
>> -.RB [ dst
>> -.IR mac\-addr
>> -.RB [ dst\-mask
>> -.IR mask ]]
>> -.RB [ proto
>> -.IR N
>> -.RB [ proto\-mask
>> -.IR mask ]]\ }
>> -.br
>> -.RB [ vlan
>> -.IR VLAN\-tag
>> -.RB [ vlan\-mask
>> -.IR mask ]]
>> -.RB [ user\-def
>> -.IR data
>> -.RB [ user\-def\-mask
>> -.IR mask ]]
>> -.RI action \ N
>> -.
>> -.\" Adjust lines (i.e. full justification) and hyphenate.
>> -.ad
>> -.hy
>
> As do the last 3 deleted lines here.

I put them back so they should be left in place for the next version.

>> +.BN class-rule-del
>> +.RB [\  flow-type \ \*(NC
>> +.RB [ src \ \*(MA\ [ src-mask \ \*(MA]]
>> +.RB [ dst \ \*(MA\ [ dst-mask \ \*(MA]]
>> +.BM proto
>> +.RB [ src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]]
>> +.RB [ dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]]
>> +.BM tos
>> +.BM l4proto
>> +.BM src-port
>> +.BM dst-port
>> +.BM spi
>> +.BM vlan-etype
>> +.BM vlan
>> +.BM user-def
>> +.BN action
>> +.BN loc
>> +.RB ]
>
> But these options aren't all applicable to all flow-types.

This is the part that gets messy and I am not sure what the best 
approach is.  I have more comments on that below.  For now what I am 
planning to implement to address this is that in the "DESCRIPTION" 
section below I add a statement to each specifier that has restrictions 
by stating "Valid for flow-types X, Y, and Z."

> [...]
>> diff --git a/ethtool.c b/ethtool.c
>> index 421fe20..e65979d 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -243,20 +243,26 @@ static struct option {
>>   		"		equal N | weight W0 W1 ...\n" },
>>       { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
>>   		"and actions",
>> -		"		{ flow-type tcp4|udp4|sctp4\n"
>> -		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
>> -		"		  [ dst-ip ADDR [dst-ip-mask MASK] ]\n"
>> -		"		  [ src-port PORT [src-port-mask MASK] ]\n"
>> -		"		  [ dst-port PORT [dst-port-mask MASK] ]\n"
>> -		"		| flow-type ether\n"
>> -		"		  [ src MAC-ADDR [src-mask MASK] ]\n"
>> -		"		  [ dst MAC-ADDR [dst-mask MASK] ]\n"
>> -		"		  [ proto N [proto-mask MASK] ] }\n"
>> -		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
>> -		"		[ user-def DATA [user-def-mask MASK] ]\n"
>> -		"		action N\n" },
>> +		"		[ class-rule-del %d ] |\n"
>> +		"		[ flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4\n"
>> +		"			[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x] ]\n"
>> +		"			[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x] ]\n"
>> +		"			[ proto %d [proto-mask MASK] ]\n"
>> +		"			[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d] ]\n"
>> +		"			[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d] ]\n"
>> +		"			[ tos %d [tos-mask %x] ]\n"
>> +		"			[ l4proto %d [l4proto-mask MASK] ]\n"
>> +		"			[ src-port %d [src-port-mask %x] ]\n"
>> +		"			[ dst-port %d [dst-port-mask %x] ]\n"
>> +		"			[ spi %d [spi-mask %x] ]\n"
>> +		"			[ vlan-etype %x [vlan-etype-mask %x] ]\n"
>> +		"			[ vlan %x [vlan-mask %x] ]\n"
>> +		"			[ user-def %x [user-def-mask %x] ]\n"
>> +		"			[ action %d ]\n"
>> +		"			[ loc %d]]\n" },
> [...]
>
> Again, it's not clear which options apply to which flow-types, and the
> 'm' shortcut is not documented.

The 'm' part I agree with 100%, however the flow types are going to 
become kinda hairy using that approach.  You basically end up with 
something like this:

	flow-type ether
		[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x]]
		[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x]]
		[ proto %d [proto-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type ip4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ l4proto %d [l4proto-mask %x]]
		[ src-port %d [src-port-mask %x]]
		[ dst-port %d [dst-port-mask %x]]
		[ spi %d [spi-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type tcp4|udp4|sctp4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ src-port %d [src-port-mask %x]]
		[ dst-port %d [dst-port-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type ah4|esp4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ spi %d [spi-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]

As you can see it will be a bit oversized to go through and specify 
which flow-type options support what fields.  If that is what you want I 
can implement it that way but for now I would prefer calling out the 
flow-type limitations of the fields in the "DESCRIPTION" portion of the 
man page.

>
> Ben.
>

Thanks,

Alex