[Bug 2948] implement "copy-data" sftp extension

[bugzilla-daemon at mindrot.org]

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

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 3344
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3344
sftp server copy-data extension

looks good - some minor comments

>diff --git a/PROTOCOL b/PROTOCOL
>index f75c1c0ae5b0..04a392db33be 100644
...
>+static void
>+process_extended_copy_data(u_int32_t id)
...
>+	/* Disallow reading & writing to the same handle */
>+	if (read_handle == write_handle || read_fd < 0 || write_fd < 0) {

I think this should also check that both the read and write handles do
not refer to the same path? (use handle_to_name())

>+		status = SSH2_FX_FAILURE;
>+	} else {

nit: prefer "goto out" over nesting if/else

>+		if (lseek(read_fd, read_off, SEEK_SET) < 0) {
>+			status = errno_to_portable(errno);
>+			error("process_extended_copy_data: read_seek failed");

nit: ditto

>+		} else if (!(handle_to_flags(write_handle) & O_APPEND) &&
>+				lseek(write_fd, write_off, SEEK_SET) < 0) {
>+			status = errno_to_portable(errno);
>+			error("process_extended_copy_data: write_seek failed");

nit: prefer __func__ to manual inclusion of function name

>+		} else {
>+			/* Process the request in chunks. */
>+			while (read_len || copy_until_eof) {

nit: prefer explicit comparison against zero (i.e "read_len > 0")

>+
>+				ret = read(read_fd, buf, len);
...
>+				ret = write(write_fd, buf, len);

I think this should use atomicio here to be signal safe.

>+				if ((size_t)ret != len) {
>+					debug2("nothing at all written");
>+					status = SSH2_FX_FAILURE;
>+					break;
>+				}

this block can go away with atomicio

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