bugzilla-daemon at mindrot.org
2013-Jun-19 09:59 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Loganaden Velvindron <loganaden at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #2199|0 |1
is obsolete| |
--- Comment #12 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2302
--> https://bugzilla.mindrot.org/attachment.cgi?id=2302&action=edit
resume diff with corrections
--
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.
bugzilla-daemon at mindrot.org
2013-Jun-19 10:04 UTC
[Bug 2021] sftp resume support (using size and offset)
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.
bugzilla-daemon at mindrot.org
2013-Jun-19 10:08 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Loganaden Velvindron <loganaden at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #2302|application/octet-stream |text/plain
mime type| |
Attachment #2302|0 |1
is patch| |
--
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.
bugzilla-daemon at mindrot.org
2013-Jun-20 00:47 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Damien Miller <djm at mindrot.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |2076
--- Comment #14 from Damien Miller <djm at mindrot.org> ---
Thanks - looks good.
--
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.
bugzilla-daemon at mindrot.org
2013-Jun-26 07:44 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021 --- Comment #15 from Loganaden Velvindron <loganaden at gmail.com> --- Created attachment 2304 --> https://bugzilla.mindrot.org/attachment.cgi?id=2304&action=edit sftp.1 man page diff I also included a description similar to that of get. I replaced retrieve with resume. What do you guys think ? Should we elaborate more ? -- 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.
bugzilla-daemon at mindrot.org
2013-Jun-26 07:57 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Loganaden Velvindron <loganaden at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #2304|0 |1
is obsolete| |
--- Comment #16 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2305
--> https://bugzilla.mindrot.org/attachment.cgi?id=2305&action=edit
sftp man page diff
Updated so that it's sorted alphabetically.
--
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.
bugzilla-daemon at mindrot.org
2013-Jul-12 03:25 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Damien Miller <djm at mindrot.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |dtucker at zip.com.au
Status|NEW |ASSIGNED
Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
Attachment #2313| |ok?(dtucker at zip.com.au)
Flags| |
Attachment #2302|0 |1
is obsolete| |
Attachment #2305|0 |1
is obsolete| |
--- Comment #17 from Damien Miller <djm at mindrot.org> ---
Created attachment 2313
--> https://bugzilla.mindrot.org/attachment.cgi?id=2313&action=edit
Combined diff with tweaks
This diff (source and manual) has a couple of tweaks:
- Add "get -a" to attempt resumption
- Make "reget" a synonym for "get -a"
- Fix some code formatting
- Allow -a and -r to coexist: make download_dir() pass the resume flag
on
- A couple of logic tweaks in do_download()
I think this is ready to go in.
--
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.
bugzilla-daemon at mindrot.org
2013-Jul-12 04:46 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021
Damien Miller <djm at mindrot.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #2313|ok?(dtucker at zip.com.au) |
Flags| |
Attachment #2314| |ok?(dtucker at zip.com.au)
Flags| |
Attachment #2313|0 |1
is obsolete| |
--- Comment #18 from Damien Miller <djm at mindrot.org> ---
Created attachment 2314
--> https://bugzilla.mindrot.org/attachment.cgi?id=2314&action=edit
another tweak
This simplifies the logic for server that reorder requests a bit more.
It only displays the warning message if it actually makes a difference
(i.e. the transfer did not complete successfully).
--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2013-Jul-12 07:06 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021 --- Comment #19 from Loganaden Velvindron <loganaden at gmail.com> --- I've tested your latest diff against an anoncvs copy. It works fine for both single files and recursive downloads. However, the diff is slightly out of date, and patch(1) warned about fuzzing for certain lines. I've attached a diff against the latest version from the cvs server. -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2013-Jul-12 07:08 UTC
[Bug 2021] sftp resume support (using size and offset)
https://bugzilla.mindrot.org/show_bug.cgi?id=2021 --- Comment #20 from Loganaden Velvindron <loganaden at gmail.com> --- Created attachment 2316 --> https://bugzilla.mindrot.org/attachment.cgi?id=2316&action=edit djm_latest_diff_anoncvs -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.