From d8eb08f9d9cf7918cd679a18a4a10b4ebe9c598a Mon Sep 17 00:00:00 2001 From: zakkor Date: Sun, 11 Sep 2022 10:03:00 +0300 Subject: [PATCH 1/6] Revert "Merge pull request #30 from seh-msft/rwalk_fix" This reverts commit 72aa6a6b1a664bf7fba53a428bce9f2d77175904, reversing changes made to 0320e6f6d7b1569ef749a76337347e36e2a72461. --- conn.go | 4 ---- go.mod | 5 ----- go.sum | 2 -- internal/qidpool/pool.go | 1 - request.go | 4 ++-- walk.go | 8 +------- 6 files changed, 3 insertions(+), 21 deletions(-) delete mode 100644 go.mod delete mode 100644 go.sum diff --git a/conn.go b/conn.go index bcbc21e..aa632ac 100644 --- a/conn.go +++ b/conn.go @@ -161,10 +161,6 @@ func (c *conn) qid(name string, qtype uint8) styxproto.Qid { return c.qidpool.Put(name, qtype) } -func (c *conn) getQid(name string, qtype uint8) (styxproto.Qid, bool) { - return c.qidpool.Get(name) -} - // All request contexts must have their cancel functions // called, to free up resources in the context. Returns false // if the tag is already cancelled diff --git a/go.mod b/go.mod deleted file mode 100644 index 7b14752..0000000 --- a/go.mod +++ /dev/null @@ -1,5 +0,0 @@ -module aqwari.net/net/styx - -go 1.16 - -require aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0 diff --git a/go.sum b/go.sum deleted file mode 100644 index 88e95d4..0000000 --- a/go.sum +++ /dev/null @@ -1,2 +0,0 @@ -aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0 h1:BeD6U5TNwhMWxeydyi5xqpaNZx1MWl5QTcW4w7Mxf+Y= -aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0/go.mod h1:XSNyyoM+OSg3vRmROPrS1lEpV7q/I9J1HAKMMxdUkU4= diff --git a/internal/qidpool/pool.go b/internal/qidpool/pool.go index 84ef385..5489782 100644 --- a/internal/qidpool/pool.go +++ b/internal/qidpool/pool.go @@ -42,7 +42,6 @@ func (p *Pool) Put(name string, qtype uint8) styxproto.Qid { m[name] = qid } }) - p.m.Put(name, qid) return qid } diff --git a/request.go b/request.go index c7be0e9..d9aed12 100644 --- a/request.go +++ b/request.go @@ -1,10 +1,11 @@ package styx import ( - "context" "os" "path" + "context" + "aqwari.net/net/styx/internal/styxfile" "aqwari.net/net/styx/internal/sys" "aqwari.net/net/styx/styxproto" @@ -265,7 +266,6 @@ func (t Tcreate) Rcreate(rwc interface{}, err error) { } if dir, ok := rwc.(Directory); t.Mode.IsDir() && ok { - f = styxfile.NewDir(dir, path.Join(t.Path(), t.Name), t.session.conn.qidpool) } else { f, err = styxfile.New(rwc) diff --git a/walk.go b/walk.go index de66bf4..91c3a60 100644 --- a/walk.go +++ b/walk.go @@ -1,7 +1,6 @@ package styx import ( - "errors" "fmt" "os" "path" @@ -181,12 +180,7 @@ func (t Twalk) Rwalk(info os.FileInfo, err error) { var mode os.FileMode if err == nil { mode = info.Mode() - fqid, found := t.session.conn.getQid(t.Path(), styxfile.QidType(styxfile.Mode9P(mode))) - if !found { - err = errors.New("rwalk did not find file") - } else { - qid = fqid - } + qid = t.session.conn.qid(t.Path(), styxfile.QidType(styxfile.Mode9P(mode))) } t.walk.filled[t.index] = 1 elem := walkElem{qid: qid, index: t.index, err: err} From c13ee8213bc39127efdc65052293ce243e7a3fd8 Mon Sep 17 00:00:00 2001 From: zakkor Date: Sun, 11 Sep 2022 11:26:22 +0300 Subject: [PATCH 2/6] qidpool: Remove superfluous `threadsafe.Map.Put` call If the name already existed, then there's no need to Put it, otherwise it gets set inside the Do call, which also means there's no need to Put it. --- internal/qidpool/pool.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/qidpool/pool.go b/internal/qidpool/pool.go index 5489782..5cfbc19 100644 --- a/internal/qidpool/pool.go +++ b/internal/qidpool/pool.go @@ -42,7 +42,6 @@ func (p *Pool) Put(name string, qtype uint8) styxproto.Qid { m[name] = qid } }) - p.m.Put(name, qid) return qid } From 94e4c88d22d8558830e6cba4dfdb2dc12bfbe545 Mon Sep 17 00:00:00 2001 From: zakkor Date: Sun, 11 Sep 2022 11:26:45 +0300 Subject: [PATCH 3/6] go.mod init --- go.mod | 5 +++++ go.sum | 2 ++ 2 files changed, 7 insertions(+) create mode 100644 go.mod create mode 100644 go.sum diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..05d4ff7 --- /dev/null +++ b/go.mod @@ -0,0 +1,5 @@ +module aqwari.net/net/styx + +go 1.19 + +require aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..88e95d4 --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0 h1:BeD6U5TNwhMWxeydyi5xqpaNZx1MWl5QTcW4w7Mxf+Y= +aqwari.net/retry v0.0.0-20180428204214-1281ce5d8df0/go.mod h1:XSNyyoM+OSg3vRmROPrS1lEpV7q/I9J1HAKMMxdUkU4= From 069bcea3e84b96cc71f272475541ef48ea6d5c7c Mon Sep 17 00:00:00 2001 From: zakkor Date: Sun, 11 Sep 2022 11:28:06 +0300 Subject: [PATCH 4/6] Add test that verifies no Qid gets created after an unsuccessful Twalk --- server_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/server_test.go b/server_test.go index f88a461..cd3710b 100644 --- a/server_test.go +++ b/server_test.go @@ -488,6 +488,34 @@ func TestWalk(t *testing.T) { } } +func TestWalkNonexistent(t *testing.T) { + srv := testServer{test: t} + srv.callback = func(req, rsp styxproto.Msg) { + if _, ok := req.(styxproto.Twalk); ok { + if _, ok := rsp.(styxproto.Rerror); !ok { + t.Errorf("expected Rerror response to nonexistent Twalk, instead got: %T", rsp) + } + } + } + srv.handler = HandlerFunc(func(s *Session) { + for s.Next() { + switch req := s.Request().(type) { + case Twalk: + t.Logf("Twalk %s", req.Path()) + req.Rwalk(nil, errors.New("not found")) + // If the walk resulted in an error, then no Qid should have been created for this path + if _, ok := s.conn.qidpool.Get(req.Path()); ok { + t.Error("qid was created when it shouldn't have been") + } + } + } + }) + + srv.runMsg(func(enc *styxproto.Encoder) { + enc.Twalk(1, 0, 1, "nonexistent") + }) +} + func blankQid() styxproto.Qid { buf := make([]byte, styxproto.QidLen) qid, _, err := styxproto.NewQid(buf, 0, 0, 0) From 7e817efaae4bf7b898ea3ddc95d2a00774c5d157 Mon Sep 17 00:00:00 2001 From: zakkor Date: Thu, 15 Sep 2022 14:06:23 +0300 Subject: [PATCH 5/6] styxfile.Stat: Avoid creating bad messages through the result of fallback `statGuess` guesses We make sure the fallback statGuess name is the base file name, not the absolute path. Otherwise in scenarios where statGuess does not manage to find a Name method on the underlying object, it will return an absolute path, which will generate bad message errors, due to slashes not being allowed in file names. --- internal/styxfile/file.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/styxfile/file.go b/internal/styxfile/file.go index e6be429..aee997c 100644 --- a/internal/styxfile/file.go +++ b/internal/styxfile/file.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + "path/filepath" "time" "aqwari.net/net/styx/internal/sys" @@ -110,6 +111,10 @@ func Stat(buf []byte, file Interface, name string, qid styxproto.Qid) (styxproto return nil, err } } else { + // name is an absolute path, make sure we don't pass an absolute path to statGuess, + // otherwise we may get back an absolute path if the file does not have a Name() method, + // which would be incorrect since stat names cannot contain slashes. + name := filepath.Base(name) fi = statGuess{file, name, qid.Type()} } uid, gid, muid := sys.FileOwner(fi) From ce9c87ae19daf43d7f882737fb630184130f7274 Mon Sep 17 00:00:00 2001 From: zakkor Date: Thu, 15 Sep 2022 14:07:13 +0300 Subject: [PATCH 6/6] test: Add TestTcreate (partially failing) When a styxfile.Directory is passed as the result of a Tcreate call and the underlying object does not have a Stat method, statGuess does not manage to detect any underlying fs.FileInfo methods, (e.g.: Name, Mode, ...) which means that for opened directories, the results of Tstat are always an incorrectly guessed fallback value --- internal/styxfile/file.go | 2 + server_test.go | 110 +++++++++++++++++++++++++++++++++----- 2 files changed, 100 insertions(+), 12 deletions(-) diff --git a/internal/styxfile/file.go b/internal/styxfile/file.go index aee997c..0d1b01a 100644 --- a/internal/styxfile/file.go +++ b/internal/styxfile/file.go @@ -130,6 +130,8 @@ func Stat(buf []byte, file Interface, name string, qid styxproto.Qid) (styxproto return stat, nil } +// FIXME: When statGuess is used with a styxfile.Directory, none of the stat methods are found, +// and we fall back on incorrectly using guessed values every time. type statGuess struct { file Interface name string diff --git a/server_test.go b/server_test.go index cd3710b..4a9103d 100644 --- a/server_test.go +++ b/server_test.go @@ -14,6 +14,7 @@ import ( "time" "aqwari.net/net/styx/internal/netutil" + "aqwari.net/net/styx/internal/styxfile" "aqwari.net/net/styx/styxproto" ) @@ -86,25 +87,46 @@ func (emptyFS) Serve9P(s *Session) { switch req := s.Request().(type) { case Tstat: if req.Path() == "/" { - req.Rstat(emptyDir(req.Path()), nil) + req.Rstat(emptyDir{emptyStatDir(req.Path())}, nil) } case Topen: - req.Ropen(emptyDir(req.Path()), nil) + req.Ropen(emptyDir{emptyStatDir(req.Path())}, nil) } } } -type emptyDir string +type emptyStatFile string + +// fs.FileInfo +func (s emptyStatFile) Mode() os.FileMode { return 0222 } +func (s emptyStatFile) IsDir() bool { return s.Mode().IsDir() } +func (s emptyStatFile) Name() string { return string(s) } +func (s emptyStatFile) Sys() interface{} { return nil } +func (s emptyStatFile) Size() int64 { return 0 } +func (s emptyStatFile) ModTime() time.Time { return time.Time{} } + +type emptyStatDir string + +// fs.FileInfo +func (s emptyStatDir) Mode() os.FileMode { return 0222 | os.ModeDir } +func (s emptyStatDir) IsDir() bool { return s.Mode().IsDir() } +func (s emptyStatDir) Name() string { return string(s) } +func (s emptyStatDir) Sys() interface{} { return nil } +func (s emptyStatDir) Size() int64 { return 0 } +func (s emptyStatDir) ModTime() time.Time { return time.Time{} } + +type emptyFile struct{ emptyStatFile } + +var _ styxfile.Interface = emptyFile{} + +func (f emptyFile) ReadAt(p []byte, offset int64) (written int, err error) { return 0, io.EOF } +func (f emptyFile) WriteAt(p []byte, offset int64) (int, error) { return 0, styxfile.ErrNotSupported } +func (f emptyFile) Close() error { return nil } + +type emptyDir struct{ emptyStatDir } + +var _ styxfile.Directory = emptyDir{} -// os.FileInfo -func (d emptyDir) Mode() os.FileMode { return os.ModeDir } -func (d emptyDir) IsDir() bool { return d.Mode().IsDir() } -func (d emptyDir) Name() string { return string(d) } -func (d emptyDir) Sys() interface{} { return nil } -func (d emptyDir) Size() int64 { return 0 } -func (d emptyDir) ModTime() time.Time { return time.Time{} } - -// styx.Directory func (d emptyDir) Readdir(int) ([]os.FileInfo, error) { return nil, nil } func chanServer(t *testing.T, handler Handler) (in, out chan styxproto.Msg) { @@ -516,6 +538,70 @@ func TestWalkNonexistent(t *testing.T) { }) } +func TestTcreate(t *testing.T) { + srv := testServer{test: t} + + type expectedstat struct { + name string + mode os.FileMode + } + fidnames := map[uint32]expectedstat{ + 1: {name: "dir", mode: 0222 | os.ModeDir}, + 2: {name: "file", mode: 0222}, + } + + srv.callback = func(req, rsp styxproto.Msg) { + if _, ok := rsp.(styxproto.Rerror); ok { + t.Errorf("got %T response to %T", rsp, req) + } + if req, ok := req.(styxproto.Tstat); ok { + if rsp, ok := rsp.(styxproto.Rstat); !ok { + t.Errorf("got %T response to %T", rsp, req) + } else { + expected := fidnames[req.Fid()] + name := string(rsp.Stat().Name()) + if name != expected.name { + t.Errorf("expected name to be %s, instead got %s", expected.name, name) + } + // FIXME: For directories, the mode does not match + mode := styxfile.ModeOS(rsp.Stat().Mode()) + if mode != expected.mode { + t.Errorf("expected mode to be %s, instead got %s", expected.mode, mode) + } + } + } + } + srv.handler = HandlerFunc(func(s *Session) { + for s.Next() { + switch req := s.Request().(type) { + case Tcreate: + t.Logf("Tcreate %s %s", req.Path(), req.NewPath()) + var f any + if req.Mode.IsDir() { + f = emptyDir{emptyStatDir(req.Name)} + } else { + f = emptyFile{emptyStatFile(req.Name)} + } + req.Rcreate(f, nil) + case Twalk: + // Empty walks get automatically handled, no need to handle + case Tstat: + // Because Rcreate returns an opened file, Tstat is called on styxfile.Interface or styxfile.Directory, + // so it will use styxfile.Stat to get stat, no need to handle + } + } + }) + + srv.runMsg(func(enc *styxproto.Encoder) { + enc.Twalk(1, 0, 1) + enc.Tcreate(1, 1, "dir", 0222|styxproto.DMDIR, styxproto.DMREAD) + enc.Tstat(1, 1) + enc.Twalk(1, 0, 2) + enc.Tcreate(1, 2, "file", 0222, styxproto.DMREAD) + enc.Tstat(1, 2) + }) +} + func blankQid() styxproto.Qid { buf := make([]byte, styxproto.QidLen) qid, _, err := styxproto.NewQid(buf, 0, 0, 0)