From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=HtyDxsf+ljByvlYNF/RxiGUFw4XNyD3pI8qIbAAgxTg=; b=AqAak0+xUiLO/zms/H6G8fsRdCuHdSDWn7KLjW99y0d32orRzDJgLfMsWVLPk0CI4k AXy5Qj+M9FBPTJdAfHJzvxi3QJXCtl7ASXQ51yWr9MNVf/paaJ2ziFyy++Ow+cMfilGs KePbciG0Cz9UBk/W80KmvF8zc+4hWPvhVxaZ4lIKqEM5+4Uor72Iy7iHrpGRh8WPxqEE CzbCkcYEwcjw+PqvEXgoESP88j7W8LWQCiVHkI7AlOWfQcNlFVPc/j6nxLZSXnELkHbR vPL0CjloVUNlBvVRkk20g+4DcTNN13a3fRGyz3yy7riZE2zl2uD4aG3ZLy+7c8MzOON9 ksAA== References: <58EFE482.7020508@gmail.com> From: Frank Rowand Message-ID: <58EFF304.2050706@gmail.com> Date: Thu, 13 Apr 2017 14:52:04 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp List-Id: Mailing list for the Fuego test framework List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Bird, Timothy" , "monstr@monstr.eu" , "fuego@lists.linuxfoundation.org" On 04/13/17 14:21, Bird, Timothy wrote: > 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 ; 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 >>> --- >>> 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= Serial port baud rate [115200] >>> -B, --basic Use basic (minimal) remote system commands >>> + -d, --device= Use serial device specified >>> -h, --help Show help and exit >>> -M, --md5sum verify file transfer with md5sum >>> -m, --minicom= 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=> 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. Ugh. Yes, that looks really silly and is less readable. OK, let's not change the host syntax for sersh, which means do make the sersh_usage() change you proposed. > >> >> >>> - --timeout is a big stick. Setting to a small value will result >>> in errors or unstable behaviour. 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/ Yes, 'nonsense_host' is perfectly fine with me. > 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. Given what you said about silly format earlier, the 5 line change above will not be needed. >> 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 >