Add and return PermissionDenied error when not root #5
Conversation
|
Thank you for the contribution. |
jpittis
force-pushed
the
jpittis/permission-denied
branch
from
038ee7a to
ffcd436
Compare
|
I can see why you'd rather the caller interpret the exit status. ffcd436 adds a This would be very much so a breaking change. What do you think of this approach? |
|
I'm starting to see that this is a more complex change than I originally expected. For example, on my system when I attempt to check the existence of a rule that does not exist for a chain that does not eixst: $ sudo iptables -C INPUT -j MY_INPUT; echo $?
iptables v1.8.2 (legacy): Couldn't load target `MY_INPUT':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
2But if we try to check for the rule that does not exist for a chain that does exist: $ sudo iptables -C INPUT -j MY_INPUT; echo $?
iptables: No chain/target/match by that name.
1I'm going to keep on trying to understand this behavior better before continuing with this PR. |
Before when the exit status was reported as a bool, callers were unable to distinguish between permission denied failures and whether or not a given rule exists. Now we use an error to represent a non-zero exit status and a bool to represent business logic like existence.
jpittis
force-pushed
the
jpittis/permission-denied
branch
from
ffcd436 to
bfc634d
Compare
src/lib.rs
Outdated
| match self.run(&["-t", table, "-F"]) { | ||
| Ok(output) => Ok(output.status.success()), | ||
| Ok(_) => Ok(()), |
There was a problem hiding this comment.
self.run(&["-t", table, "-F"]).and_then(|_| Ok(()))| @@ -417,6 +420,10 @@ impl IPTables { | |||
| }; | |||
| } | |||
|
|
|||
| Ok(output) | |||
| match output.status.code() { | |||
There was a problem hiding this comment.
output.status.code().and_then(|i| if i == 0 { Ok(output) } else { Err(IPTError::BadExitStatus(i)) })There was a problem hiding this comment.
AFAIK this is invalid: and_then return type Option.
|
Is this still being worked on ? I was also surprised to see my program "work" without any errors when ran as a regular user. |
|
I've abandoned this so it's not currently being worked on. Might have time to work on it this weekend. |
| Ok(output) => Ok(output.status.success()), | ||
| Err(err) => Err(err), | ||
| } | ||
| pub fn insert(&self, table: &str, chain: &str, rule: &str, position: i32) -> IPTResult<()> { |
There was a problem hiding this comment.
Here's an example where the result type should maybe be IPResult<bool> instead. Thoughts?
As discussed in #4, the current implementation does not return an error when the process does not have permission to use iptables.
Fortunately, iptables returns a special status code for permission errors:
This patch does three things:
PermissionDeniederror.PermissionDeniederror.rustfmt.I can see why the maintainers might not like 3). The reformat diff is pretty small so I didn't revert it. If you'd like me to revert it and or patch it in a separate commit, please ask.
Finally, I didn't add any tests to this patch. Likely the code that tests this will need to be run without
sudoto trigger thePermissionDeniedpath. I'm not sure of the nicest way to do this so I was going to wait for the maintainers to discuss.Thanks!