https://bugzilla.mindrot.org/show_bug.cgi?id=2021
--- Comment #13 from Loganaden Velvindron <[email protected]> --- (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. _______________________________________________ openssh-bugs mailing list [email protected] https://lists.mindrot.org/mailman/listinfo/openssh-bugs
