* [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
@ 2017-04-13 18:35 Bird, Timothy
2017-04-13 20:50 ` Frank Rowand
0 siblings, 1 reply; 6+ messages in thread
From: Bird, Timothy @ 2017-04-13 18:35 UTC (permalink / raw)
To: monstr, Frank Rowand, fuego
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.
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.
+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 "/".
- --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
+ 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:
@@ -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)
- 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:
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
2017-04-13 18:35 [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp Bird, Timothy
@ 2017-04-13 20:50 ` Frank Rowand
2017-04-13 21:21 ` Bird, Timothy
0 siblings, 1 reply; 6+ messages in thread
From: Frank Rowand @ 2017-04-13 20:50 UTC (permalink / raw)
To: Bird, Timothy, monstr, fuego
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.
> +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().
> - --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/'.
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
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.
> - 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
else:
port = '/dev/' + host
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
2017-04-13 20:50 ` Frank Rowand
@ 2017-04-13 21:21 ` Bird, Timothy
2017-04-13 21:52 ` Frank Rowand
0 siblings, 1 reply; 6+ messages in thread
From: Bird, Timothy @ 2017-04-13 21:21 UTC (permalink / raw)
To: Frank Rowand, monstr, fuego
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
2017-04-13 21:21 ` Bird, Timothy
@ 2017-04-13 21:52 ` Frank Rowand
0 siblings, 0 replies; 6+ messages in thread
From: Frank Rowand @ 2017-04-13 21:52 UTC (permalink / raw)
To: Bird, Timothy, monstr, fuego
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 <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.
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
@ 2017-04-13 18:48 Bird, Timothy
2017-04-13 21:06 ` Frank Rowand
0 siblings, 1 reply; 6+ messages in thread
From: Bird, Timothy @ 2017-04-13 18:48 UTC (permalink / raw)
To: monstr, Frank Rowand, fuego
> -----Original Message-----
> From: Bird, Timothy on Thursday, April 13, 2017 11:36 AM
Just some commentary on the patch...
This patch solves the problem we were discussing earlier.
With the feature, I can do any of the following, which are all equivalent:
$ sercp foo ttyACM1:/tmp
$ sercp -d /dev/ttyACM1 foo serial:/tmp
$ sercp -d /dev/serial/by-path/pci-0000:08:00.0-usb-0:3:1.4 foo serial:/tmp
All of these copy file 'foo' to the host at the other end of the serial port.
You can use 'serial' as the placeholder for the device path in either the source
or destination file arguments. So this works also:
$ sercp -d /dev/serial/by-path/pci-0000:08:00.0-usb-0:3:1.4 serial:/tmp/foo bar
(copy the file 'foo' from the remove host back to the local filesystem, as 'bar').
Let me know what you think.
BTW - what's up with the unused "user@" parameters? Why are those supported?
Fuego does not need them or use them. Was there some plan to use them in the future?
-- Tim
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp
2017-04-13 18:48 Bird, Timothy
@ 2017-04-13 21:06 ` Frank Rowand
0 siblings, 0 replies; 6+ messages in thread
From: Frank Rowand @ 2017-04-13 21:06 UTC (permalink / raw)
To: Bird, Timothy, monstr, fuego
On 04/13/17 11:48, Bird, Timothy wrote:
>> -----Original Message-----
>> From: Bird, Timothy on Thursday, April 13, 2017 11:36 AM
>
> Just some commentary on the patch...
>
> This patch solves the problem we were discussing earlier.
> With the feature, I can do any of the following, which are all equivalent:
> $ sercp foo ttyACM1:/tmp
> $ sercp -d /dev/ttyACM1 foo serial:/tmp
> $ sercp -d /dev/serial/by-path/pci-0000:08:00.0-usb-0:3:1.4 foo serial:/tmp
> All of these copy file 'foo' to the host at the other end of the serial port.
>
> You can use 'serial' as the placeholder for the device path in either the source
> or destination file arguments. So this works also:
> $ sercp -d /dev/serial/by-path/pci-0000:08:00.0-usb-0:3:1.4 serial:/tmp/foo bar
> (copy the file 'foo' from the remove host back to the local filesystem, as 'bar').
>
> Let me know what you think.
I like it.
> BTW - what's up with the unused "user@" parameters? Why are those supported?
> Fuego does not need them or use them. Was there some plan to use them in the future?
The intent was to allow a drop in replacement for scp and ssh, with the only change
to the command being that a serial device name replaces the host name. So having
a "user@" would not result in an error.
At the moment the user name is just ignored. The user name could possibly be used
in the future:
sercp
-- If user specified for the target, try to 'chown user' after copying
-- If user specified for the source, try to 'su user' before reading
the file. This is likely to fail due to passwords.
sersh
-- If user specified, try to 'su user' to execute the command as user.
This is likely to fail due to passwords.
It seems like the 'chown user' might be a good idea, but the other changes
look like feature creep to me.
>
> -- Tim
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-13 21:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-13 18:35 [Fuego] [PATCH] Add support for full device paths (with slashes and colons) to sercp Bird, Timothy
2017-04-13 20:50 ` Frank Rowand
2017-04-13 21:21 ` Bird, Timothy
2017-04-13 21:52 ` Frank Rowand
2017-04-13 18:48 Bird, Timothy
2017-04-13 21:06 ` Frank Rowand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.