Bug 1417162 - Broken example in tc-csum.8
Summary: Broken example in tc-csum.8
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: iproute
Version: 7.4
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: rc
: ---
Assignee: Matteo Croce
QA Contact: Jaroslav Aster
URL:
Whiteboard:
Depends On:
Blocks: 1470965
TreeView+ depends on / blocked
 
Reported: 2017-01-27 11:22 UTC by Phil Sutter
Modified: 2018-04-10 14:30 UTC (History)
8 users (show)

Fixed In Version: iproute-4.11.0-12.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-04-10 14:28:47 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHEA-2018:0815 0 None None None 2018-04-10 14:30:42 UTC

Description Phil Sutter 2017-01-27 11:22:26 UTC
The provided example is broken in two ways. A patch fixing both has been sent upstream, but not accepted yet:

Author: Phil Sutter <phil@nwl.cc>
Date:   Fri Jan 27 12:08:29 2017 +0100

    man: tc-csum.8: Fix example
    
    This fixes two issues with the provided example:
    
    - Add missing 'dev' keyword to second command.
    - Use a real IPv4 address instead of a bogus hex value since that will
      be rejected by get_addr_ipv4().
    
    Fixes: dbfb17a67f9c7 ("man: tc-csum.8: Add an example")
    Reported-by: Davide Caratti <dcaratti@redhat.com>

Comment 1 Phil Sutter 2017-03-27 12:21:52 UTC
Upstream accepted the patch:

commit 6bbe5e6290db5cf3e78d6a1a179c76535a2eea3f
Author: Phil Sutter <phil@nwl.cc>
Date:   Sat Jan 28 12:59:10 2017 +0100

    man: tc-csum.8: Fix example
    
    This fixes two issues with the provided example:
    
    - Add missing 'dev' keyword to second command.
    - Use a real IPv4 address instead of a bogus hex value since that will
      be rejected by get_addr_ipv4().
    
    Fixes: dbfb17a67f9c7 ("man: tc-csum.8: Add an example")
    Reported-by: Davide Caratti <dcaratti@redhat.com>
    Signed-off-by: Phil Sutter <phil@nwl.cc>

Comment 2 Jakub Sitnicki 2017-07-04 09:26:38 UTC
Nit pick - after 6bbe5e6290db ("man: tc-csum.8: Fix example") description is now out of sync with the command example:

EXAMPLES
       The   following  performs  stateless  NAT  for  incoming  packets  from
       192.168.1.100 to new  destination  18.52.86.120  (0x12345678  in  hex).
       Assuming  these  are  UDP packets, both IP and UDP checksums have to be
       recalculated:

              # tc qdisc add dev eth0 ingress handle ffff:
              # tc filter add dev eth0 prio 1 protocol ip parent ffff: \
                   u32 match ip src 192.0.2.100/32 flowid :1 \
                   action pedit munge ip dst set 198.51.100.1 pipe \
                   csum ip and udp

Might be worth shooting a patch upstream.

Comment 3 Davide Caratti 2017-07-04 09:58:27 UTC
(In reply to Jakub Sitnicki from comment #2)
> Nit pick - after 6bbe5e6290db ("man: tc-csum.8: Fix example")
...
> Might be worth shooting a patch upstream.

sure, and maybe there is also a grammar fix to be upstreamed (I forgot mentioning it to Phil when we talked about this, sorry).

'action to fix for then incorrect checksums'

I think 'then' is a 'the' , and maybe the whole sentence can be reworded as follows:

diff --git a/man/man8/tc-csum.8 b/man/man8/tc-csum.8
index 718301d..bee06d2 100644
--- a/man/man8/tc-csum.8
+++ b/man/man8/tc-csum.8
@@ -29,9 +29,9 @@ csum - checksum update action
 The
 .B csum
 action triggers checksum recalculation of specified packet headers. It is
-commonly used after packet editing using the
+commonly used to fix incorrect checksums after the
 .B pedit
-action to fix for then incorrect checksums.
+action has modified the packet contents.
 .SH OPTIONS
 .TP
 .I TARGET

Comment 4 Matteo Croce 2017-07-24 13:10:15 UTC
Fixed upstream in b09515553fded944713955815a3f1cc855384abd

Comment 6 Jaroslav Aster 2017-11-29 16:48:57 UTC
Is it really fixed? I don't see Phil's patch in the last build, only typo patch,  Example seems to be still broken - different ips in the text and in the commands.


The  following  performs stateless NAT for incoming packets from 192.168.1.100 to new destination 18.52.86.120 (0x12345678 in hex). Assuming these are UDP packets, both IP and UDP checksums have to be
       recalculated:

              # tc qdisc add dev eth0 ingress handle ffff:
              # tc filter add dev eth0 prio 1 protocol ip parent ffff: \
                   u32 match ip src 192.0.2.100/32 flowid :1 \
                   action pedit munge ip dst set 198.51.100.1 pipe \
                   csum ip and udp

Comment 7 Phil Sutter 2017-11-29 17:30:12 UTC
Hi Jaroslav,

(In reply to Jaroslav Aster from comment #6)
> Is it really fixed? I don't see Phil's patch in the last build, only typo
> patch,  Example seems to be still broken - different ips in the text and in
> the commands.

The patch I mentioned in comment 1 got pulled in implicitly with the rebase. Inconsistency between example and it's description regarding IP addresses is consistent with upstream. I'll prepare a patch, feel free to set QA_FAILED here or create a new ticket for RHEL7.6 if you think it's not worth waiting for upstream.

Thanks, Phil

Comment 8 Phil Sutter 2017-11-29 17:37:30 UTC
Patch sent upstream: https://marc.info/?l=linux-netdev&m=151197686415592&w=2

Comment 9 Jaroslav Aster 2017-11-30 11:15:26 UTC
Hi Phil,

Thanks. For me, this issue is not enough reason for the respin, but if your patch will be accepted by upstream and there will be a respin for some other reason, we will fix it.

Comment 10 Phil Sutter 2017-11-30 11:32:24 UTC
Hi Jaroslav,

(In reply to Jaroslav Aster from comment #9)
> Thanks. For me, this issue is not enough reason for the respin, but if your
> patch will be accepted by upstream and there will be a respin for some other
> reason, we will fix it.

There will be, due to bug 1459600. :)

Cheers, Phil

Comment 11 Jaroslav Aster 2017-11-30 12:33:13 UTC
I know, but has your patch been already accepted by upstream? This is the second condition.

If you think, it will take only couple of days, we can wait.

Comment 15 errata-xmlrpc 2018-04-10 14:28:47 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2018:0815


Note You need to log in before you can comment on or make changes to this bug.