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

Jump to bottom

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Treat XDG_CONFIG_HOME and $HOME/.config the same way #2084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 26 additions & 6 deletions pkg/homedir/homedir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@ import (
"path/filepath"
)

// GetDataHome returns XDG_DATA_HOME.
// GetDataHome returns $HOME/.local/share and nil error if XDG_DATA_HOME is not set.
// DataHome (deprecated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For all these name changes…

  • I don’t really see enough benefit but also I don’t really care
  • I do think the code should either:
    • Introduce a new name for symmetry, and intend to keep the old names forever. Then it’s not really deprecated.
    • Decide to deprecate the old name; in that case this should use the standard Deprecated: syntax so that tool make us migrate immediately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

… all these name changes apart from the real semantic change WRT ConfigHome.

Copy link
Member

Choose a reason for hiding this comment

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

I rather not do the unrelated name changes, it just adds churn for no reason. If I look at a code and see the two different functions used I will always have to follow them to figure out if they are actual equal.

func GetDataHome() (string, error) {
return DataHome()
}

// DataHome returns XDG_DATA_HOME.
// DataHome returns $HOME/.local/share and nil error if XDG_DATA_HOME is not set.
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func GetDataHome() (string, error) {
func DataHome() (string, error) {
if xdgDataHome := os.Getenv("XDG_DATA_HOME"); xdgDataHome != "" {
return xdgDataHome, nil
}
Expand All @@ -21,11 +26,16 @@ func GetDataHome() (string, error) {
return filepath.Join(home, ".local", "share"), nil
}

// GetCacheHome returns XDG_CACHE_HOME.
// GetCacheHome returns $HOME/.cache and nil error if XDG_CACHE_HOME is not set.
// GetCacheHome (deprecated)
func GetCacheHome() (string, error) {
return CacheHome()
}

// CacheHome returns XDG_CACHE_HOME.
// CacheHome returns $HOME/.cache and nil error if XDG_CACHE_HOME is not set.
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func GetCacheHome() (string, error) {
func CacheHome() (string, error) {
if xdgCacheHome := os.Getenv("XDG_CACHE_HOME"); xdgCacheHome != "" {
return xdgCacheHome, nil
}
Expand All @@ -35,3 +45,13 @@ func GetCacheHome() (string, error) {
}
return filepath.Join(home, ".cache"), nil
}

// GetRuntimeDir (deprecated)
func GetRuntimeDir() (string, error) {
return RuntimeDir()
}

// GetShortcutString (deprecated)
func GetShortcutString() string {
return ShortcutString()
}
50 changes: 28 additions & 22 deletions pkg/homedir/homedir_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ func Get() string {
return homedir
}

// GetShortcutString returns the string that is shortcut to user's home directory
// ShortcutString returns the string that is shortcut to user's home directory
// in the native shell of the platform running on.
func GetShortcutString() string {
func ShortcutString() string {
return "~"
}

Expand All @@ -52,7 +52,7 @@ func GetShortcutString() string {
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func StickRuntimeDirContents(files []string) ([]string, error) {
runtimeDir, err := GetRuntimeDir()
runtimeDir, err := RuntimeDir()
if err != nil {
// ignore error if runtimeDir is empty
return nil, nil //nolint: nilerr
Expand Down Expand Up @@ -101,11 +101,20 @@ func isWriteableOnlyByOwner(perm os.FileMode) bool {
return (perm & 0o722) == 0o700
}

// GetConfigHome returns XDG_CONFIG_HOME.
// GetConfigHome returns $HOME/.config and nil error if XDG_CONFIG_HOME is not set.
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
// GetConfigHome deprecated
func GetConfigHome() (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not compile; was this intended to be ConfigHome?

rootlessConfigHomeDir, rootlessConfigHomeDirError = ConfigHome()
if rootlessConfigHomeDirError == nil {
_ = os.MkdirAll(rootlessConfigHomeDir, 0o700)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this function is deprecated and we intend to migrate all users, I don’t see that changing the existing one to not report failures is useful: that forces us to immediately review/migrate all users, negating the benefit of the deprecation and introduction of a new name for the new version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, right now, if the directory is missing, ConfigHome returns ("", ENOENT), so AFAICS this condition does nothing.

Shouldn’t all these functions, with their rare corner cases we don’t encounter on our workstations, have pretty good test coverage? I still mourn for tests lost in #1740 .

}

return rootlessConfigHomeDir, rootlessConfigHomeDirError
}

// ConfigHome returns XDG_CONFIG_HOME. (Deprecated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably this one is not deprecated.

// ConfigHome returns $HOME/.config and nil error if XDG_CONFIG_HOME is not set.
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func ConfigHome() (string, error) {
rootlessConfigHomeDirOnce.Do(func() {
cfgHomeDir := os.Getenv("XDG_CONFIG_HOME")
if cfgHomeDir == "" {
Expand All @@ -115,33 +124,30 @@ func GetConfigHome() (string, error) {
rootlessConfigHomeDirError = fmt.Errorf("cannot resolve %s: %w", home, err)
return
}
tmpDir := filepath.Join(resolvedHome, ".config")
_ = os.MkdirAll(tmpDir, 0o700)
st, err := os.Stat(tmpDir)
if err != nil {
rootlessConfigHomeDirError = err
return
} else if int(st.Sys().(*syscall.Stat_t).Uid) == os.Geteuid() {
cfgHomeDir = tmpDir
} else {
rootlessConfigHomeDirError = fmt.Errorf("path %q exists and it is not owned by the current user", tmpDir)
return
}
cfgHomeDir = filepath.Join(resolvedHome, ".config")
}
st, err := os.Stat(cfgHomeDir)
Copy link
Member

Choose a reason for hiding this comment

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

I am still not convinced that the stat adds value here.

In fact if we keep the the stat then I see zero reason to change anything here because then most callers still have to ignore ENOENT errors which adds a lot of code and doesn't help the issue containers/podman#23818.

Neither HomeDir() or GetCacheHome() validate permissions either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that propagating ENOENT to callers is probably undesirable.

To be specific, we need some plan for the podman machine code that writes to the config directory. If the config directory doesn’t exist, how does it get created? The current version of ConfigHome seems not to allow that to happen at all.

Neither HomeDir() or GetCacheHome() validate permissions either.

We’ve had past painful experience with users who have XDG_*DIR set in the environment using su (without --login) and clobbering other users’ accounts (and if root is overwriting other users’ data, that other user might not be able to recover without help).

I think if we ignore or special-case ENOENT, the permission checks have no downsides, and should be preserved.

if err != nil {
rootlessConfigHomeDirError = err
return
} else if int(st.Sys().(*syscall.Stat_t).Uid) != os.Geteuid() {
rootlessConfigHomeDirError = fmt.Errorf("path %q exists and it is not owned by the current user", tmpDir)
return
}
rootlessConfigHomeDir = cfgHomeDir
})

return rootlessConfigHomeDir, rootlessConfigHomeDirError
}

// GetRuntimeDir returns a directory suitable to store runtime files.
// RuntimeDir returns a directory suitable to store runtime files.
// The function will try to use the XDG_RUNTIME_DIR env variable if it is set.
// XDG_RUNTIME_DIR is typically configured via pam_systemd.
// If XDG_RUNTIME_DIR is not set, GetRuntimeDir will try to find a suitable
// If XDG_RUNTIME_DIR is not set, RuntimeDir will try to find a suitable
// directory for the current user.
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func GetRuntimeDir() (string, error) {
func RuntimeDir() (string, error) {
var rootlessRuntimeDirError error

rootlessRuntimeDirOnce.Do(func() {
Expand Down
21 changes: 13 additions & 8 deletions pkg/homedir/homedir_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,21 @@ func Get() string {
return home
}

// GetConfigHome returns the home directory of the current user with the help of
// GetConfigHome (deprecated)
func GetConfigHome() (string, error) {
return ConfigHome()
}

// ConfigHome returns the home directory of the current user with the help of
// environment variables depending on the target operating system.
// Returned path should be used with "path/filepath" to form new paths.
func GetConfigHome() (string, error) {
func ConfigHome() (string, error) {
return filepath.Join(Get(), ".config"), nil
}

// GetShortcutString returns the string that is shortcut to user's home directory
// ShortcutString returns the string that is shortcut to user's home directory
// in the native shell of the platform running on.
func GetShortcutString() string {
func ShortcutString() string {
return "%USERPROFILE%" // be careful while using in format functions
}

Expand All @@ -44,15 +49,15 @@ func StickRuntimeDirContents(files []string) ([]string, error) {
return nil, nil
}

// GetRuntimeDir returns a directory suitable to store runtime files.
// RuntimeDir returns a directory suitable to store runtime files.
// The function will try to use the XDG_RUNTIME_DIR env variable if it is set.
// XDG_RUNTIME_DIR is typically configured via pam_systemd.
// If XDG_RUNTIME_DIR is not set, GetRuntimeDir will try to find a suitable
// If XDG_RUNTIME_DIR is not set, RuntimeDir will try to find a suitable
// directory for the current user.
//
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func GetRuntimeDir() (string, error) {
data, err := GetDataHome()
func RuntimeDir() (string, error) {
data, err := DataHome()
if err != nil {
return "", err
}
Expand Down