Skip to content

Commit

Permalink
Addressed review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
  • Loading branch information
@sol-0
sol-0 committed Nov 24, 2021
1 parent 1895c74 commit 9f4f9ab
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 26 deletions.
13 changes: 1 addition & 12 deletions pkg/networkservice/chains/nsmgr/unix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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) })

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 2 additions & 3 deletions pkg/networkservice/common/mechanisms/recvfd/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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()
}
Expand Down
9 changes: 5 additions & 4 deletions pkg/networkservice/common/mechanisms/recvfd/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
}

Expand All @@ -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),
Expand Down
14 changes: 7 additions & 7 deletions pkg/registry/common/recvfd/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
}
Expand All @@ -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),
Expand Down

0 comments on commit 9f4f9ab

Please sign in to comment.