Improve file mode API #170
Conversation
|
@syokensyo Here's my unfinished first draft of the API redesign I mentioned. Do you have any objections to the overall direction of this? |
dralley
force-pushed
the
filemode-api
branch
4 times, most recently
from
26f8f7d to
ea0d1f2
Compare
| .with_file( | ||
| "./test_assets/empty_file_for_symlink_create", | ||
| rpm::FileOptions::symlink("/usr/bin/awesome_link", "/usr/bin/awesome") | ||
| .permissions(0o644) | ||
| .pre_install_script("echo preinst") |
There was a problem hiding this comment.
@drahnr Could you do a quick skim of these changes and let me know what you think? It still needs polish so don't spend too much time on it, just let me know if I'm doing something stupid.
dralley
force-pushed
the
filemode-api
branch
2 times, most recently
from
f9f0ca9 to
52d1055
Compare
| SymbolicLink { permissions: u16 }, | ||
| // For "Invalid" we use a larger integer since it is possible to create an invalid | ||
| // FileMode by providing an overflowing integer. | ||
| Invalid { raw_mode: i32, reason: &'static str }, |
There was a problem hiding this comment.
I don't think this should be one of our "valid" states, we can just return an error. If we want to parse invalid RPMs then we can compute the mode lazily.
dralley
force-pushed
the
filemode-api
branch
2 times, most recently
from
1d463a0 to
fee222f
Compare
| // see: constants::FileFlags | ||
| // @todo: should we represent "defattr", that is, set default permissions on all files in a package | ||
| // without needing to explicitly them for each FileOptions | ||
| // @todo: how about "%docdir"? which automatically marks subsequent files in those directories as docs |
| // @todo: is this problematic? `with_file` has a source argument, but I do not believe %dir uses | ||
| // any "source" necessarily, just a destination. Maybe we need `PackageBuilder::with_dir()`, | ||
| // except that sounds like it would process files recursively inside a directory, and we would | ||
| // still need to duplicate much of this code. |
There was a problem hiding this comment.
On second thought, in case we don't "inherit permissions" and there is no default, it would probably need to stat the directory to get those permissions on its own?
| @@ -201,9 +173,37 @@ pub struct FileOptions { | |||
| pub(crate) caps: Option<FileCaps>, | |||
| } | |||
|
|
|||
| // @todo: should we even have "default permissions" / mode, or use Option<FileMode>? | |||
| // if they can be skipped (unsure), 'inherit_permissions' could go away | |||
| } | ||
| } | ||
| } | ||
|
|
||
| // @todo: 0o7777? should it be 0o777? |
There was a problem hiding this comment.
Apparently this is legit
Let's fix up cdplayer with a liberal sprinkling of %attrs. Here's what the %files list looks like after we've had our way with it:
%files
%attr(-, root, root) %doc README
%attr(4755, root, root) /usr/local/bin/cdp
%attr(-, root, root) /usr/local/bin/cdplay
%attr(-, root, rot) /usr/local/man/man1/cdp.1A couple points are worth noting here. The line for README shows that multiple macros can be used on a line — in this case, one to set file attributes, and one to mark the file as being documentation. The %attr for /usr/local/bin/cdp declares the file to be setuid root. If it sends a shiver down your spine to know that anybody can create a package that will run setuid root when installed on your system — Good! Just because RPM makes it easy to install software doesn't mean that you should blindly install every package you find.
A single RPM command can quickly point out areas of potential problems and should be issued on any package file whose creators you don't trust:
% rpm -qlvp ../RPMS/i386/cdplayer-1.0-1.i386.rpm
drwxr-xr-x- root root 1024 Sep 13 20:16 /usr/doc/cdplayer-1.0-1
-rw-r--r--- root root 1085 Nov 10 01:10 /usr/doc/cdplayer-1.0-1/README
-rwsr-xr-x- root root 40739 Sep 13 21:32 /usr/local/bin/cdp
lrwxrwxrwx- root root 47 Sep 13 21:32 /usr/local/bin/cdplay -> ./cdp
-rwxr-xr-x- root rot 4550 Sep 13 21:32 /usr/local/man/man1/cdp.1
%
This almost makes me want to push the setuid/setgid bits into their own methods, but that would be a real pain for anyone trying to implement something that builds from spec files.
| #[allow(clippy::new_ret_no_self)] | ||
| pub fn new(dest: impl Into<String>) -> FileOptionsBuilder { |
There was a problem hiding this comment.
The basic principle behind the change is that symlinks need to include the destination where they link to, and the initial PR had the user provide that information with a .symlink(), which created ambiguity.
However, on second thought, maybe instead of having the user specify this, we should assume the "source" file always exists (and in practice we probably do anyway) and use std::fs::read_link to get that info? In practice if we provide a "source" shouldn't we be stating the file to make sure it is what the user says it is anyway?
TODO: check if rpm always stats the files and reads their properties or if it sometimes can just declare them without needing them to exist in the buildroot.
There was a problem hiding this comment.
@syokensyo ^ wdyt, since this impacts how we handle symlinks
There was a problem hiding this comment.
I think it is better to make sure the link exists in the build root, and use std::fs::read_link read the link's value as the target, however it is also better to provide a custom link target api with high priority over link's value.
There was a problem hiding this comment.
Leaving notes for myself
- yes
rpmalways tries to look for the files and getstatinfo, but in the case of%ghostfiles they aren't required to exist and they literally create fake stat data - they use
lstatwhich I believe is equivalent to https://doc.rust-lang.org/std/fs/fn.symlink_metadata.html - I think
%attrcan't be applied to symlinks?
see rpm/build/files.c
Does it really make sense to provide a link override though? If all the other files have to exist, why make a totally separate codepath?
There was a problem hiding this comment.
"provide a link override" is flow original behavior... and yes, you are right, there is no scenario for that kind of behavior if other files exists.
Avoid users needing to deal with raw u16 numbers and return errors on invalid values rather than making "invalid" part of the enum.
dralley
force-pushed
the
filemode-api
branch
from
fee222f to
bfbc0d8
Compare
📜 Checklist
--all-featuresenabled