Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
Scott Cheloha wrote: > On Sat, Dec 31, 2022 at 07:05:20PM +0100, Mark Kettenis wrote: > > > Date: Sat, 31 Dec 2022 10:33:26 -0500 > > > From: Scott Cheloha > > > > > > Here's another one. > > > > > > The progress meter in scp(1) and sftp(1) updates periodically, once > > > per second. But using alarm(3) to repeatedly rearm the signal causes > > > that update period to drift forward: > > > > > > [...] > > > > > > If we use setitimer(2), the update period does not drift: > > > > > > [...] > > > > Bad idea. The setitimer(2) interface is marked as "OB XSI" in POSIX, > > which means that it is considerent "Obsolescent" and may be removed in > > a future version of POSIX. Since we want ssh to be as portable as > > possible we shouldn't use it there. > > Strict POSIX conformance is pretty portable. But in practice, I think > of portability as "using stuff everyone has". I think you are right, especially when it comes to OpenSSH. OpenSSH is likely impossible in a "pure POSIX" environment, it needs "POSIX + a whole lot more". In anycase, that POSIX sub-battle is dumb.
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
Joerg Sonnenberger wrote: > On Sat, Dec 31, 2022 at 04:16:18PM -0500, Scott Cheloha wrote: > > Even Windows went with Linux: WSL2 has Linux syscall compatibility, > > WSL2 is running a Linux kernel under HyperV. WSL1 is the system call > translation layer. Your reply is irrelevant to the point Scott was making.
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
On Sat, Dec 31, 2022 at 04:16:18PM -0500, Scott Cheloha wrote: > Even Windows went with Linux: WSL2 has Linux syscall compatibility, WSL2 is running a Linux kernel under HyperV. WSL1 is the system call translation layer. Joerg
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
On Sat, 31 Dec 2022 19:05:20 +0100, Mark Kettenis wrote: > Bad idea. The setitimer(2) interface is marked as "OB XSI" in POSIX, > which means that it is considerent "Obsolescent" and may be removed in > a future version of POSIX. Since we want ssh to be as portable as > possible we shouldn't use it there. Especially for something that > really is just a cosmetic "fix". I considered this but I can't think of any platform that OpenSSH supports which doesn't have setitimer(2). The reality is that even if POSIX removes setitimer(2), systems that already implement it will not remove it. So I think the only concern is some theoretical future POSIX system. - todd
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
On Sat, Dec 31, 2022 at 07:05:20PM +0100, Mark Kettenis wrote: > > Date: Sat, 31 Dec 2022 10:33:26 -0500 > > From: Scott Cheloha > > > > Here's another one. > > > > The progress meter in scp(1) and sftp(1) updates periodically, once > > per second. But using alarm(3) to repeatedly rearm the signal causes > > that update period to drift forward: > > > > [...] > > > > If we use setitimer(2), the update period does not drift: > > > > [...] > > Bad idea. The setitimer(2) interface is marked as "OB XSI" in POSIX, > which means that it is considerent "Obsolescent" and may be removed in > a future version of POSIX. Since we want ssh to be as portable as > possible we shouldn't use it there. Strict POSIX conformance is pretty portable. But in practice, I think of portability as "using stuff everyone has". POSIX has been hunting the 4.1cBSD time interfaces for over two decades, but no practical operating system is going to remove gettimeofday() or setitimer() just because POSIX prefers clock_gettime() and timer_settime() (which we don't have). Even Windows went with Linux: WSL2 has Linux syscall compatibility, and they have setitimer(). Defined here: https://github.com/microsoft/WSL2-Linux-Kernel/blob/linux-msft-wsl-5.15.y/kernel/time/itimer.c#L332 > Especially for something that really is just a cosmetic "fix". I admit that this is a minor gripe. I can live without it. I hope that setitimer's looming POSIX obsolescence is not a barrier to my using it in other programs in the tree, though.
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
> Date: Sat, 31 Dec 2022 10:33:26 -0500
> From: Scott Cheloha
>
> Here's another one.
>
> The progress meter in scp(1) and sftp(1) updates periodically, once
> per second. But using alarm(3) to repeatedly rearm the signal causes
> that update period to drift forward:
>
> $ kdump -ts -R -f scp-current.ktrace.out
> 25343 scp 1672500224.844047 PSIG SIGALRM caught handler=0x239bf6ec750
> mask=0<>
> 25343 scp 1.009691 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
> 25343 scp 1.000314 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
> 25343 scp 1.009971 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
> 25343 scp 1.009872 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
> 25343 scp 1.009831 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
> 25343 scp 1.000315 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
>
> If we use setitimer(2), the update period does not drift:
>
> $ kdump -ts -R -f scp-patched.ktrace.out
> 33053 scp 1672500345.413781 PSIG SIGALRM caught handler=0x9d45436b7f0
> mask=0<>
> 33053 scp 1.14 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.27 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.000208 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 0.999742 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.83 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.57 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.91 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 0.999604 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
> 33053 scp 1.000176 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
>
> ok?
Bad idea. The setitimer(2) interface is marked as "OB XSI" in POSIX,
which means that it is considerent "Obsolescent" and may be removed in
a future version of POSIX. Since we want ssh to be as portable as
possible we shouldn't use it there. Especially for something that
really is just a cosmetic "fix".
> Index: progressmeter.c
> ===
> RCS file: /cvs/src/usr.bin/ssh/progressmeter.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 progressmeter.c
> --- progressmeter.c 23 Jan 2020 07:10:22 - 1.50
> +++ progressmeter.c 31 Dec 2022 15:30:50 -
> @@ -25,8 +25,10 @@
>
> #include
> #include
> +#include
> #include
>
> +#include
> #include
> #include
> #include
> @@ -232,12 +234,13 @@ static void
> sig_alarm(int ignore)
> {
> alarm_fired = 1;
> - alarm(UPDATE_INTERVAL);
> }
>
> void
> start_progress_meter(const char *f, off_t filesize, off_t *ctr)
> {
> + struct itimerval itv = {{ UPDATE_INTERVAL, 0 }, { UPDATE_INTERVAL, 0 }};
> +
> start = last_update = monotime_double();
> file = f;
> start_pos = *ctr;
> @@ -252,13 +255,17 @@ start_progress_meter(const char *f, off_
>
> ssh_signal(SIGALRM, sig_alarm);
> ssh_signal(SIGWINCH, sig_winch);
> - alarm(UPDATE_INTERVAL);
> + if (setitimer(ITIMER_REAL, , NULL) == -1)
> + err(1, "setitimer");
> }
>
> void
> stop_progress_meter(void)
> {
> - alarm(0);
> + struct itimerval zero = { 0 };
> +
> + if (setitimer(ITIMER_REAL, , NULL) == -1)
> + err(1, "setitimer");
>
> if (!can_output())
> return;
>
>
Re: ssh: progress meter: prefer setitimer(2) to alarm(3)
On Sat, 31 Dec 2022 10:33:26 -0500, Scott Cheloha wrote: > The progress meter in scp(1) and sftp(1) updates periodically, once > per second. But using alarm(3) to repeatedly rearm the signal causes > that update period to drift forward: OK millert@ - todd
ssh: progress meter: prefer setitimer(2) to alarm(3)
Here's another one.
The progress meter in scp(1) and sftp(1) updates periodically, once
per second. But using alarm(3) to repeatedly rearm the signal causes
that update period to drift forward:
$ kdump -ts -R -f scp-current.ktrace.out
25343 scp 1672500224.844047 PSIG SIGALRM caught handler=0x239bf6ec750
mask=0<>
25343 scp 1.009691 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
25343 scp 1.000314 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
25343 scp 1.009971 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
25343 scp 1.009872 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
25343 scp 1.009831 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
25343 scp 1.000315 PSIG SIGALRM caught handler=0x239bf6ec750 mask=0<>
If we use setitimer(2), the update period does not drift:
$ kdump -ts -R -f scp-patched.ktrace.out
33053 scp 1672500345.413781 PSIG SIGALRM caught handler=0x9d45436b7f0
mask=0<>
33053 scp 1.14 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.27 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.000208 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 0.999742 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.83 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.57 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.91 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 0.999604 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
33053 scp 1.000176 PSIG SIGALRM caught handler=0x9d45436b7f0 mask=0<>
ok?
Index: progressmeter.c
===
RCS file: /cvs/src/usr.bin/ssh/progressmeter.c,v
retrieving revision 1.50
diff -u -p -r1.50 progressmeter.c
--- progressmeter.c 23 Jan 2020 07:10:22 - 1.50
+++ progressmeter.c 31 Dec 2022 15:30:50 -
@@ -25,8 +25,10 @@
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -232,12 +234,13 @@ static void
sig_alarm(int ignore)
{
alarm_fired = 1;
- alarm(UPDATE_INTERVAL);
}
void
start_progress_meter(const char *f, off_t filesize, off_t *ctr)
{
+ struct itimerval itv = {{ UPDATE_INTERVAL, 0 }, { UPDATE_INTERVAL, 0 }};
+
start = last_update = monotime_double();
file = f;
start_pos = *ctr;
@@ -252,13 +255,17 @@ start_progress_meter(const char *f, off_
ssh_signal(SIGALRM, sig_alarm);
ssh_signal(SIGWINCH, sig_winch);
- alarm(UPDATE_INTERVAL);
+ if (setitimer(ITIMER_REAL, , NULL) == -1)
+ err(1, "setitimer");
}
void
stop_progress_meter(void)
{
- alarm(0);
+ struct itimerval zero = { 0 };
+
+ if (setitimer(ITIMER_REAL, , NULL) == -1)
+ err(1, "setitimer");
if (!can_output())
return;
