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.

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

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

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 4, 2024

Currently we handle XDG_CONFIG_HOME and the fallback $HOME/.config differently. We create the later if it does not exist and verify that it is owned by the current user. This patch makes XDG_CONFIG_HOME follow the same pattern.

Copy link
Contributor

openshift-ci bot commented Sep 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Luap99
Copy link
Member

Luap99 commented Sep 4, 2024

Can we first get an agreement what this should do? I still like to push hard that this function should not try to create and state the directory at all.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 4, 2024

I have no problem with that other then it is potentially a breaking change.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

This is presuming a consensus on something like containers/podman#23818 (comment) ; I’m not sure we have that yet.

// ConfigHome returns $HOME/.config and nil error if XDG_CONFIG_HOME is not set.
// Verifies ownership of config directory matches the current process or
// returns error.
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

// Verifies ownership of config directory matches the current process or
// returns error.
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
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?

// returns error.
// See also https://standards.freedesktop.org/basedir-spec/latest/ar01s03.html
func GetConfigHome() (string, error) {
rootlessConfigHomeDirOnce.Do(func() {
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 is a new function with a new semantics, the …{DirOnce,Dir,DirError} variables must not be shared with the other function.


(sync.OnceValues now exists. Aesthetically I don’t think it’s all that great, but it does have one advantage of allowing the nested function to use an ordinary return value, err control flow.)

pkg/homedir/homedir_unix.go Outdated Show resolved Hide resolved
pkg/homedir/homedir_unix.go Outdated Show resolved Hide resolved
return rootlessConfigHomeDir, rootlessConfigHomeDirError
}

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

@mtrmac mtrmac Sep 4, 2024

Choose a reason for hiding this comment

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

If we go with the plan of gradual migration per containers/podman#23818 (comment) , please

  • point at the alternative
  • explain why it this one is deprecated / what needs to change in callers
  • consider using the standard Deprecated: syntax that linters understand. OTOH that would force us to migrate immediately, I’m not 100% sure we want to sign up for doing that

Currently we handle XDG_CONFIG_HOME and the fallback $HOME/.config
differently. We create the later if it does not exist and verify that
it is owned by the current user. This patch makes XDG_CONFIG_HOME
follow the same pattern.

Also Deprecate all GetDATA() functions and just use DATA().

Signed-off-by: Daniel J Walsh <[email protected]>
func GetConfigHome() (string, error) {
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.

@@ -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.

}
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants