Add support for the matchall filter in TrafficFilterNewRequest

[sophacles]

…uest

[cathay4t]

• Do not use assert.
• The commit message should be polished also.
• Add test case or example code for this new function.

[sophacles]

Hi @cathay4t , I have cleaned up the commit message and added an example (this example depends on these 2 PRs: https://github.com/rust-netlink/netlink-packet-route/pull/20/files and rust-netlink/netlink-packet-route#22 ).

Regarding the assert: I added the assert in to match the other code in the same struct. I agree that having the assert in library code should be avoided, however it seems odd to have one builder method return a result while the rest don't. This pattern is found also in the add_qdisc.rs file (QDiscNewRequest struct) and get.rs file (QDiscGetRequest struct). All these asserts in the traffic control request types validate that the message matches what the kernel expects. Would it make more sense to do a separate PR that changes these builder methods to return a Result all at once?

Alternately, I wonder if splitting the QDiscNewRequest QDiscGetRequest and TrafficFilterNewRequest into builder types and message types would be a good way to go. When splitting them, all the validation logic can happen at the time of a call to a build(self) -> Result<MessageType> method. Such a pattern would allow the builder.parameter1(x).parameter2(y) pattern that is common in rust, while removing the asserts.

Let me know what you think about this!

[sophacles]

Hi @cathay4t - any thoughts on the above comment?

[cathay4t]

For assert, I have created #16 to track the effort of removing it.

I got compile failure when invoking cargo build --example subnet_nat, please fix.

[sophacles]

When I build this PR I see the errors:

error[E0433]: failed to resolve: could not find `matchall` in `tc`
   --> src/traffic_control/add_filter.rs:121:45
    |
121 |     pub fn matchall(mut self, data: Vec<tc::matchall::Nla>) -> Self {
    |                                             ^^^^^^^^ could not find `matchall` in `tc`

error[E0433]: failed to resolve: could not find `matchall` in `tc`
   --> src/traffic_control/add_filter.rs:129:37
    |
129 |             .push(tc::Nla::Kind(tc::matchall::KIND.to_string()));
    |                                     ^^^^^^^^ could not find `matchall` in `tc`

error[E0599]: no variant or associated item named `Matchall` found for enum `TcOpt` in the current scope
   --> src/traffic_control/add_filter.rs:131:45
    |
131 |             data.into_iter().map(tc::TcOpt::Matchall).collect(),
    |                                             ^^^^^^^^ variant or associated item not found in `TcOpt`

Some errors have detailed explanations: E0433, E0599.
For more information about an error, try `rustc --explain E0433`.
error: could not compile `rtnetlink` due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

Is this the errors you see?

If so, it's because Cargo.toml still has this line:

netlink-packet-route = { version = "0.15" }

But the matchall and tc nat patches aren't in version 0.15, they are on the main branch of the repo.
Changing that line in Cargo.toml to be something like:

netlink-packet-route = { path = "../netlink-packet-route", version = "0.15" }

Fixes this on my computer (since all the rust-netlink repos are checked out into the same parent directory on my machine).

I'm not sure of the proper way to handle this situation.

If you are getting a different error, would you mind pasting it here, since it builds and runs for me?

[cathay4t]

@sophacles Let me tag new release in netlink-packet-route after some quick test, ETA 2023 April 24

[cathay4t]

@sophacles I have published netlink-packet-route 0.16.0. Please use it and update this PR. Sorry for the long wait.

[cathay4t]

Meanwhile, please remove Re #12 - from commit message and PR title.

[sophacles]

Hi @cathay4t I have updated the PR by rebasing on main, updating the dependency, and fixing up the name changes.

Note for the functions that have slave in the name I have:

1. added a new method that is identical but uses the port terms instead
2. deprecated the method using the slave terms.

If you would prefer a something else, please let me know and I'll update.

[cathay4t]

@sophacles The patch looks good to me. I have already updated the dependencies. Please do the rebase again. Thank you!

[sophacles]

@cathay4t I've redone the rebase and pushed to my branch.

Note: since the "remove asserts" pr was merged, the code has changed slightly in the matchall method to return a result rather than do an assert in the same way as the u32 method. I've also updated the example to reflect this.

[sophacles]

@cathay4t do you need me to do anything else to this PR since my last update?

Need to use latest API of netlink-packet-route

[cathay4t]

@sophacles Sorry for the wait. Please rebase to use latest netlink-packet-route API.

[cathay4t]

@sophacles ping again.