From: Alexander Duyck <alexander.h.duyck@intel•com>
To: Ben Hutchings <bhutchings@solarflare•com>
Cc: "davem@davemloft•net" <davem@davemloft•net>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>
Subject: Re: [ethtool PATCH 6/6] Update documentation for -u/-U operations
Date: Thu, 28 Apr 2011 13:40:57 -0700 [thread overview]
Message-ID: <4DB9D0D9.2000401@intel.com> (raw)
In-Reply-To: <1303928629.2875.91.camel@bwh-desktop>
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
next prev parent reply other threads:[~2011-04-28 20:40 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-21 20:40 [ethtool PATCH 0/6] Network flow classifier Alexander Duyck
2011-04-21 20:40 ` [ethtool PATCH 1/6] Add support for ESP as a separate protocol from AH Alexander Duyck
2011-04-27 15:47 ` Ben Hutchings
2011-04-21 20:40 ` [ethtool PATCH 2/6] Add support for NFC flow classifier extensions Alexander Duyck
2011-04-27 15:48 ` Ben Hutchings
2011-04-21 20:40 ` [ethtool PATCH 3/6] ethtool: remove strings based approach for displaying n-tuple Alexander Duyck
2011-04-27 18:12 ` Ben Hutchings
2011-04-21 20:40 ` [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool Alexander Duyck
2011-04-27 15:54 ` Ben Hutchings
2011-04-27 16:46 ` Alexander Duyck
2011-04-27 17:09 ` Ben Hutchings
2011-04-27 18:33 ` Alexander Duyck
2011-04-21 20:40 ` [ethtool PATCH 5/6] v4 Add RX packet classification interface Alexander Duyck
2011-04-27 18:12 ` Ben Hutchings
2011-04-27 23:00 ` Ben Hutchings
2011-04-28 20:15 ` Alexander Duyck
2011-04-21 20:40 ` [ethtool PATCH 6/6] Update documentation for -u/-U operations Alexander Duyck
2011-04-27 18:23 ` Ben Hutchings
2011-04-28 20:40 ` Alexander Duyck [this message]
2011-04-29 2:57 ` Ben Hutchings
2011-04-21 20:51 ` [ethtool PATCH 0/6] Network flow classifier Ben Hutchings
2011-04-21 21:11 ` Alexander Duyck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DB9D0D9.2000401@intel.com \
--to=alexander.h.duyck@intel$(echo .)com \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox