From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [ethtool PATCH 6/6] Update documentation for -u/-U operations Date: Thu, 28 Apr 2011 13:40:57 -0700 Message-ID: <4DB9D0D9.2000401@intel.com> References: <20110421202857.23054.63316.stgit@gitlad.jf.intel.com> <20110421204045.23054.12548.stgit@gitlad.jf.intel.com> <1303928629.2875.91.camel@bwh-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "davem@davemloft.net" , "Kirsher, Jeffrey T" , "netdev@vger.kernel.org" To: Ben Hutchings Return-path: Received: from mga09.intel.com ([134.134.136.24]:1240 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751131Ab1D1Uk6 (ORCPT ); Thu, 28 Apr 2011 16:40:58 -0400 In-Reply-To: <1303928629.2875.91.camel@bwh-desktop> Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> >> 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 > '-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