From 9f4f9ab35344758515a8ec2452563002487e8b5e Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Wed, 24 Nov 2021 19:21:02 +0700 Subject: [PATCH] Addressed review comments Signed-off-by: Oleg Solodkov --- pkg/networkservice/chains/nsmgr/unix_test.go | 13 +------------ .../common/mechanisms/recvfd/common.go | 5 ++--- .../common/mechanisms/recvfd/server.go | 9 +++++---- pkg/registry/common/recvfd/server.go | 14 +++++++------- 4 files changed, 15 insertions(+), 26 deletions(-) diff --git a/pkg/networkservice/chains/nsmgr/unix_test.go b/pkg/networkservice/chains/nsmgr/unix_test.go index 3787fd8d2..36e4385ff 100644 --- a/pkg/networkservice/chains/nsmgr/unix_test.go +++ b/pkg/networkservice/chains/nsmgr/unix_test.go @@ -14,13 +14,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -// +build !windows +// +build linux package nsmgr_test import ( "context" - "runtime" "testing" "time" @@ -38,10 +37,6 @@ import ( "github.com/networkservicemesh/sdk/pkg/tools/sandbox" ) -const ( - linuxOsName = "linux" -) - func Test_Local_NoURLUsecase(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) }) @@ -88,9 +83,6 @@ func Test_Local_NoURLUsecase(t *testing.T) { } func Test_MultiForwarderSendfd(t *testing.T) { - if runtime.GOOS != linuxOsName { - t.Skip("sendfd works only on linux") - } t.Cleanup(func() { goleak.VerifyNone(t) }) ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) @@ -166,9 +158,6 @@ func Test_MultiForwarderSendfd(t *testing.T) { } func Test_TimeoutRecvfd(t *testing.T) { - if runtime.GOOS != linuxOsName { - t.Skip("recvfd works only on linux") - } t.Cleanup(func() { goleak.VerifyNone(t) }) ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) diff --git a/pkg/networkservice/common/mechanisms/recvfd/common.go b/pkg/networkservice/common/mechanisms/recvfd/common.go index 09415e792..00c00a81d 100644 --- a/pkg/networkservice/common/mechanisms/recvfd/common.go +++ b/pkg/networkservice/common/mechanisms/recvfd/common.go @@ -86,7 +86,7 @@ func recvFDAndSwapInodeToFile(ctx context.Context, fileMap *perConnectionFileMap return err } -func swapFileToInode(fileMap *perConnectionFileMap, parameters map[string]string, closeAllFiles bool) error { +func swapFileToInode(fileMap *perConnectionFileMap, parameters map[string]string) error { // Get the inodeURL from parameters fileURLStr, ok := parameters[common.InodeURL] if !ok { @@ -112,11 +112,10 @@ func swapFileToInode(fileMap *perConnectionFileMap, parameters map[string]string // Swap the fileURL for the inodeURL in parameters parameters[common.InodeURL] = inodeURL.String() - // If closeAllFiles == true, close any files we may have open for any other inodes // This is used to clean up files sent by MechanismPreferences that were *not* selected to be the // connection mechanism for inodeURLStr, file := range fileMap.filesByInodeURL { - if closeAllFiles || inodeURLStr != inodeURL.String() { + if inodeURLStr != inodeURL.String() { delete(fileMap.filesByInodeURL, inodeURLStr) _ = file.Close() } diff --git a/pkg/networkservice/common/mechanisms/recvfd/server.go b/pkg/networkservice/common/mechanisms/recvfd/server.go index f45162693..805de5450 100644 --- a/pkg/networkservice/common/mechanisms/recvfd/server.go +++ b/pkg/networkservice/common/mechanisms/recvfd/server.go @@ -69,7 +69,7 @@ func (r *recvFDServer) Request(ctx context.Context, request *networkservice.Netw } // Swap back from File to Inode in the InodeURL in the Parameters - err = swapFileToInode(fileMap, conn.GetMechanism().GetParameters(), false) + err = swapFileToInode(fileMap, conn.GetMechanism().GetParameters()) if err != nil { return nil, err } @@ -79,12 +79,11 @@ func (r *recvFDServer) Request(ctx context.Context, request *networkservice.Netw func (r *recvFDServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { // Clean up the fileMap no matter what happens - defer r.fileMaps.Delete(conn.GetId()) + defer r.closeFiles(conn) // Get the grpcfd.FDRecver recv, ok := grpcfd.FromContext(ctx) if !ok { - r.closeFiles(conn) return next.Server(ctx).Close(ctx, conn) } @@ -107,11 +106,13 @@ func (r *recvFDServer) Close(ctx context.Context, conn *networkservice.Connectio } // Swap back from File to Inode in the InodeURL in the Parameters - err = swapFileToInode(fileMap, conn.GetMechanism().GetParameters(), true) + err = swapFileToInode(fileMap, conn.GetMechanism().GetParameters()) return &empty.Empty{}, err } func (r *recvFDServer) closeFiles(conn *networkservice.Connection) { + defer r.fileMaps.Delete(conn.GetId()) + fileMap, _ := r.fileMaps.LoadOrStore(conn.GetId(), &perConnectionFileMap{ filesByInodeURL: make(map[string]*os.File), inodeURLbyFilename: make(map[string]*url.URL), diff --git a/pkg/registry/common/recvfd/server.go b/pkg/registry/common/recvfd/server.go index 051c0d254..84be895e1 100644 --- a/pkg/registry/common/recvfd/server.go +++ b/pkg/registry/common/recvfd/server.go @@ -83,7 +83,7 @@ func (r *recvfdNseServer) Register(ctx context.Context, endpoint *registry.Netwo r.fileMaps.Store(endpoint.GetName(), fileMap) } // Swap back from File to Inode in the InodeURL in the Parameters - err = swapFileToInode(fileMap, returnedEndpoint, false) + err = swapFileToInode(fileMap, returnedEndpoint) if err != nil { return nil, err } @@ -100,12 +100,11 @@ func (r *recvfdNseServer) Unregister(ctx context.Context, endpoint *registry.Net } // Clean up the fileMap no matter what happens - defer r.fileMaps.Delete(endpoint.GetName()) + defer r.closeFiles(endpoint) // Get the grpcfd.FDRecver recv, ok := grpcfd.FromContext(ctx) if !ok { - r.closeFiles(endpoint) return next.NetworkServiceEndpointRegistryServer(ctx).Unregister(ctx, endpoint) } // Get the fileMap @@ -129,7 +128,7 @@ func (r *recvfdNseServer) Unregister(ctx context.Context, endpoint *registry.Net // Swap back from File to Inode in the InodeURL in the Parameters endpoint = endpoint.Clone() - err = swapFileToInode(fileMap, endpoint, true) + err = swapFileToInode(fileMap, endpoint) if err != nil { return nil, err } @@ -181,7 +180,7 @@ func recvFDAndSwapInodeToUnix(ctx context.Context, fileMap *perEndpointFileMap, return err } -func swapFileToInode(fileMap *perEndpointFileMap, endpoint *registry.NetworkServiceEndpoint, closeAllFiles bool) error { +func swapFileToInode(fileMap *perEndpointFileMap, endpoint *registry.NetworkServiceEndpoint) error { // Transform string to URL for correctness checking and ease of use unixURL, err := url.Parse(endpoint.GetUrl()) if err != nil { @@ -201,11 +200,10 @@ func swapFileToInode(fileMap *perEndpointFileMap, endpoint *registry.NetworkServ // Swap the fileURL for the inodeURL in parameters endpoint.Url = inodeURL.String() - // If closeAllFiles == true, close any files we may have open for any other inodes // This is used to clean up files sent by MechanismPreferences that were *not* selected to be the // connection mechanism for inodeURLStr, file := range fileMap.filesByInodeURL { - if closeAllFiles || inodeURLStr != inodeURL.String() { + if inodeURLStr != inodeURL.String() { delete(fileMap.filesByInodeURL, inodeURLStr) _ = file.Close() } @@ -215,6 +213,8 @@ func swapFileToInode(fileMap *perEndpointFileMap, endpoint *registry.NetworkServ } func (r *recvfdNseServer) closeFiles(endpoint *registry.NetworkServiceEndpoint) { + defer r.fileMaps.Delete(endpoint.GetName()) + fileMap, _ := r.fileMaps.LoadOrStore(endpoint.GetName(), &perEndpointFileMap{ filesByInodeURL: make(map[string]*os.File), inodeURLbyFilename: make(map[string]*url.URL),