From 1e1d6b2be37873c5e62461834df973f41565c662 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Tue, 2 Nov 2021 12:26:18 -0700 Subject: Allow SetAttr and Allocate for deleted files It's safe to call SetAttr and Allocate on fsgofer because the file path is not used to open the file, if needed. Fixes #3654 PiperOrigin-RevId: 407149393 --- pkg/p9/file.go | 35 ++++++++++++++++++++++++++++++++--- pkg/p9/handlers.go | 6 +++--- pkg/p9/p9test/BUILD | 2 +- pkg/p9/p9test/p9test.go | 1 + pkg/p9/server.go | 9 ++++++++- 5 files changed, 45 insertions(+), 8 deletions(-) (limited to 'pkg') diff --git a/pkg/p9/file.go b/pkg/p9/file.go index 8d6af2d6b..b4b556cb9 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -21,13 +21,37 @@ import ( "gvisor.dev/gvisor/pkg/fd" ) +// AttacherOptions contains Attacher configuration. +type AttacherOptions struct { + // SetAttrOnDeleted is set to true if it's safe to call File.SetAttr for + // deleted files. + SetAttrOnDeleted bool + + // AllocateOnDeleted is set to true if it's safe to call File.Allocate for + // deleted files. + AllocateOnDeleted bool +} + +// NoServerOptions partially implements Attacher with empty AttacherOptions. +type NoServerOptions struct{} + +// ServerOptions implements Attacher. +func (*NoServerOptions) ServerOptions() AttacherOptions { + return AttacherOptions{} +} + // Attacher is provided by the server. type Attacher interface { // Attach returns a new File. // - // The client-side attach will be translate to a series of walks from + // The client-side attach will be translated to a series of walks from // the file returned by this Attach call. Attach() (File, error) + + // ServerOptions returns configuration options for this attach point. + // + // This is never caller in the client-side. + ServerOptions() AttacherOptions } // File is a set of operations corresponding to a single node. @@ -301,7 +325,7 @@ type File interface { type DefaultWalkGetAttr struct{} // WalkGetAttr implements File.WalkGetAttr. -func (DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) { +func (*DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) { return nil, nil, AttrMask{}, Attr{}, unix.ENOSYS } @@ -309,7 +333,7 @@ func (DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, er type DisallowClientCalls struct{} // SetAttrClose implements File.SetAttrClose. -func (DisallowClientCalls) SetAttrClose(SetAttrMask, SetAttr) error { +func (*DisallowClientCalls) SetAttrClose(SetAttrMask, SetAttr) error { panic("SetAttrClose should not be called on the server") } @@ -321,6 +345,11 @@ func (*DisallowServerCalls) Renamed(File, string) { panic("Renamed should not be called on the client") } +// ServerOptions implements Attacher. +func (*DisallowServerCalls) ServerOptions() AttacherOptions { + panic("ServerOptions should not be called on the client") +} + // DefaultMultiGetAttr implements File.MultiGetAttr() on top of File. func DefaultMultiGetAttr(start File, names []string) ([]FullStat, error) { stats := make([]FullStat, 0, len(names)) diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 2657081e3..c85af5e9e 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -178,7 +178,7 @@ func (t *Tsetattrclunk) handle(cs *connState) message { // This might be technically incorrect, as it's possible that // there were multiple links and you can still change the // corresponding inode information. - if ref.isDeleted() { + if !cs.server.options.SetAttrOnDeleted && ref.isDeleted() { return unix.EINVAL } @@ -913,7 +913,7 @@ func (t *Tsetattr) handle(cs *connState) message { // This might be technically incorrect, as it's possible that // there were multiple links and you can still change the // corresponding inode information. - if ref.isDeleted() { + if !cs.server.options.SetAttrOnDeleted && ref.isDeleted() { return unix.EINVAL } @@ -946,7 +946,7 @@ func (t *Tallocate) handle(cs *connState) message { } // We don't allow allocate on files that have been deleted. - if ref.isDeleted() { + if !cs.server.options.AllocateOnDeleted && ref.isDeleted() { return unix.EINVAL } diff --git a/pkg/p9/p9test/BUILD b/pkg/p9/p9test/BUILD index 9c1ada0cb..f3eb8468b 100644 --- a/pkg/p9/p9test/BUILD +++ b/pkg/p9/p9test/BUILD @@ -12,7 +12,7 @@ MOCK_SRC_PACKAGE = "gvisor.dev/gvisor/pkg/p9" # mockgen_reflect is a source file that contains mock generation code that # imports the p9 package and generates a specification via reflection. The # usual generation path must be split into two distinct parts because the full -# source tree is not available to all build targets. Only declared depencies +# source tree is not available to all build targets. Only declared dependencies # are available (and even then, not the Go source files). genrule( name = "mockgen_reflect", diff --git a/pkg/p9/p9test/p9test.go b/pkg/p9/p9test/p9test.go index fd5ac3dbe..56939d100 100644 --- a/pkg/p9/p9test/p9test.go +++ b/pkg/p9/p9test/p9test.go @@ -307,6 +307,7 @@ func NewHarness(t *testing.T) (*Harness, *p9.Client) { } // Start the server, synchronized on exit. + h.Attacher.EXPECT().ServerOptions().Return(p9.AttacherOptions{}).Times(1) server := p9.NewServer(h.Attacher) h.wg.Add(1) go func() { diff --git a/pkg/p9/server.go b/pkg/p9/server.go index 6428ad745..e7d129f9d 100644 --- a/pkg/p9/server.go +++ b/pkg/p9/server.go @@ -34,6 +34,8 @@ type Server struct { // attacher provides the attach function. attacher Attacher + options AttacherOptions + // pathTree is the full set of paths opened on this server. // // These may be across different connections, but rename operations @@ -48,10 +50,15 @@ type Server struct { renameMu sync.RWMutex } -// NewServer returns a new server. +// NewServer returns a new server. attacher may be nil. func NewServer(attacher Attacher) *Server { + opts := AttacherOptions{} + if attacher != nil { + opts = attacher.ServerOptions() + } return &Server{ attacher: attacher, + options: opts, pathTree: newPathNode(), } } -- cgit v1.2.3