Various repository fixes#146
Conversation
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Looks sane to me as is, just nits
crates/composefs/src/util.rs
Outdated
| format!("{}{}", prefix, rand_string) | ||
| } | ||
|
|
||
| pub fn replace_symlinkat( |
There was a problem hiding this comment.
I think unit tests for this would be pretty easy to add too
alexlarsson
force-pushed
the
repository-fixes
branch
from
08611b7 to
362631d
Compare
| let category_fd = self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY)?; | ||
| let category_fd = match filter_errno( | ||
| self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY), | ||
| Errno::NOENT, |
There was a problem hiding this comment.
BTW I maintain https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.open_dir_optional
We had some previous debates about using cap-std here. I (obviously) like it a lot.
filter_errno makes sense as is but it'd probably be good to have a higher level openat_optional wrapper per above.
allisonkarlitskaya
left a comment
allisonkarlitskaya
left a comment
There was a problem hiding this comment.
Just minor changes from me too
crates/composefs/src/repository.rs
Outdated
| Errno::NOENT, | ||
| ) | ||
| .context("Opening {category} dir in repository")? | ||
| { |
There was a problem hiding this comment.
This is begging for a
let Some(category_fd) = ...? else {
return;
}| } | ||
| } | ||
|
|
||
| Err(Errno::EXIST) |
There was a problem hiding this comment.
That could probably use some context...
allisonkarlitskaya
force-pushed
the
repository-fixes
branch
from
362631d to
0f8d2eb
Compare
|
What happened with the commit description? |
|
Uh. There's very clearly some rebase fail here, sorry :( |
allisonkarlitskaya
force-pushed
the
repository-fixes
branch
from
0f8d2eb to
fff41b1
Compare
|
This looks good to me, but I added a commit that imho clean up filter_errno(). |
crates/composefs/src/repository.rs
Outdated
| .context("Opening {category} dir in repository")? | ||
| let Some(category_fd) = self | ||
| .openat(category, OFlags::RDONLY | OFlags::DIRECTORY) | ||
| .filter_errno(Errno::NOENT) |
There was a problem hiding this comment.
This is deeply cool. Way nicer to use.
Missing DCO, though .... :/
alexlarsson
force-pushed
the
repository-fixes
branch
from
adf4a54 to
454449c
Compare
For example, if there has been no streams created, we don't want to fail with some random ENOENT. Signed-off-by: Alexander Larsson <alexl@redhat.com>
Naming a new version of an image the same as the old version is pretty standard, and this is currently giving EEXIST errors. We fix this by doing an atomic symlink + rename operation to replace the symlink if it doesn't already have the expected value. Drop our existing Repository::ensure_symlink() function and use the new function for that as well. Co-authored-by: Alexander Larsson <alexl@redhat.com> Signed-off-by: Alexander Larsson <alexl@redhat.com> Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
allisonkarlitskaya
force-pushed
the
repository-fixes
branch
from
454449c to
7b6075e
Compare
|
(force-pushed to fix the conflict) |
This adds error context when opening images and streams. Without this it is kinda hard to figure out what ENOENT really means. Signed-off-by: Alexander Larsson <alexl@redhat.com>
allisonkarlitskaya
force-pushed
the
repository-fixes
branch
from
7b6075e to
968b8b3
Compare
|
@allisonkarlitskaya Pushing this seems to have lost the nicer filter_errno() commit? |
|
I.e. 454449c |
|
The Ubuntu fail is kernel mount regression noise. |
And I missed this, of course. Sorry :( |
|
→ #151 |
3 participants
These were broken out from: #144