Fixes #184; better fix for file batching
I didn't like how the initial fix for #184 required the handler author
to track the token/offset for the filelist. I came up with a way to do
it that mimic'd the FileReader handler interface using a method
something like ReaderAt except for file lists.
changed MaxFileinfoList to a var for testing and tweaking
Renaming FileInfoer interface to FileLister.
FileInfoer -> FileLister
Fileinfo -> Filelist
diff --git a/request-example.go b/request-example.go
index 3333a8d..c355fbd 100644
--- a/request-example.go
+++ b/request-example.go
@@ -11,7 +11,6 @@
"os"
"path/filepath"
"sort"
- "strconv"
"sync"
"time"
)
@@ -101,20 +100,27 @@
return nil
}
-func (fs *root) Fileinfo(r Request) ([]os.FileInfo, error) {
+type listerat []os.FileInfo
+
+// Modeled after strings.Reader's ReadAt() implementation
+func (f listerat) ListAt(ls []os.FileInfo, offset int64) (int, error) {
+ var n int
+ if offset >= int64(len(f)) {
+ return 0, io.EOF
+ }
+ n = copy(ls, f[offset:])
+ if n < len(ls) {
+ return n, io.EOF
+ }
+ return n, nil
+}
+
+func (fs *root) Filelist(r Request) (ListerAt, error) {
fs.filesLock.Lock()
defer fs.filesLock.Unlock()
+
switch r.Method {
case "List":
- var err error
- batch_size := 10
- current_offset := 0
- if token := r.LsNext(); token != "" {
- current_offset, err = strconv.Atoi(token)
- if err != nil {
- return nil, os.ErrInvalid
- }
- }
ordered_names := []string{}
for fn, _ := range fs.files {
if filepath.Dir(fn) == r.Filepath {
@@ -126,21 +132,13 @@
for i, fn := range ordered_names {
list[i] = fs.files[fn]
}
- if len(list) < current_offset {
- return nil, io.EOF
- }
- new_offset := current_offset + batch_size
- if new_offset > len(list) {
- new_offset = len(list)
- }
- r.LsSave(strconv.Itoa(new_offset))
- return list[current_offset:new_offset], nil
+ return listerat(list), nil
case "Stat":
file, err := fs.fetch(r.Filepath)
if err != nil {
return nil, err
}
- return []os.FileInfo{file}, nil
+ return listerat([]os.FileInfo{file}), nil
case "Readlink":
file, err := fs.fetch(r.Filepath)
if err != nil {
@@ -152,7 +150,7 @@
return nil, err
}
}
- return []os.FileInfo{file}, nil
+ return listerat([]os.FileInfo{file}), nil
}
return nil, nil
}
diff --git a/request-interfaces.go b/request-interfaces.go
index 46db449..f7afb14 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -23,8 +23,16 @@
Filecmd(Request) error
}
-// FileInfoer should return file listing info and errors (readdir, stat)
-// note stat requests would return a list of 1
-type FileInfoer interface {
- Fileinfo(Request) ([]os.FileInfo, error)
+// FileLister should return file info interface and errors (readdir, stat)
+type FileLister interface {
+ Filelist(Request) (ListerAt, error)
+}
+
+// ListerAt does for file lists what io.ReaderAt does for files.
+// ListerAt should return the number of entries copied and an io.EOF
+// error if at end of list. This is testable by comparing how many you
+// copied to how many could be copied (eg. n < len(ls) below).
+// The copy() builtin is best for the copying.
+type ListerAt interface {
+ ListAt([]os.FileInfo, int64) (int, error)
}
diff --git a/request-server.go b/request-server.go
index b51a10b..b0cad92 100644
--- a/request-server.go
+++ b/request-server.go
@@ -21,7 +21,7 @@
FileGet FileReader
FilePut FileWriter
FileCmd FileCmder
- FileInfo FileInfoer
+ FileList FileLister
}
// RequestServer abstracts the sftp protocol with an http request-like protocol
diff --git a/request-server_test.go b/request-server_test.go
index ee9621b..60aed2a 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -315,6 +315,7 @@
func TestRequestReaddir(t *testing.T) {
p := clientRequestServerPair(t)
+ MaxFilelist = 22 // make not divisible by our test amount (100)
defer p.Close()
for i := 0; i < 100; i++ {
fname := fmt.Sprintf("/foo_%02d", i)
diff --git a/request.go b/request.go
index 6d7d6ff..795f800 100644
--- a/request.go
+++ b/request.go
@@ -11,6 +11,9 @@
"github.com/pkg/errors"
)
+// MaxFilelist is the max number of files to return in a readdir batch.
+var MaxFilelist int64 = 100
+
// Request contains the data and state for the incoming service request.
type Request struct {
// Get, Put, Setstat, Stat, Rename, Remove
@@ -29,10 +32,11 @@
}
type state struct {
- writerAt io.WriterAt
- readerAt io.ReaderAt
- endofdir bool // in case handler doesn't use EOF on file list
- readdirToken string
+ writerAt io.WriterAt
+ readerAt io.ReaderAt
+ listerAt ListerAt
+ endofdir bool // in case handler doesn't use EOF on file list
+ lsoffset int64
}
type packet_data struct {
@@ -68,20 +72,13 @@
return request
}
-// LsSave takes a token to keep track of file list batches. Openssh uses a
-// batch size of 100, so I suggest sticking close to that.
-func (r Request) LsSave(token string) {
+// Returns current offset for file list, and sets next offset
+func (r Request) lsNext(offset int64) (current int64) {
r.stateLock.RLock()
defer r.stateLock.RUnlock()
- r.state.readdirToken = token
-}
-
-// LsNext should return the token from the previous call to know which batch
-// to return next.
-func (r Request) LsNext() string {
- r.stateLock.RLock()
- defer r.stateLock.RUnlock()
- return r.state.readdirToken
+ current = r.state.lsoffset
+ r.state.lsoffset = r.state.lsoffset + offset
+ return current
}
// manage file read/write state
@@ -93,7 +90,10 @@
r.state.writerAt = s
case io.ReaderAt:
r.state.readerAt = s
-
+ case ListerAt:
+ r.state.listerAt = s
+ case int64:
+ r.state.lsoffset = s
}
}
@@ -109,6 +109,12 @@
return r.state.readerAt
}
+func (r Request) getLister() ListerAt {
+ r.stateLock.RLock()
+ defer r.stateLock.RUnlock()
+ return r.state.listerAt
+}
+
// For backwards compatibility. The Handler didn't have batch handling at
// first, and just always assumed 1 batch. This preserves that behavior.
func (r Request) setEOD(eod bool) {
@@ -157,7 +163,7 @@
case "Setstat", "Rename", "Rmdir", "Mkdir", "Symlink", "Remove":
rpkt, err = filecmd(handlers.FileCmd, r)
case "List", "Stat", "Readlink":
- rpkt, err = fileinfo(handlers.FileInfo, r)
+ rpkt, err = filelist(handlers.FileList, r)
default:
return rpkt, errors.Errorf("unexpected method: %s", r.Method)
}
@@ -179,6 +185,7 @@
pd := r.popPacket()
data := make([]byte, clamp(pd.length, maxTxPacket))
n, err := reader.ReadAt(data, pd.offset)
+ // only return EOF erro if no data left to read
if err != nil && (err != io.EOF || n == 0) {
return nil, err
}
@@ -226,21 +233,45 @@
}}, nil
}
-// wrap FileInfoer handler
-func fileinfo(h FileInfoer, r Request) (responsePacket, error) {
- if r.getEOD() {
- return nil, io.EOF
+// wrap FileLister handler
+func filelist(h FileLister, r Request) (responsePacket, error) {
+ var err error
+ lister := r.getLister()
+ if lister == nil {
+ lister, err = h.Filelist(r)
+ if err != nil {
+ return nil, err
+ }
+ r.setFileState(lister)
}
- finfo, err := h.Fileinfo(r)
- if err != nil {
+
+ offset := r.lsNext(MaxFilelist)
+ finfo := make([]os.FileInfo, MaxFilelist)
+ n, err := lister.ListAt(finfo, offset)
+ // ignore EOF as we only return it when there are no results
+ if err != nil && err != io.EOF {
return nil, err
}
+ finfo = finfo[:n] // avoid need for nil tests below
+
+ // no results
+ if n == 0 {
+ switch r.Method {
+ case "List":
+ return nil, io.EOF
+ case "Stat", "Readlink":
+ err = &os.PathError{Op: "readlink", Path: r.Filepath,
+ Err: syscall.ENOENT}
+ return nil, err
+ }
+ }
switch r.Method {
case "List":
pd := r.popPacket()
dirname := path.Base(r.Filepath)
ret := &sshFxpNamePacket{ID: pd.id}
+
for _, fi := range finfo {
ret.NameAttrs = append(ret.NameAttrs, sshFxpNameAttr{
Name: fi.Name(),
@@ -248,31 +279,13 @@
Attrs: []interface{}{fi},
})
}
- // No entries means we should return EOF as the Handler didn't.
- if len(finfo) == 0 {
- return nil, io.EOF
- }
- // If files are returned but no token is set, return EOF next call.
- if r.LsNext() == "" {
- r.setEOD(true)
- }
return ret, nil
case "Stat":
- if len(finfo) == 0 {
- err = &os.PathError{Op: "stat", Path: r.Filepath,
- Err: syscall.ENOENT}
- return nil, err
- }
return &sshFxpStatResponse{
ID: r.pkt_id,
info: finfo[0],
}, nil
case "Readlink":
- if len(finfo) == 0 {
- err = &os.PathError{Op: "readlink", Path: r.Filepath,
- Err: syscall.ENOENT}
- return nil, err
- }
filename := finfo[0].Name()
return &sshFxpNamePacket{
ID: r.pkt_id,
@@ -282,8 +295,9 @@
Attrs: emptyFileStat,
}},
}, nil
+ default:
+ return nil, errors.Errorf("unexpected method: %s", r.Method)
}
- return nil, err
}
// file data for additional read/write packets
diff --git a/request_test.go b/request_test.go
index e537fc1..d08d54b 100644
--- a/request_test.go
+++ b/request_test.go
@@ -39,7 +39,7 @@
return nil
}
-func (t *testHandler) Fileinfo(r Request) ([]os.FileInfo, error) {
+func (t *testHandler) Filelist(r Request) (ListerAt, error) {
if t.err != nil {
return nil, t.err
}
@@ -51,7 +51,7 @@
if err != nil {
return nil, err
}
- return []os.FileInfo{fi}, nil
+ return listerat([]os.FileInfo{fi}), nil
}
// make sure len(fakefile) == len(filecontents)
@@ -98,7 +98,7 @@
FileGet: handler,
FilePut: handler,
FileCmd: handler,
- FileInfo: handler,
+ FileList: handler,
}
}
