Re: [dts] [PATCH v3 01/17] framwork/packet: sniff_packet specify running target support

["Liu, Yong" <yong.liu@intel.com>]

Hi Phil,
I think it will be too complicated for each suite handle the temporary pcap file.
Packet module is just designed for this kind of purpose. 
There's only need few changes on your v3 patch for fulfilling the need.

Logic for load_tcpdump_sniff_pcap will be pretty clear:

	pcap = load_pcap_f(index)  # just stop sniff process
	self.session.copy_file_from(pcap) # copy from tester to self CRB
	return pcap.split(os.sep)[-1] # return filename

As to load_tcpdump_sniff_packets, I think this function should be revised since pcap file only available after ssh copy.

	file=self.load_tcpdump_sniff_pcap(index)
	return packet.load_pcapfile(file)

You may also need to remove load_sniff_packets function in packet module as it will be useless. 
	
Thanks,
Marvin

> -----Original Message-----
> From: Phil Yang [mailto:Phil.Yang@arm.com]
> Sent: Wednesday, April 04, 2018 3:27 PM
> To: Liu, Yong <yong.liu@intel.com>; dts@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: RE: [PATCH v3 01/17] framwork/packet: sniff_packet specify
> running target support
> 
> Hi Marvin,
> 
> > Filename will be return after running load_pcap_f function, please use
> that
> > name.
> > It can be multiple sniffer functions work in the same time.
> 
> If so, I think it is better to copy sniffer pcap file outside
> load_tcpdump_sniff_pcap and load_tcpdump_sniff_packets function of tester
> module.
> I was planning to do it that way in patch V2, but it is need to add
> tester.session.copy_file_from() operation in each test case.
> 
> Can I roll back to patch V2?
> 
> Thanks,
> Phil Yang
> 
> > -----Original Message-----
> > From: Liu, Yong <yong.liu@intel.com>
> > Sent: Wednesday, April 4, 2018 11:28 AM
> > To: Phil Yang <Phil.Yang@arm.com>; dts@dpdk.org
> > Cc: nd <nd@arm.com>
> > Subject: RE: [PATCH v3 01/17] framwork/packet: sniff_packet specify
> running
> > target support
> >
> > Phil,
> > Some comments are inline.
> >
> > Thanks,
> > Marvin
> >
> > > -----Original Message-----
> > > From: Phil Yang [mailto:phil.yang@arm.com]
> > > Sent: Monday, April 02, 2018 11:46 AM
> > > To: dts@dpdk.org
> > > Cc: nd@arm.com; Liu, Yong <yong.liu@intel.com>; phil.yang@arm.com
> > > Subject: [PATCH v3 01/17] framwork/packet: sniff_packet specify
> > > running target support
> > >
> > > If tester in crb file was not the machine which running dts, the
> > > sniff_packet process will not running on tester.
> > >
> > > Create a ssh connection to the tester and run tcpdump to make sure
> > > sniff_packet process running on the machine we expected.
> > >
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > ---
> > >  framework/packet.py | 34 ++++++++++++++++++++++++++++------
> > >  framework/tester.py | 38 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 66 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/framework/packet.py b/framework/packet.py index
> > > 976b82b..484e511 100755
> > > --- a/framework/packet.py
> > > +++ b/framework/packet.py
> > > @@ -812,15 +812,30 @@ def get_filter_cmd(filters=[]):
> > >          return ""
> > >
> > >
> > > -def sniff_packets(intf, count=0, timeout=5, filters=[]):
> > > +def sniff_packets(intf, count=0, timeout=5, filters=[], target=[]):
> > >      """
> > >      sniff all packets for certain port in certain seconds
> > >      """
> > >      param = ""
> > >      direct_param = r"(\s+)\[ (\S+) in\|out\|inout \]"
> > > -    tcpdump_help = subprocess.check_output("tcpdump -h; echo 0",
> > > -                                           stderr=subprocess.STDOUT,
> > > -                                           shell=True)
> > > +
> > > +    # target[] contain the remote machine info for ssh connection
> > > +    # target[0]: username
> > > +    # target[1]: ip address
> > > +    # target[2]: pass word
> > > +    if target:
> > > +        tcpdump_help_pipe = subprocess.Popen(["ssh",
> > > +                            "%s@%s" % (target[0], target[1]),
> > > +                            "tcpdump -h"],
> > > +                            stderr=subprocess.PIPE,
> > > +                            stdout=subprocess.PIPE,
> > > +                            shell=False)
> > > +        tcpdump_help = "".join(tuple(tcpdump_help_pipe.communicate()))
> > > +        tcpdump_help_pipe.wait()
> > > +    else:
> > > +        tcpdump_help = subprocess.check_output("tcpdump -h; echo 0",
> > > +                                    stderr=subprocess.STDOUT,
> > > + shell=True)
> > > +
> > >      for line in tcpdump_help.split('\n'):
> > >          m = re.match(direct_param, line)
> > >          if m:
> > > @@ -850,9 +865,16 @@ def sniff_packets(intf, count=0, timeout=5,
> > > filters=[]):
> > >      else:
> > >          cmd = sniff_cmd % options
> > >
> > > -    args = shlex.split(cmd)
> > > +    if target:
> > > +        pipe = subprocess.Popen(["ssh",
> > > +                "%s@%s" % (target[0], target[1]),
> > > +                cmd],
> > > +                stdin=subprocess.PIPE,
> > > +                shell=False)
> > > +    else:
> > > +        args = shlex.split(cmd)
> > > +        pipe = subprocess.Popen(args)
> > >
> > > -    pipe = subprocess.Popen(args)
> > >      index = str(time.time())
> > >      SNIFF_PIDS[index] = (pipe, intf, timeout)
> > >      time.sleep(1)
> > > diff --git a/framework/tester.py b/framework/tester.py index
> > > a775f68..10761a8 100755
> > > --- a/framework/tester.py
> > > +++ b/framework/tester.py
> > > @@ -70,6 +70,7 @@ class Tester(Crb):
> > >          self.bgCmds = []
> > >          self.bgItf = ''
> > >          self.re_run_time = 0
> > > +	self.sniff_intf = ''
> > >
> > >      def init_ext_gen(self):
> > >          """
> > > @@ -704,6 +705,43 @@ class Tester(Crb):
> > >              self.proc.kill()
> > >              self.proc = None
> > >
> > > +    def tcpdump_sniff_packets(self, intf, count=0, timeout=5,
> filters=[]):
> > > +        """
> > > +        Wrapper for packet module sniff_packets
> > > +        """
> > > +        # load functions in packet module
> > > +        module = __import__("packet")
> > > +        sniff_f = getattr(module, "sniff_packets")
> > > +
> > > +        target=[]
> > > +        target.append(self.get_username())
> > > +        target.append(self.get_ip_address())
> > > +        target.append(self.get_password())
> > > +	self.sniff_intf = intf
> >
> > Indent not align here, and I think there's no need to save sniffer
> interface in this
> > module.
> >
> > > +        return sniff_f(intf, count, timeout, filters, target)
> > > +
> > > +    def load_tcpdump_sniff_pcap(self, index=''):
> > > +        """
> > > +        Wrapper for packet module load_sniff_pcap
> > > +        """
> > > +        # load functions in packet module
> > > +        module = __import__("packet")
> > > +        load_pcap_f = getattr(module, "load_sniff_pcap")
> > > +	self.session.copy_file_from("/tmp/sniff_%s.pcap" % self.sniff_intf,
> > > "/tmp/")
> > > +
> >
> > Filename will be return after running load_pcap_f function, please use
> that
> > name.
> > It can be multiple sniffer functions work in the same time.
> >
> > > +        return load_pcap_f(index)
> > > +
> > > +    def load_tcpdump_sniff_packets(self, index=''):
> > > +        """
> > > +        Wrapper for packet module load_sniff_packets
> > > +        """
> > > +        # load functions in packet module
> > > +        module = __import__("packet")
> > > +        load_f = getattr(module, "load_sniff_packets")
> > > +	self.session.copy_file_from("/tmp/sniff_%s.pcap" % self.sniff_intf,
> > > "/tmp/")
> > > +
> > > +        return load_f(index)
> > > +
> > >      def kill_all(self, killall=False):
> > >          """
> > >          Kill all scapy process or DPDK application on tester.
> > > --
> > > 2.7.4