[Bug 2021] sftp resume support (using size and offset)

[bugzilla-daemon at mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2021

--- Comment #13 from Loganaden Velvindron <loganaden at gmail.com> ---
(In reply to Damien Miller from comment #11)
> Comment on attachment 2199 [details]
> Resume diff with copyright
> 
> >Index: sftp-client.c
> >===================================================================
> >RCS file: /cvs/openssh/sftp-client.c,v
> >retrieving revision 1.108
> >diff -u -p -r1.108 sftp-client.c
> >--- sftp-client.c	2 Jul 2012 12:15:39 -0000	1.108
> >+++ sftp-client.c	11 Dec 2012 19:00:53 -0000
> >@@ -15,6 +15,24 @@
> >  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >  */
> > 
> >+/*
> >+ * Copyright (c) 2012 Loganaden Velvindron
> >+ *
> >+ * Permission to use, copy, modify, and distribute this software for any
> >+ * purpose with or without fee is hereby granted, provided that the above
> >+ * copyright notice and this permission notice appear in all copies.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> >+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> >+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> >+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> >+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> >+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> >+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >+ *
> >+ * Sponsored by AfriNIC.
> >+ */
> 
> Normally we wouldn't add a whole copyright notice for a change of a
> few dozen lines. I'm happy to credit AfriNIC in the commit message.

The diff I uploaded the last time was incomplete. The complete diff is
a bit long. In Any case, I'm fine with it :-)


> 
> >@@ -1050,11 +1069,36 @@ do_download(struct sftp_conn *conn, char
> > 		return(-1);
> > 	}
> > 
> >-	local_fd = open(local_path, O_WRONLY | O_CREAT | O_TRUNC,
> >-	    mode | S_IWRITE);
> >+	if (resume) {
> >+		if (stat(local_path, &st) == -1) {
> >+			offset = 0;
> >+			highwater = 0;
> >+			local_fd = open(local_path, O_WRONLY | O_CREAT,
> >+					mode | S_IWRITE);
> >+		}
> 
> I thing it would be safer and clearer to do:
> 
> local_fd = open(local_path, O_WRONLY | O_CREAT | S_IWRITE | mode |
> resume ? 0 : O_TRUNC)
> fstat(local_fd, &st)
> offset = highwater = st.st_size
> // test bigger, etc.

Yep, agreed as well.

> 
> 
> >@@ -1139,6 +1183,12 @@ do_download(struct sftp_conn *conn, char
> > 				write_error = 1;
> > 				max_req = 0;
> > 			}
> >+			else if (req->offset <= highwater)
> >+				highwater = req->offset + len;
> >+			else if (req->offset > highwater) {
> >+				ftruncate(local_fd, 0);
> >+				printf("reordered blocks detected");
> 
> This will spam the user for every block transferred and break the
> download of a file that would have otherwise downloaded fine (by
> truncating it). I think it would be better to just leave highwater
> at 0 for this case and do a debug() call on the first run through
> the loop.

That makes sense.

> 	
> >+			}	
> > 			progress_counter += len;
> > 			xfree(data);
> > 
> >@@ -1187,6 +1237,11 @@ do_download(struct sftp_conn *conn, char
> > 	/* Sanity check */
> > 	if (TAILQ_FIRST(&requests) != NULL)
> > 		fatal("Transfer complete, but requests still in queue");
> >+	/* Truncate at highest contiguous point to avoid holes on interrupt */
> >+	if (read_error || write_error || interrupted) {
> >+		debug("truncating at %llu", highwater);
> >+		ftruncate(local_fd, highwater);
> 
> Here you should check if highwater == 0 to detect reordered blocks
> and warn the user: logit("server reordered requests: %s download
> cannot be resumed", local_path) or something similar.

Added as you requested.


> 
> >@@ -1233,6 +1288,7 @@ download_dir_internal(struct sftp_conn *
> > 	SFTP_DIRENT **dir_entries;
> > 	char *filename, *new_src, *new_dst;
> > 	mode_t mode = 0777;
> >+	int resume = 0;
> > 
> > 	if (depth >= MAX_DIR_DEPTH) {
> > 		error("Maximum directory depth exceeded: %d levels", depth);
> >@@ -1284,7 +1340,7 @@ download_dir_internal(struct sftp_conn *
> > 				ret = -1;
> > 		} else if (S_ISREG(dir_entries[i]->a.perm) ) {
> > 			if (do_download(conn, new_src, new_dst,
> >-			    &(dir_entries[i]->a), pflag) == -1) {
> >+			    &(dir_entries[i]->a), pflag, resume) == -1) {
> 
> why use a variable here and not just 0?


do_download() needs to later pass on the resume value. It can be 1 as
well.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.