Bug 2783 - Patch for disabling progress meter after file has finished uploading
Summary: Patch for disabling progress meter after file has finished uploading
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: scp (show other bugs)
Version: 7.6p1
Hardware: Other Linux
: P5 enhancement
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-24 19:33 AEST by gyroninja
Modified: 2021-04-23 15:10 AEST (History)
1 user (show)

See Also:


Attachments
patch that fixes issue (413 bytes, application/octet-stream)
2017-09-24 19:33 AEST, gyroninja
no flags Details
patch that fixes issue (413 bytes, patch)
2017-09-24 19:34 AEST, gyroninja
no flags Details | Diff
patch that fixes issue (418 bytes, patch)
2017-09-25 09:39 AEST, gyroninja
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gyroninja 2017-09-24 19:33:46 AEST
Created attachment 3058 [details]
patch that fixes issue

Once the file has finished uploading the progress meter stops updating.
Without this patch after the file has finished uploading the total time value is bigger than it should be. This time being wrong also causes your average upload rate to be incorrect since the timer is incrementing without any data being sent.
Comment 1 gyroninja 2017-09-24 19:34:41 AEST
Created attachment 3059 [details]
patch that fixes issue
Comment 2 Damien Miller 2017-09-25 07:55:57 AEST
Comment on attachment 3059 [details]
patch that fixes issue

This bug report doesn't make any sense: version 7.6p1 isn't released yet and progressmeter.c doesn't contain the code you're modifying:

>-	if (cur_pos < end_pos) {
>-		signal(SIGALRM, update_progress_meter);
>-		alarm(UPDATE_INTERVAL);
>-		errno = save_errno;
>-	}
>+	signal(SIGALRM, update_progress_meter);
>+	alarm(UPDATE_INTERVAL);
>+	errno = save_errno;

The existing code already reads as after your patch is applied:

   241          if (can_output())
   242                  refresh_progress_meter();
   243  
   244          signal(SIGALRM, update_progress_meter);
   245          alarm(UPDATE_INTERVAL);
   246          errno = save_errno;

Generally though, apparent "hangs" at 100% of the progress meter display are caused by buffering happening in the sending system ssh process, kernel socket buffers, the network or at the receiving side. Stopping the clock until the remote end has acknowledged receipt of the entire file is *correcting* the time and bandwidth calculations that are being distorted by the aforementioned buffering.
Comment 3 gyroninja 2017-09-25 09:39:59 AEST
Created attachment 3060 [details]
patch that fixes issue
Comment 4 gyroninja 2017-09-25 09:43:55 AEST
Hi, sorry for making a few mistake. This is my first time contributing to the project.
This change was originally for 7.4p1, but it didn't look like progressmeter.c had changed since then so I selected the latest version.
It looks like I accidentally had my diff backwards. I have corrected the attachment with the diff in the right order.
The diff is supposed to insert the if, because the progress bar should not update after it has finished transferring the whole file.
Comment 5 Damien Miller 2019-07-19 15:45:43 AEST
I think these problems (and others) were fixed in the progressmeter.c changes that Darren committed before the OpenSSH 8.0 release. Please reopen the bug if this is not the case.
Comment 6 Damien Miller 2021-04-23 15:10:02 AEST
closing resolved bugs as of 8.6p1 release