search

containers / storage

Container Storage Library
Apache License 2.0
539 stars 234 forks source link

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

Open Luap99 opened 2 months ago

Luap99 commented 2 months ago

There are cases why the code returns empty error string causing big confusion for users, i.e. https://github.com/containers/podman/issues/22439#issuecomment-2066397554

Let's take data only layers as an example: https://github.com/containers/storage/blob/c051e2a0df0d5b7883a165661e68fd9006713c06/drivers/overlay/overlay.go#L2035-L2051

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 commented 2 months ago

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 commented 2 months ago

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.