Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid text file busy error on track #2494

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+120 −22
Open

avoid text file busy error on track #2494

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9119392
avoid text file busy error on track
felipemadero Dec 20, 2024
081f826
Merge branch 'main' into fix-text-file-busy-while-tracj
felipemadero Jan 14, 2025
7430f84
Merge branch 'main' into fix-text-file-busy-while-tracj
felipemadero Jan 18, 2025
9116cf8
moved exec setup to utils and added unit
felipemadero Jan 18, 2025
7f33e6d
lint
felipemadero Jan 18, 2025
3751999
fix
felipemadero Jan 18, 2025
0ee6c95
Merge branch 'main' into fix-text-file-busy-while-tracj
felipemadero Feb 24, 2025
5d340a1
address PR comments
felipemadero Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions pkg/localnet/tmpnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,13 +382,7 @@ func TmpNetInstallVM(network *tmpnet.Network, binaryPath string, vmID ids.ID) er
return err
}
pluginPath := filepath.Join(pluginDir, vmID.String())
if err := utils.FileCopy(binaryPath, pluginPath); err != nil {
return err
}
if err := os.Chmod(pluginPath, constants.DefaultPerms755); err != nil {
return err
}
return nil
return utils.SetupExecFile(binaryPath, pluginPath)
}

// Set up blockchain config for all nodes in the network
Expand Down
6 changes: 2 additions & 4 deletions pkg/node/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,9 @@ func TrackSubnetWithLocalMachine(
return fmt.Errorf("unknown vm: %s", sc.VM)
}
rootDir := app.GetLocalDir(clusterName)

pluginPath := filepath.Join(rootDir, "node1", "plugins", vmID.String())
if err := utils.FileCopy(vmBin, pluginPath); err != nil {
return err
}
if err := os.Chmod(pluginPath, constants.DefaultPerms755); err != nil {
if err := utils.SetupExecFile(vmBin, pluginPath); err != nil {
return err
}

Expand Down
20 changes: 19 additions & 1 deletion pkg/utils/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func IsExecutable(filename string) bool {
return false
}
info, _ := os.Stat(filename)
return info.Mode()&0x0100 != 0
return info.Mode()&0o0100 != 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this &0o0100?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0o0100 (better 0o100, will fix), is a bit mask that, when applying bitwise AND (&) results
in a != 0 state whenever the file can be executed by the owner.
A perm can be seen as a 9-bit sequence mapping to rwxrwxrwx read-write-execute
for owner,group,everybody.
octal 0o100 means binary 001000000.
Probably better to set a constant and also add a comment

Copy link
Collaborator Author

@felipemadero felipemadero Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It previously was 0x0100 == binary 100000000. this was a bug, filtering in combinations where
the owner has read perms

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, would you mind adding a comment that explains this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

}

// UserHomePath returns the absolute path of a file located in the user's home directory.
Expand Down Expand Up @@ -77,6 +77,24 @@ func FileCopy(src string, dst string) error {
return os.WriteFile(dst, data, constants.WriteReadReadPerms)
}

// SetupExecFile copies a file into destination and set it to have exec perms,
// if destination either does not exists, or is not executable
func SetupExecFile(src string, dst string) error {
if !IsExecutable(dst) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to error/log if the file isn't executable?

Copy link
Collaborator Author

@felipemadero felipemadero Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to error. This also happens on first install.
We are not going to error on first install, and we prefer to try to
recover if a previous install failed.
Probably we can add log for a failed previous install, while trying to recover.

// Either it was never installed, or it was partially done (copy or chmod
// failure)
// As the file is not executable, there is no risk of encountering text file busy
// error during copy, because that happens when the binary is being executed.
if err := FileCopy(src, dst); err != nil {
return err
}
if err := os.Chmod(dst, constants.DefaultPerms755); err != nil {
return err
}
}
return nil
}

