Skip to content

Add BSD support to libs/go/sia/util#3267

Open
tuftedocelot wants to merge 1 commit intoAthenZ:masterfrom
tuftedocelot:master
Open

Add BSD support to libs/go/sia/util#3267
tuftedocelot wants to merge 1 commit intoAthenZ:masterfrom
tuftedocelot:master

Conversation

@tuftedocelot
Copy link
Copy Markdown

Description

Generifies #3093 to support all BSDs

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Attach Screenshots (Optional)

@tuftedocelot
Copy link
Copy Markdown
Author

@olgeni I'm curious if this PR works for you on FreeBSD?

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new utility file, os_util_bsd.go, providing OS-level operations for BSD systems. The review feedback identifies several critical improvements: replacing log.Fatalf with error returns in library code to prevent unexpected application termination, consolidating duplicate logic across platform-specific files using Go build tags, and ensuring that errors from directory ownership changes and file walking are properly propagated rather than ignored.

I am having trouble creating individual review comments. Click here to see my feedback.

libs/go/sia/util/os_util_bsd.go (105-130)

high

The use of log.Fatalf in idCommand can cause the entire application to terminate unexpectedly. For example, if an invalid username is passed, the id command will fail, and the program will exit. Library code should avoid terminating the application and instead return errors to the caller.

I recommend refactoring idCommand and uidGidForUser to return errors, and then propagate those errors up to the callers (SvcAttrs, UidGidForUserGroup, and UpdateFile).

func idCommand(username, arg string) (int, error) {
	//shelling out to id is used here because the os/user package
	//requires cgo, which doesn't cross-compile
	var args []string
	if username == "" {
		args = []string{arg}
	} else {
		args = []string{arg, username}
	}
	cmd := exec.Command(GetUtilPath("id"), args...)
	out, err := cmd.Output()
	if err != nil {
		return 0, fmt.Errorf("cannot exec '%s': %w", cmd.String(), err)
	}
	s := strings.TrimSpace(string(out))
	id, err := strconv.Atoi(s)
	if err != nil {
		return 0, fmt.Errorf("unexpected UID/GID format from '%s': %q: %w", cmd.String(), string(out), err)
	}
	return id, nil
}

func uidGidForUser(username string) (int, int, error) {
	uid, err := idCommand(username, "-u")
	if err != nil {
		return -1, -1, err
	}
	gid, err := idCommand(username, "-g")
	if err != nil {
		return -1, -1, err
	}
	return uid, gid, nil
}

libs/go/sia/util/os_util_bsd.go (17)

medium

This new file duplicates code that is already present in os_util_linux.go, os_util_darwin.go, and os_util_freebsd.go. This will make future maintenance more difficult.

To reduce code duplication, I recommend consolidating all unix-like implementations into a single file. You could create a file named os_util_unix.go with the build tag //go:build unix at the top. The unix build tag (available in Go 1.19+) covers Linux, macOS (darwin), and all BSD variants.

This would involve:

  1. Creating os_util_unix.go with the contents of this file.
  2. Adding //go:build unix at the top of os_util_unix.go.
  3. Deleting os_util_linux.go, os_util_darwin.go, os_util_freebsd.go, and this new file (os_util_bsd.go).

If you prefer a smaller change, you could just remove os_util_freebsd.go since this file (os_util_bsd.go) already covers all BSDs, including FreeBSD.

libs/go/sia/util/os_util_bsd.go (62-76)

medium

This block of code for determining the group ID is duplicated in UidGidForUserGroup (lines 87-101).

To improve maintainability and reduce duplication, you could extract this logic into a private helper function. For example:

func resolveGid(groupname string, userGid int) int {
	// Override the group id if user explicitly specified the group.
	if groupname != "" {
		if gid := GetGroupGID(groupname); gid != -1 {
			return gid
		}
	}
	
	// if the group is not specified or invalid then we'll default
	// to our unix group name called athenz
	if gid := GetGroupGID(siaUnixGroup); gid != -1 {
		return gid
	}
	
	// otherwise use the user group id value
	return userGid
}

Then you can simplify SvcAttrs and UidGidForUserGroup by calling this helper.

libs/go/sia/util/os_util_bsd.go (152-154)

medium

The errors returned by changeDirectoryOwnership and setupDirOwnership are ignored. If changing ownership fails, the error should be handled and propagated up.

Assuming setupDirOwnership is modified to return an error as I suggested elsewhere, you should check the errors here.

	if err := changeDirectoryOwnership(siaMainDir, ownerUid, ownerGid); err != nil {
		return err
	}
	if err := setupDirOwnership(certDir, ownerUid, ownerGid); err != nil {
		return err
	}
	if err := setupDirOwnership(keyDir, ownerUid, ownerGid); err != nil {
		return err
	}

libs/go/sia/util/os_util_bsd.go (179-186)

medium

This function suppresses the error that can be returned by filepath.WalkDir. If an error occurs during the directory walk (e.g., a permissions issue), it will be silently ignored. The function should return this error so the caller can handle it.

Also, the error handling inside the closure can be simplified.

func setupDirOwnership(siaDir string, ownerUid, ownerGid int) error {
	return filepath.WalkDir(siaDir, func(path string, dirEntry fs.DirEntry, err error) error {
		if err != nil {
			return err
		}
		return changeDirectoryOwnership(path, ownerUid, ownerGid)
	})
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant