Re: [PATCH net 2/3] pktgen: give user result when disable vlan/svlan

[Hangbin Liu <liuhangbin@gmail.com>]

On Thu, May 15, 2014 at 07:05:56PM +0800, Hangbin Liu wrote:
> On Thu, May 15, 2014 at 12:00:31PM +0200, Daniel Borkmann wrote:
> > On 05/15/2014 11:46 AM, Hangbin Liu wrote:
> > >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > >---
> > >  net/core/pktgen.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > >diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > >index dcf367f..1809bdf 100644
> > >--- a/net/core/pktgen.c
> > >+++ b/net/core/pktgen.c
> > >@@ -1573,8 +1573,7 @@ static ssize_t pktgen_if_write(struct file *file,
> > >  			pkt_dev->vlan_id = 0xffff; /* turn off VLAN/SVLAN */
> > >  			pkt_dev->svlan_id = 0xffff;
> > >
> > >-			if (debug)
> > >-				pr_debug("VLAN/SVLAN turned off\n");
> > >+			sprintf(pg_result, "OK: VLAN/SVLAN turned off");
> > 
> > I think that might break user scripts as pg_result is copied to user
> > space, and currently only expected to return 'OK: svlan_id=%u' if it
> > was actually successful. Unfortunately, scripts that might only check
> > for 'OK' in the string could make wrong assumptions later on.

or maybe we can return 0xffff when disabled vlan? like

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index fabd17e..64afb91 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1580,7 +1580,10 @@ static ssize_t pktgen_if_write(struct file *file,
                        pkt_dev->vlan_id = 0xffff; /* turn off VLAN/SVLAN */
                        pkt_dev->svlan_id = 0xffff;

-                       sprintf(pg_result, "OK: VLAN/SVLAN turned off");
+                       sprintf(pg_result, "OK: vlan_id=0x%x", pkt_dev->vlan_id);
+
+                       if (debug)
+                               pr_debug("VLAN/SVLAN turned off\n");
                }
                return count;
        }

But I saw dst_mac, src_mac and clear_counters also didn't use the OK: param=value
format.
sprintf(pg_result, "OK: dstmac %pM", pkt_dev->dst_mac);
sprintf(pg_result, "OK: srcmac %pM", pkt_dev->src_mac);
sprintf(pg_result, "OK: Clearing counters.\n");

We can change src_mac and dst_mac to "srcmac=%pM" easily, but I don't know how
to edit counters. return OK: counters=0 ?

> 
> But the original behavior will keep the last return value, which may make user
> confused. e.g.
> 
> # echo vlan_id 10 > /proc/net/pktgen/eno4
> # cat /proc/net/pktgen/eno4
> Params: count 1000  min_pkt_size: 0  max_pkt_size: 0
>      frags: 0  delay: 0  clone_skb: 0  ifname: eno4
>      flows: 0 flowlen: 0
>      queue_map_min: 0  queue_map_max: 0
>      dst_min:   dst_max:
>         src_min:   src_max:
>      src_mac: 6c:ae:8b:20:7b:cc dst_mac: 00:00:00:00:00:00
>      udp_src_min: 9  udp_src_max: 9  udp_dst_min: 9  udp_dst_max: 9
>      src_mac_count: 0  dst_mac_count: 0
>      vlan_id: 10  vlan_p: 0  vlan_cfi: 0
>      Flags:
> Current:
>      pkts-sofar: 0  errors: 0
>      started: 0us  stopped: 0us idle: 0us
>      seq_num: 0  cur_dst_mac_offset: 0  cur_src_mac_offset: 0
>      cur_saddr: 0.0.0.0  cur_daddr: 0.0.0.0
>      cur_udp_dst: 0  cur_udp_src: 0
>      cur_queue_map: 0
>      flows: 0
> Result: OK: vlan_id=10
> 
> # echo vlan_id 9999 > /proc/net/pktgen/eno4
> # cat /proc/net/pktgen/eno4
> Params: count 1000  min_pkt_size: 0  max_pkt_size: 0
>      frags: 0  delay: 0  clone_skb: 0  ifname: eno4
>      flows: 0 flowlen: 0
>      queue_map_min: 0  queue_map_max: 0
>      dst_min:   dst_max:
>         src_min:   src_max:
>      src_mac: 6c:ae:8b:20:7b:cc dst_mac: 00:00:00:00:00:00
>      udp_src_min: 9  udp_src_max: 9  udp_dst_min: 9  udp_dst_max: 9
>      src_mac_count: 0  dst_mac_count: 0
>      Flags:
> Current:
>      pkts-sofar: 0  errors: 0
>      started: 0us  stopped: 0us idle: 0us
>      seq_num: 0  cur_dst_mac_offset: 0  cur_src_mac_offset: 0
>      cur_saddr: 0.0.0.0  cur_daddr: 0.0.0.0
>      cur_udp_dst: 0  cur_udp_src: 0
>      cur_queue_map: 0
>      flows: 0
> Result: OK: vlan_id=10
> 
> > 
> > >  		}
> > >  		return count;
> > >  	}
> > >@@ -1629,8 +1628,7 @@ static ssize_t pktgen_if_write(struct file *file,
> > >  		} else {
> > >  			pkt_dev->svlan_id = 0xffff;
> > >
> > >-			if (debug)
> > >-				pr_debug("SVLAN turned off\n");
> > >+			sprintf(pg_result, "OK: SVLAN turned off");
> > 
> > Ditto.
> > 
> > >  		}
> > >  		return count;
> > >  	}
> > >
> 
> -- 
> 
> Thanks & Best Regards
> Hangbin Liu <liuhangbin@gmail.com>

-- 

Thanks & Best Regards
Hangbin Liu <liuhangbin@gmail.com>