Repository: pkg/sftp
Pull request #499 (2022-03-03 09:32:07)

writeToSequential: improve tests for write errors
I tryed to improve test cases for write errors as requested in #494

Of course I kept the author for the original commits


Patch:
diff --git a/client.go b/client.go
index 1587eb8..9e0b616 100644
--- a/client.go
+++ b/client.go
@@ -1176,11 +1176,11 @@ func (f *File) writeToSequential(w io.Writer) (written int64, err error) {
 		if n > 0 {
 			f.offset += int64(n)
 
-			m, err2 := w.Write(b[:n])
+			m, err := w.Write(b[:n])
 			written += int64(m)
 
-			if err == nil {
-				err = err2
+			if err != nil {
+				return written, err
 			}
 		}
 
diff --git a/request-interfaces.go b/request-interfaces.go
index e5dc49b..c8c424c 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -90,8 +90,6 @@ type LstatFileLister interface {
 // We use "/" as start directory for relative paths, implementing this
 // interface you can customize the start directory.
 // You have to return an absolute POSIX path.
-//
-// Deprecated: if you want to set a start directory use WithStartDirectory RequestServerOption instead.
 type RealPathFileLister interface {
 	FileLister
 	RealPath(string) string
diff --git a/request-server.go b/request-server.go
index b7dadd6..5fa828b 100644
--- a/request-server.go
+++ b/request-server.go
@@ -27,8 +27,6 @@ type RequestServer struct {
 	*serverConn
 	pktMgr *packetManager
 
-	startDirectory string
-
 	mu           sync.RWMutex
 	handleCount  int
 	openRequests map[string]*Request
@@ -49,14 +47,6 @@ func WithRSAllocator() RequestServerOption {
 	}
 }
 
-// WithStartDirectory sets a start directory to use as base for relative paths.
-// If unset the default is "/"
-func WithStartDirectory(startDirectory string) RequestServerOption {
-	return func(rs *RequestServer) {
-		rs.startDirectory = cleanPath(startDirectory)
-	}
-}
-
 // NewRequestServer creates/allocates/returns new RequestServer.
 // Normally there will be one server per user-session.
 func NewRequestServer(rwc io.ReadWriteCloser, h Handlers, options ...RequestServerOption) *RequestServer {
@@ -72,8 +62,6 @@ func NewRequestServer(rwc io.ReadWriteCloser, h Handlers, options ...RequestServ
 		serverConn: svrConn,
 		pktMgr:     newPktMgr(svrConn),
 
-		startDirectory: "/",
-
 		openRequests: make(map[string]*Request),
 	}
 
@@ -222,11 +210,11 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if realPather, ok := rs.Handlers.FileList.(RealPathFileLister); ok {
 				realPath = realPather.RealPath(pkt.getPath())
 			} else {
-				realPath = cleanPathWithBase(rs.startDirectory, pkt.getPath())
+				realPath = cleanPath(pkt.getPath())
 			}
 			rpkt = cleanPacketPath(pkt, realPath)
 		case *sshFxpOpendirPacket:
-			request := requestFromPacket(ctx, pkt, rs.startDirectory)
+			request := requestFromPacket(ctx, pkt)
 			handle := rs.nextRequest(request)
 			rpkt = request.opendir(rs.Handlers, pkt)
 			if _, ok := rpkt.(*sshFxpHandlePacket); !ok {
@@ -234,7 +222,7 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 				rs.closeRequest(handle)
 			}
 		case *sshFxpOpenPacket:
-			request := requestFromPacket(ctx, pkt, rs.startDirectory)
+			request := requestFromPacket(ctx, pkt)
 			handle := rs.nextRequest(request)
 			rpkt = request.open(rs.Handlers, pkt)
 			if _, ok := rpkt.(*sshFxpHandlePacket); !ok {
@@ -247,10 +235,7 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if !ok {
 				rpkt = statusFromError(pkt.ID, EBADF)
 			} else {
-				request = &Request{
-					Method:   "Stat",
-					Filepath: cleanPathWithBase(rs.startDirectory, request.Filepath),
-				}
+				request = NewRequest("Stat", request.Filepath)
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case *sshFxpFsetstatPacket:
@@ -259,24 +244,15 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if !ok {
 				rpkt = statusFromError(pkt.ID, EBADF)
 			} else {
-				request = &Request{
-					Method:   "Setstat",
-					Filepath: cleanPathWithBase(rs.startDirectory, request.Filepath),
-				}
+				request = NewRequest("Setstat", request.Filepath)
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case *sshFxpExtendedPacketPosixRename:
-			request := &Request{
-				Method:   "PosixRename",
-				Filepath: cleanPathWithBase(rs.startDirectory, pkt.Oldpath),
-				Target:   cleanPathWithBase(rs.startDirectory, pkt.Newpath),
-			}
+			request := NewRequest("PosixRename", pkt.Oldpath)
+			request.Target = pkt.Newpath
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 		case *sshFxpExtendedPacketStatVFS:
-			request := &Request{
-				Method:   "StatVFS",
-				Filepath: cleanPathWithBase(rs.startDirectory, pkt.Path),
-			}
+			request := NewRequest("StatVFS", pkt.Path)
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 		case hasHandle:
 			handle := pkt.getHandle()
@@ -287,7 +263,7 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case hasPath:
-			request := requestFromPacket(ctx, pkt, rs.startDirectory)
+			request := requestFromPacket(ctx, pkt)
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			request.close()
 		default:
diff --git a/request.go b/request.go
index 116c27a..c6da4b6 100644
--- a/request.go
+++ b/request.go
@@ -168,11 +168,9 @@ func (r *Request) copy() *Request {
 }
 
 // New Request initialized based on packet data
-func requestFromPacket(ctx context.Context, pkt hasPath, baseDir string) *Request {
-	request := &Request{
-		Method:   requestMethod(pkt),
-		Filepath: cleanPathWithBase(baseDir, pkt.getPath()),
-	}
+func requestFromPacket(ctx context.Context, pkt hasPath) *Request {
+	method := requestMethod(pkt)
+	request := NewRequest(method, pkt.getPath())
 	request.ctx, request.cancelCtx = context.WithCancel(ctx)
 
 	switch p := pkt.(type) {
@@ -182,13 +180,13 @@ func requestFromPacket(ctx context.Context, pkt hasPath, baseDir string) *Reques
 		request.Flags = p.Flags
 		request.Attrs = p.Attrs.([]byte)
 	case *sshFxpRenamePacket:
-		request.Target = cleanPathWithBase(baseDir, p.Newpath)
+		request.Target = cleanPath(p.Newpath)
 	case *sshFxpSymlinkPacket:
 		// NOTE: given a POSIX compliant signature: symlink(target, linkpath string)
 		// this makes Request.Target the linkpath, and Request.Filepath the target.
-		request.Target = cleanPathWithBase(baseDir, p.Linkpath)
+		request.Target = cleanPath(p.Linkpath)
 	case *sshFxpExtendedPacketHardlink:
-		request.Target = cleanPathWithBase(baseDir, p.Newpath)
+		request.Target = cleanPath(p.Newpath)
 	}
 	return request
 }


Test patch:
diff --git a/client_integration_test.go b/client_integration_test.go
index b508384..c57ae77 100644
--- a/client_integration_test.go
+++ b/client_integration_test.go
@@ -1249,6 +1249,53 @@ func TestClientReadSequential(t *testing.T) {
 	}
 }
 
+// this writer requires maxPacket = 3 and always returns an error for the second write call
+type lastChunkErrSequentialWriter struct {
+	counter int
+}
+
+func (w *lastChunkErrSequentialWriter) Write(b []byte) (int, error) {
+	w.counter++
+	if w.counter == 1 {
+		if len(b) != 3 {
+			return 0, errors.New("this writer requires maxPacket = 3, please set MaxPacketChecked(3)")
+		}
+		return len(b), nil
+	}
+	return 1, errors.New("this writer fails after the first write")
+}
+
+func TestClientWriteSequentialWriterErr(t *testing.T) {
+	client, cmd := testClient(t, READONLY, NODELAY, MaxPacketChecked(3))
+	defer cmd.Wait()
+	defer client.Close()
+
+	d, err := ioutil.TempDir("", "sftptest-writesequential-writeerr")
+	require.NoError(t, err)
+
+	defer os.RemoveAll(d)
+
+	f, err := ioutil.TempFile(d, "write-sequential-writeerr-test")
+	require.NoError(t, err)
+	fname := f.Name()
+	_, err = f.Write([]byte("12345"))
+	require.NoError(t, err)
+	require.NoError(t, f.Close())
+
+	sftpFile, err := client.Open(fname)
+	require.NoError(t, err)
+	defer sftpFile.Close()
+
+	w := &lastChunkErrSequentialWriter{}
+	written, err := sftpFile.writeToSequential(w)
+	assert.Error(t, err)
+	expected := int64(4)
+	if written != expected {
+		t.Errorf("sftpFile.Write() = %d, but expected %d", written, expected)
+	}
+	assert.Equal(t, 2, w.counter)
+}
+
 func TestClientReadDir(t *testing.T) {
 	sftp1, cmd1 := testClient(t, READONLY, NODELAY)
 	sftp2, cmd2 := testClientGoSvr(t, READONLY, NODELAY)
diff --git a/request-server_test.go b/request-server_test.go
index c8e64d6..88862fb 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -37,7 +37,7 @@ func (cs csPair) testHandler() *root {
 
 const sock = "/tmp/rstest.sock"
 
-func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPair {
+func clientRequestServerPair(t *testing.T) *csPair {
 	skipIfWindows(t)
 	skipIfPlan9(t)
 
@@ -62,6 +62,7 @@ func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPa
 		require.NoError(t, err)
 
 		handlers := InMemHandler()
+		var options []RequestServerOption
 		if *testAllocator {
 			options = append(options, WithRSAllocator())
 		}
@@ -780,37 +781,6 @@ func TestRequestStatVFSError(t *testing.T) {
 	checkRequestServerAllocator(t, p)
 }
 
-func TestRequestStartDirOption(t *testing.T) {
-	startDir := "/start/dir"
-	p := clientRequestServerPair(t, WithStartDirectory(startDir))
-	defer p.Close()
-
-	// create the start directory
-	err := p.cli.MkdirAll(startDir)
-	require.NoError(t, err)
-	// the working directory must be the defined start directory
-	wd, err := p.cli.Getwd()
-	require.NoError(t, err)
-	require.Equal(t, startDir, wd)
-	// upload a file using a relative path, it must be uploaded to the start directory
-	fileName := "file.txt"
-	_, err = putTestFile(p.cli, fileName, "")
-	require.NoError(t, err)
-	// we must be able to stat the file using both a relative and an absolute path
-	for _, filePath := range []string{fileName, path.Join(startDir, fileName)} {
-		fi, err := p.cli.Stat(filePath)
-		require.NoError(t, err)
-		assert.Equal(t, fileName, fi.Name())
-	}
-	// list dir contents using a relative path
-	entries, err := p.cli.ReadDir(".")
-	assert.NoError(t, err)
-	assert.Len(t, entries, 1)
-	// delete the file using a relative path
-	err = p.cli.Remove(fileName)
-	assert.NoError(t, err)
-}
-
 func TestCleanDisconnect(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
@@ -861,7 +831,6 @@ func TestRealPath(t *testing.T) {
 func TestCleanPath(t *testing.T) {
 	assert.Equal(t, "/", cleanPath("/"))
 	assert.Equal(t, "/", cleanPath("."))
-	assert.Equal(t, "/", cleanPath(""))
 	assert.Equal(t, "/", cleanPath("/."))
 	assert.Equal(t, "/", cleanPath("/a/.."))
 	assert.Equal(t, "/a/c", cleanPath("/a/b/../c"))