image: Skip creating lock file if short-name-aliases.conf is missing

[Lekensteyn]

The lock file ~/.cache/containers/short-name-aliases.conf.lock used to be created in all cases, even for read-only attempts for a non-existing ~/.cache/containers/short-name-aliases.conf file. Do not create this file unless necessary, this keeps the home directory a bit cleaner.

Link: podman-container-tools/podman#27921

[mtrmac]

Thanks!

I’m pretty unconvinced this makes much of a difference ultimately — Podman must have writable storage for mount counting and locks, after all — but if it can be done cleanly, why not.

[Lekensteyn]

I’m pretty unconvinced this makes much of a difference ultimately — Podman must have writable storage for mount counting and locks, after all — but if it can be done cleanly, why not.

Thanks for the review! I should've addressed all comments.

Yes, writeable storage is required, here I just try to remove unnecessary writes and new directories that would have been empty otherwise.

[mtrmac]

This comment adds nothing, please drop.

---

… It’s actually actively misleading in case of a non-ErrNotExist error.

Overall, the code should either only activate on fileutils.Exists() == nil, or it should properly handle the unexpected error cases by reporting them. I think the latter is better, to help the user debug such situations.

[Lekensteyn]

It's intentionally only skipping the branch when the file is definitely missing. For permissions errors and so on, I am purposely ignoring the error and entering the branch. If checking for file existence fails, then creating the lock file will likely produce a better error message.

I can look into bailing out earlier if checking for file existence fails.

[mtrmac]

Hum… it’s not obvious to me that if fileutils.Exists fails with an I/O error or something, then accessing/creating a file within will certainly fail, but it’s close enough not to be worth worrying about. So not adding the extra error checking is fair enough.

[Lekensteyn]

I can go either way, right now I have this in the latest pushed version:

	err = fileutils.Exists(confPath)
	if err != nil && !errors.Is(err, fs.ErrNotExist) {
		return nil, "", err
	}
	if err == nil {
		lock, err := getShortNameAliasesLock(confPath)

Whereas the err == nil check actually implies the condition !errors.Is(err, fs.ErrNotExist), but I didn't want to call errors.Is twice.

Your call?