// ReadFile reads a file and returns the contents as a string
func ReadFile(filePath string) (string, error) {
filePath = ExpandHome(filePath)
Expand Down
80 changes: 72 additions & 8 deletions pkg/utils/file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import (
"os"
"path/filepath"
"testing"

"github.com/ava-labs/avalanche-cli/pkg/constants"
"github.com/stretchr/testify/require"
)

func TestExpandHome(t *testing.T) {
Expand Down Expand Up @@ -45,29 +48,26 @@ func TestExpandHome(t *testing.T) {
}
}

// createTempGoMod creates a temporary go.mod file with the provided content.
func createTempGoMod(t *testing.T, content string) string {
// createTemp creates a temporary file with the provided name prefix and content.
func createTemp(t *testing.T, namePrefix string, content string) string {
t.Helper()
file, err := os.CreateTemp("", "go.mod")
file, err := os.CreateTemp("", namePrefix)
if err != nil {
t.Fatal(err)
}

if _, err := file.Write([]byte(content)); err != nil {
t.Fatal(err)
}

if err := file.Close(); err != nil {
t.Fatal(err)
}

return file.Name()
}

// TestReadGoVersion tests all scenarios in one function using sub-tests.
func TestReadGoVersion(t *testing.T) {
t.Run("Success", func(t *testing.T) {
tempFile := createTempGoMod(t, "module example.com/test\n\ngo 1.23\n")
tempFile := createTemp(t, "go.mod", "module example.com/test\n\ngo 1.23\n")
defer os.Remove(tempFile) // Clean up the temp file

version, err := ReadGoVersion(tempFile)
Expand All @@ -82,7 +82,7 @@ func TestReadGoVersion(t *testing.T) {
})

t.Run("NoVersion", func(t *testing.T) {
tempFile := createTempGoMod(t, "module example.com/test\n")
tempFile := createTemp(t, "go.mod", "module example.com/test\n")
defer os.Remove(tempFile)

_, err := ReadGoVersion(tempFile)
Expand All @@ -98,3 +98,67 @@ func TestReadGoVersion(t *testing.T) {
}
})
}

func TestSetupExecFile(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see this test

srcContent := "src content"
destContent := "dest content"
t.Run("Src does not exists", func(t *testing.T) {
src := createTemp(t, "testexecfile", srcContent)
dest := createTemp(t, "testexecfile", destContent)
err := os.Remove(src)
require.NoError(t, err)
require.Equal(t, false, FileExists(src))
require.Equal(t, true, FileExists(dest))
require.Equal(t, false, IsExecutable(dest))
err = SetupExecFile(src, dest)
require.Error(t, err)
content, err := os.ReadFile(dest)
require.NoError(t, err)
require.Equal(t, true, FileExists(dest))
require.Equal(t, false, IsExecutable(dest))
require.Equal(t, destContent, string(content))
})
t.Run("Dest does not exists", func(t *testing.T) {
src := createTemp(t, "testexecfile", srcContent)
dest := createTemp(t, "testexecfile", destContent)
err := os.Remove(dest)
require.NoError(t, err)
require.Equal(t, false, FileExists(dest))
require.Equal(t, false, IsExecutable(dest))
err = SetupExecFile(src, dest)
require.NoError(t, err)
content, err := os.ReadFile(dest)
require.NoError(t, err)
require.Equal(t, true, FileExists(dest))
require.Equal(t, true, IsExecutable(dest))
require.Equal(t, srcContent, string(content))
})
t.Run("Dest is not executable", func(t *testing.T) {
src := createTemp(t, "testexecfile", srcContent)
dest := createTemp(t, "testexecfile", destContent)
require.Equal(t, true, FileExists(dest))
require.Equal(t, false, IsExecutable(dest))
err := SetupExecFile(src, dest)
require.NoError(t, err)
content, err := os.ReadFile(dest)
require.NoError(t, err)
require.Equal(t, true, FileExists(dest))
require.Equal(t, true, IsExecutable(dest))
require.Equal(t, srcContent, string(content))
})
t.Run("Dest is already executable", func(t *testing.T) {
src := createTemp(t, "testexecfile", srcContent)
dest := createTemp(t, "testexecfile", destContent)
err := os.Chmod(dest, constants.DefaultPerms755)
require.NoError(t, err)
require.Equal(t, true, FileExists(dest))
require.Equal(t, true, IsExecutable(dest))
err = SetupExecFile(src, dest)
require.NoError(t, err)
content, err := os.ReadFile(dest)
require.NoError(t, err)
require.Equal(t, true, FileExists(dest))
require.Equal(t, true, IsExecutable(dest))
require.Equal(t, destContent, string(content))
})
}
Loading