Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp

["Bird, Timothy" <Tim.Bird@sony.com>]

I'll work on these, as requested.  I may not finish them today, and I'm off on vacation starting
tomorrow.  (Sorry Michal).

BTW - making the patch was super-obnoxious, because there are some lines with trailing
whitespace in serio, and I've now set my editor to automatically trim whitespace at the
end of lines for python files.  So whenever I edit the file, it changes 4 places that have
nothing to do with the change at hand.  I manually removed these hunks before sending
the patch.  Would you accept a whitespace cleanup patch?  Or should I just fix the workflow
on my side?  Let me know.

Comments inline below.

> -----Original Message-----
> From: Frank Rowand [mailto:frowand.list@gmail.com]
> Sent: Thursday, April 13, 2017 1:50 PM
> To: Bird, Timothy <Tim.Bird@sony.com>; monstr@monstr.eu;
> fuego@lists.linuxfoundation.org
> Subject: Re: [PATCH] Add support for full device paths (with slashes and
> colons) to sercp
> 
> On 04/13/17 11:35, Bird, Timothy wrote:
> > Add a new argument (-d, --device) to support a full device path.
> > Due to file argument parsing constraints, sersh cannot handle
> > certain serial paths used as the host for the source or destination.
> >
> > This feature makes it possible to use a full path with slashes
> > and colons to specify the serial port for the operation.  Use something
> > like:
> >  $ sercp -d /dev/serial/by-path/pci-0000:00:00.0-usb-0:3:1.4 file1
> serial:/tmp
> >
> > The reason for this is that in some cases, using the full path is more
> > convenient than the single-word device.  For example, the full path can
> > uniquely identify a serial port on a USB port, that might change device
> > names as USB ports are connected and disconnected.
> 
> Yes to the concept.  Some implementation details need adjustment.
> 
> All of my code below is untested.
> 
> 
> > Signed-off-by: Tim Bird <tim.bird@sony.com>
> > ---
> >  serio | 31 +++++++++++++++++++++++++------
> >  1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/serio b/serio
> > index b1ed793..f1e2730 100755
> > --- a/serio
> > +++ b/serio
> > @@ -537,6 +537,7 @@ def usage_sercp():
> >  	print '''
> >     -b, --baudrate=<baud>           Serial port baud rate [115200]
> >     -B, --basic                     Use basic (minimal) remote system commands
> > +   -d, --device=<device path>      Use serial device specified
> >     -h, --help                      Show help and exit
> >     -M, --md5sum                    verify file transfer with md5sum
> >     -m, --minicom=<name>            Name of the minicom config file to use
> > @@ -555,6 +556,10 @@ Notes:
> >        a "/" before the ":".  This means that "host1" and "host2" may not
> >        contain a "/".  Due to this constraint "host1" and "host2" are device
> >        names relative to "/dev/".
> > +    - if a device path is specified, then it should be a fully-qualified path
> > +      (not relative to '/dev/'), and you should use "serial" in the source
> > +      or destination path as the host.
> 
>   +      In this case the hostname is just a placeholder.  If multiple source
>   +      files are specified the hostname must be the same for each of the
>   +      source files.
OK.
> 
> > +ex: sercp -d /dev/serial/by-path/pci-0000:00:00.0-usb-0:3:1.4 file1
> serial:/tmp
> >      - All source files ("file1 ...") must be on the same host.
> >      - The source files ("file1 ...") and the destination location ("file2")
> >        must be on different hosts.  Local copies and copies between remote
> > @@ -625,7 +630,8 @@ def usage_sersh():
> >
> >  Notes:
> >      - user is ignored
> > -    - host is a serial device name, relative to "/dev/".
> > +    - host is a serial device name, relative to "/dev/", or a full path
> > +      to the serial device, starting with "/".
> 
> The format of host should not change for sersh.  I want host to be consistent
> for sercp and sersh, so the parsing of host for sersh should be changed to
> not allow an embedded slash.  This means that the '-d, --device=<device
> path>'
> also needs to be added to sersh option parsing and the same additions as
> were
> made to usage_sercp() should be added to usage_sersh().

I actually made a version of the patch that added the same thing to sersh, but the
result looked funny, and I noted that it already had no restrictions on the "host" string,
so didn't really need to specify an extra parameter.

Here's what it looked like: 
$ sersh -d /dev/serial/by-path/pci-0000:00:00.0-usb-0:3:1.4 serial ls

that extra argument 'serial' just seems silly.  But if you want to keep things consistent,
I'm OK with supporting it as shown.

> 
> 
> >      - --timeout is a big stick.  Setting <seconds> to a small value will result
> >        in errors or unstable behaviour.  <seconds> must be long enough to
> allow
> >        the entire command to be echoed.
> > @@ -652,7 +658,7 @@ Return:
> >
> >  def main():
> >
> > -	def parse_arg_for_files(args):
> > +	def parse_arg_for_files(args, device_path):
> >  		host = None
> >  		files = []
> >
> > @@ -689,7 +695,14 @@ def main():
> >  					host_start = 0
> >  				new_host = arg[host_start:host_end]
> >  				if not '/' in new_host:
> > -					new_host = '/dev/' + new_host
> 
> That existing code has a test that is not needed.  The paragraph above
> these two lines guarantees that there is not a '/' in new_host, so
> the 'if not '/' in new_host:' is redundant and should be removed.
> Line 692 should always prefix new_host with '/dev/'.

OK.

> The following 8 lines should not be added.  Just use the existing code
> to find the host name and ensure that it is the same for each
> source file.  The test should be on device_path, not on the name
> of new_host (see my next chunk of code, just before the return).
> 
> > +					if new_host=="serial":
> > +						if device_path:
> > +							new_host =
> device_path
> > +						else:
> > +							print "ERROR: must
> specific device path (with -d) with host 'serial'"
> > +							sys.exit(EX_USAGE)
> > +					else:
> > +						new_host = '/dev/' +
> new_host
> >  				if host is None:
> >  					host = new_host
> >  				elif host != new_host:
> 
> Then just before the return statement, adjust the value of host:
> 
>   +		if host is not None and device_path is not None:
>   +			host = device_path

OK.  Since we don't check the host string, this will allow things like this:
$ sercp -d /dev/serial/by-path/pci:000:080.usb:1.3.40 file1  nonsense_host:/tmp/

It's OK with me, but just checking.
 
>   		return host, files
> 
> > @@ -717,6 +730,7 @@ def main():
> >  	create_link_path = None
> >  	create_links     = None
> >  	destination      = None
> > +	device_path      = None
> >  	timeout          = None
> >  	minicom          = None
> >  	port             = None
> > @@ -763,10 +777,11 @@ def main():
> >
> >  		try:
> >  			opts, args = getopt.getopt(sys.argv[1:],
> > -					    'b:BhMm:rT:v',
> > +					    'b:Bd:hMm:rT:v',
> >  					    [
> >  					     'baudrate=',
> >  					     'basic',
> > +					     'device=',
> >  					     'help',
> >  					     'md5sum',
> >  					     'minicom=',
> > @@ -786,6 +801,8 @@ def main():
> >  				baudrate = arg
> >  			elif opt in ('-B', '--basic'):
> >  				basic = 1
> > +			elif opt in ('-d', '--device'):
> > +				device_path = arg
> >  			elif opt in ('-h', '--help'):
> >  				usage_sercp()
> >  				sys.exit(EX_OK)
> > @@ -885,8 +902,10 @@ def main():
> >  			print 'ERROR: file1 and file2 required'
> >  			sys.exit(EX_USAGE)
> >
> 
> No need to line wrap the following, there are many longer lines already.
OK

> > -		src_host, source = parse_arg_for_files(args[0:len(args) - 1])
> > -		dst_host, dst    = parse_arg_for_files(args[len(args) - 1:])
> > +		src_host, source = parse_arg_for_files(args[0:len(args) - 1],
> > +					device_path)
> > +		dst_host, dst    = parse_arg_for_files(args[len(args) - 1:],
> > +					device_path)
> >
> >  		if src_host:
> >  			if dst_host:
> >
> 
> Then the following change will be needed for sersh at line 940 so that
> sersh will follow the same host name rules:
> 
>                 if '/' in host:
>  -                      port = host
>  +			print 'ERROR: invalid host name'
>  +			sys.exit(EX_USAGE)
>  +		elif host is not None and device_path is not None:
>  +			port = device_path

OK.
>                 else:
>                         port = '/dev/' + host
> 

Let me know if you have any objections after hearing some more input.  Otherwise
I'll just re-do the patch as requested.
 -- Tim