-
Notifications
You must be signed in to change notification settings - Fork 239
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
Make --symlink idempotent #577
Conversation
When creating symlinks specified with --symlink, check errno if creation fails. If the reason was EEXIST, check the path of the existing symlink. If it matches what was desired, continue as normal. Fixes containers#577 Signed-off-by: Jakob Kaivo <jkk@ung.org>
Fixes #549 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this! I don't know whether it's OK for this to be unconditional, or whether that would be a breaking change; that can be a separate thread. For now I'm reviewing the implementation as if it's OK to do this simple version.
@alexlarsson, @cgwalters: Do you think it's OK to make If you think it needs to be explicit, there are a couple of possible spellings. The simplest but least general would be But I think it might be better to do something more like the recently-added
|
If we had a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very nice enhancement to flatpak, please merge.
Maybe after fixing the whitespace problem on line 1493 (two extra blanks before break)
I was really hoping for a review of this from @cgwalters or @alexlarsson, who have a better overview than I do of all the places where bubblewrap is used and the ways in which it needs to stay backwards-compatible.
Did I miss one of the maintainers of bubblewrap adding you to the project? Or if you are not maintainers, does this mean we can count on receiving contributions from you if this change is merged but turns out to cause regressions? |
bubblewrap.c
Outdated
char existing[PATH_MAX + 1] = ""; | ||
ssize_t elen = readlink (dest, existing, sizeof (existing) - 1); | ||
if (elen < 0) | ||
{ | ||
if (errno == EINVAL) | ||
die ("Can't make symlink at %s: destination exists and is not a symlink", op->dest); | ||
else | ||
die_with_error ("Can't make symlink at %s: destination exists, and cannot read symlink target", op->dest); | ||
} | ||
|
||
existing[elen] = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use readlink_malloc()
(into a new cleanup_free char *existing = NULL
) in preference to a statically-sized buffer. This returns a non-null string on success, or NULL
with errno
set on failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @jkaivo will you be able to do this chages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just seeing this. I don't see what actual advantage readlink_malloc()
has over POSIX readlink()
with an appropriately sized buffer. Since submitting this patch, I have solved my personal use-case issues by only using Steam on my Steam Deck, so I have very little motivation to make additional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see what actual advantage readlink_malloc() has over POSIX readlink() with an appropriately sized buffer
If it was possible to choose an appropriately sized buffer, then there would be no advantage. The problem with POSIX readlink() is that there is no correct size for the buffer, because Linux PATH_MAX
is a lie (it is entirely valid to have paths longer than PATH_MAX
).
Not a maintainer, but since this patch actually made flatpak useful for me (I add symlinks below /run/user), I will of course help to the best of my abilities. |
@smcv If you need an LGTM from @alexlarsson or @cgwalters before you'll feel comfortable accepting this pull request, what do we have to do to get their attention? The pull request has been waiting for them since April and I'm sure @jkaivo would be more motivated to implement your comment if we had some indication that the patch might actually go in once the fix is made. If the concern is backwards-incompatibility with current users... I for one can testify that certain users of bwrap are already assuming the functionality is idempotent, and choking when it isn't. That's the reason I found this thread in the first place: a flatpak app I use fails to start if one of the many hidden directories in my homedir has a symlink to somewhere. I'm working around it with Thanks for your help. |
Personally I think it is pretty safe to just add this as-is. I'm not aware of anyone using bwrap --symlink and then specifically looking for some kind of EEXIST error and handling that in a custom way. It would be really hard to extract that particular error anyway as other things could error out. So, LGTM, but I would prefer if all the changes were squashed into one commit. |
It sounds as though the submitter of this PR is no longer interested in contributing it, so I'll try to find time among my other responsibilities to rebase this and fix my review comment myself. |
Something along these lines?:
|
Yes, a lot like that (but with some coding style fixes). Thanks! Please send pull requests for any future changes that you want to propose: the limiting factor on this project is the maintainers' limited time, and that's more efficiently used if we aren't passing diff output around like it's the 1990s.
coding style: space before
coding style: this project uses GNU-style indentation
The trailing |
When creating symlinks specified with --symlink, check errno if creation fails. If the reason was EEXIST, check the path of the existing symlink. If it matches what was desired, continue as normal. Resolves: containers#549 Signed-off-by: Jakob Kaivo <jkk@ung.org> [Anders Blomdell: use readlink_malloc()] [smcv: Squash multiple changes into one; coding style fixes] Co-authored-by: Anders Blomdell Co-authored-by: Simon McVittie <smcv@collabora.com> Signed-off-by: Simon McVittie <smcv@collabora.com>
I'll merge this after CI succeeds, unless another maintainer objects. |
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Add documentation and test coverage for #577
When creating symlinks specified with --symlink, check errno if creation fails. If the reason was EEXIST, check the path of the existing symlink. If it matches what was desired, continue as normal.