[Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
Frank Rowand
frowand.list at gmail.com
Thu Apr 13 21:52:04 UTC 2017
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 at gmail.com]
>> Sent: Thursday, April 13, 2017 1:50 PM
>> To: Bird, Timothy <Tim.Bird at sony.com>; monstr at monstr.eu;
>> fuego at 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 at 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.
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 <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/
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
>
More information about the Fuego
mailing list