openssh bugs - Jan 2013 - [Bug 2021] sftp resume support (using size and offset)

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

--- Comment #10 from Loganaden Velvindron <loganaden at gmail.com> ---
ping :-) ?

-- 
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.

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

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2199
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2199
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.
>@@ -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.

>@@ -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.
>+			}	
> 			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.
>@@ -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?

-- 
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.