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

cachedFeatureCheck()/cachedFeatureRecord() error reporting broken #1891

Open
Luap99 opened this issue Apr 19, 2024 · 2 comments
Open

cachedFeatureCheck()/cachedFeatureRecord() error reporting broken #1891

Luap99 opened this issue Apr 19, 2024 · 2 comments

Comments

@Luap99
Copy link
Member

Luap99 commented Apr 19, 2024

There are cases why the code returns empty error string causing big confusion for users, i.e. containers/podman#22439 (comment)

Let's take data only layers as an example:

func supportsDataOnlyLayersCached(home, runhome string) (bool, error) {
feature := "dataonly-layers"
overlayCacheResult, overlayCacheText, err := cachedFeatureCheck(runhome, feature)
if err == nil {
if overlayCacheResult {
logrus.Debugf("Cached value indicated that data-only layers for overlay are supported")
return true, nil
}
logrus.Debugf("Cached value indicated that data-only layers for overlay are not supported")
return false, errors.New(overlayCacheText)
}
supportsDataOnly, err := supportsDataOnlyLayers(home)
if err2 := cachedFeatureRecord(runhome, feature, supportsDataOnly, ""); err2 != nil {
return false, fmt.Errorf("recording overlay data-only layers support status: %w", err2)
}
return supportsDataOnly, err
}

The first time around there will be no cache so it goes down to call supportsDataOnlyLayers() which will return false on WSL at least, then cachedFeatureRecord() writes an empty file to suggest it does not support this.

On the second run cachedFeatureCheck() will read the empty file and know it does not support this so it uses the empty cached text on line 2044 as error, thus returning an empty error message.

AFAICT this seems to effect more than this one caller. IMO we should never trust the file content as error message.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 19, 2024

The files were created by us… so they are “trusted” in principle; but now that we have past versions having written empty data into the files, I agree that consumers need at least a fallback error text.

@Luap99
Copy link
Member Author

Luap99 commented Apr 19, 2024

Trust is a hard word I agree. I am fine if there is a fallback message.

Btw the error message on WSL from supportsDataOnlyLayers() is not helpful either, it is just EINVAL so just invalid argument as text without any context. That is certainly not much more helpful then no message.

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

No branches or pull requests

2 participants