{"action":"created","issue":{"url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/260","repository_url":"https://api.github.com/repos/mullvad/mullvadvpn-app","labels_url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/260/labels{/name}","comments_url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/260/comments","events_url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/260/events","html_url":"https://github.com/mullvad/mullvadvpn-app/pull/260","id":336812432,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk4MTc4NjIx","number":260,"title":"Linux firewall integration","user":{"login":"faern","id":332294,"node_id":"MDQ6VXNlcjMzMjI5NA==","avatar_url":"https://avatars1.githubusercontent.com/u/332294?v=4","gravatar_id":"","url":"https://api.github.com/users/faern","html_url":"https://github.com/faern","followers_url":"https://api.github.com/users/faern/followers","following_url":"https://api.github.com/users/faern/following{/other_user}","gists_url":"https://api.github.com/users/faern/gists{/gist_id}","starred_url":"https://api.github.com/users/faern/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/faern/subscriptions","organizations_url":"https://api.github.com/users/faern/orgs","repos_url":"https://api.github.com/users/faern/repos","events_url":"https://api.github.com/users/faern/events{/privacy}","received_events_url":"https://api.github.com/users/faern/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"pronebird","id":704044,"node_id":"MDQ6VXNlcjcwNDA0NA==","avatar_url":"https://avatars2.githubusercontent.com/u/704044?v=4","gravatar_id":"","url":"https://api.github.com/users/pronebird","html_url":"https://github.com/pronebird","followers_url":"https://api.github.com/users/pronebird/followers","following_url":"https://api.github.com/users/pronebird/following{/other_user}","gists_url":"https://api.github.com/users/pronebird/gists{/gist_id}","starred_url":"https://api.github.com/users/pronebird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pronebird/subscriptions","organizations_url":"https://api.github.com/users/pronebird/orgs","repos_url":"https://api.github.com/users/pronebird/repos","events_url":"https://api.github.com/users/pronebird/events{/privacy}","received_events_url":"https://api.github.com/users/pronebird/received_events","type":"User","site_admin":false},"assignees":[{"login":"pronebird","id":704044,"node_id":"MDQ6VXNlcjcwNDA0NA==","avatar_url":"https://avatars2.githubusercontent.com/u/704044?v=4","gravatar_id":"","url":"https://api.github.com/users/pronebird","html_url":"https://github.com/pronebird","followers_url":"https://api.github.com/users/pronebird/followers","following_url":"https://api.github.com/users/pronebird/following{/other_user}","gists_url":"https://api.github.com/users/pronebird/gists{/gist_id}","starred_url":"https://api.github.com/users/pronebird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pronebird/subscriptions","organizations_url":"https://api.github.com/users/pronebird/orgs","repos_url":"https://api.github.com/users/pronebird/repos","events_url":"https://api.github.com/users/pronebird/events{/privacy}","received_events_url":"https://api.github.com/users/pronebird/received_events","type":"User","site_admin":false},{"login":"jvff","id":3216731,"node_id":"MDQ6VXNlcjMyMTY3MzE=","avatar_url":"https://avatars3.githubusercontent.com/u/3216731?v=4","gravatar_id":"","url":"https://api.github.com/users/jvff","html_url":"https://github.com/jvff","followers_url":"https://api.github.com/users/jvff/followers","following_url":"https://api.github.com/users/jvff/following{/other_user}","gists_url":"https://api.github.com/users/jvff/gists{/gist_id}","starred_url":"https://api.github.com/users/jvff/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jvff/subscriptions","organizations_url":"https://api.github.com/users/jvff/orgs","repos_url":"https://api.github.com/users/jvff/repos","events_url":"https://api.github.com/users/jvff/events{/privacy}","received_events_url":"https://api.github.com/users/jvff/received_events","type":"User","site_admin":false},{"login":"pinkisemils","id":7757572,"node_id":"MDQ6VXNlcjc3NTc1NzI=","avatar_url":"https://avatars0.githubusercontent.com/u/7757572?v=4","gravatar_id":"","url":"https://api.github.com/users/pinkisemils","html_url":"https://github.com/pinkisemils","followers_url":"https://api.github.com/users/pinkisemils/followers","following_url":"https://api.github.com/users/pinkisemils/following{/other_user}","gists_url":"https://api.github.com/users/pinkisemils/gists{/gist_id}","starred_url":"https://api.github.com/users/pinkisemils/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pinkisemils/subscriptions","organizations_url":"https://api.github.com/users/pinkisemils/orgs","repos_url":"https://api.github.com/users/pinkisemils/repos","events_url":"https://api.github.com/users/pinkisemils/events{/privacy}","received_events_url":"https://api.github.com/users/pinkisemils/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2018-06-28T23:22:52Z","updated_at":"2018-06-29T03:02:15Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/mullvad/mullvadvpn-app/pulls/260","html_url":"https://github.com/mullvad/mullvadvpn-app/pull/260","diff_url":"https://github.com/mullvad/mullvadvpn-app/pull/260.diff","patch_url":"https://github.com/mullvad/mullvadvpn-app/pull/260.patch"},"body":"Here comes the Linux firewall integration.\r\n\r\nBeware, I have not tested it very extensively yet. There might even be an entire class of rules missing or misbehaving. I just feel like I have been massaging this for long enough without any insight into what I'm doing. So I don't think I can make it much better without having a round of feedback first. I have probably become blind to my own errors by now.\r\n\r\nSorry for it being a bit of a large PR. You can follow some changes commit for commit at first, but then the entire firewall implementation comes in one commit sadly.\r\n\r\nOur netfilter library is very low level, and as such creating the rules here becomes very low level. Ask about anything you don't understand and I'll try to answer as best as I can :)\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/comments/401233395","html_url":"https://github.com/mullvad/mullvadvpn-app/pull/260#issuecomment-401233395","issue_url":"https://api.github.com/repos/mullvad/mullvadvpn-app/issues/260","id":401233395,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMTIzMzM5NQ==","user":{"login":"jvff","id":3216731,"node_id":"MDQ6VXNlcjMyMTY3MzE=","avatar_url":"https://avatars3.githubusercontent.com/u/3216731?v=4","gravatar_id":"","url":"https://api.github.com/users/jvff","html_url":"https://github.com/jvff","followers_url":"https://api.github.com/users/jvff/followers","following_url":"https://api.github.com/users/jvff/following{/other_user}","gists_url":"https://api.github.com/users/jvff/gists{/gist_id}","starred_url":"https://api.github.com/users/jvff/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jvff/subscriptions","organizations_url":"https://api.github.com/users/jvff/orgs","repos_url":"https://api.github.com/users/jvff/repos","events_url":"https://api.github.com/users/jvff/events{/privacy}","received_events_url":"https://api.github.com/users/jvff/received_events","type":"User","site_admin":false},"created_at":"2018-06-29T03:02:15Z","updated_at":"2018-06-29T03:02:15Z","author_association":"CONTRIBUTOR","body":"\n\n\n\nReviewed 6 of 9 files at r1, 3 of 4 files at r2.\nReview status: 0 of 10 files reviewed, 9 unresolved discussions (waiting on @pronebird, @pinkisemils, @jvff, and @faern)\n\n---\n\n*[talpid-core/Cargo.toml, line 12 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG7wZy04FzOz0gROMbx:-LG7wZy04FzOz0gROMby:bpzo2u4) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/Cargo.toml#L12)):*\n> ```TOML\n> duct = \"0.10\"\n> error-chain = \"0.12\"\n> ipnetwork = \"0.13\"\n> ```\n\nCan this be moved to `[target.'cfg(unix)'.dependencies]`?\n\n---\n\n*[talpid-core/Cargo.toml, line 15 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG7xFEO9IiukfUGSYrs:-LG7wtHG-J8WqNBcwy0H:byqggtt) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/Cargo.toml#L15)):*\n> ```TOML\n> jsonrpc-core = { git = \"https://github.com/paritytech/jsonrpc\", tag = \"v8.0.1\" }\n> jsonrpc-macros = { git = \"https://github.com/paritytech/jsonrpc\", tag = \"v8.0.1\" }\n> lazy_static = \"1.0\"\n> ```\n\nCan this be moved to `[target.'cfg(target_os = \"linux\")'.dependencies]`?\n\n---\n\n*[talpid-core/src/lib.rs, line 20 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG7wgzj4MhAvKQ-G4mB:-LG7wgzj4MhAvKQ-G4mC:bq0o0nb) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/lib.rs#L20)):*\n> ```Rust\n> #[macro_use]\n> extern crate error_chain;\n> extern crate ipnetwork;\n> ```\n\nCan this be `#[cfg(unix)]`?\n\n---\n\n*[talpid-core/src/lib.rs, line 25 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG7xEIW1BDsMjUEuMrW:-LG7x25KDIkNAW7cKtE7:b-jtrare) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/lib.rs#L25)):*\n> ```Rust\n> extern crate jsonrpc_macros;\n> #[macro_use]\n> extern crate lazy_static;\n> ```\n\nCan this be `#[cfg(target_os = \"linux\")]`?\n\n---\n\n*[talpid-core/src/firewall/mod.rs, line 9 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG7xMLj5tAjEsElsg8I:-LG7xMLj5tAjEsElsg8J:bmgbpj2) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/firewall/mod.rs#L9)):*\n> ```Rust\n> \n> #[cfg(unix)]\n> lazy_static! {\n> ```\n\nHopefully your PR for `rustc` from the Impl Days gets merged in time for Rust 2018 :)\n\n---\n\n*[talpid-core/src/firewall/linux/mod.rs, line 91 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG8Mg6d6wFp2JQvOY6T:-LG8Mg6d6wFp2JQvOY6U:bbkjurw) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/firewall/linux/mod.rs#L91)):*\n> ```Rust\n>         }\n> \n>         // First try to set the new policy with a batch starting with a command to delete the table\n> ```\n\nWould the same approach used in `reset_policy` work here? Adding a \"dummy\" table first with the same name then deleting it?\n\n---\n\n*[talpid-core/src/firewall/linux/mod.rs, line 155 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG8NuRf5WIBc2L8AaKF:-LG8NuRf5WIBc2L8AaKG:blr20hq) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/firewall/linux/mod.rs#L155)):*\n> ```Rust\n>             .recv(&mut buf[..])\n>             .chain_err(|| ErrorKind::NetlinkRecvError)?;\n>         trace!(\"Read {} bytes from netlink\", ret);\n> ```\n\nThis might be a style issue, but I would try to move the 4 lines above into a separate function that returns the `&buf[..ret]` slice in an `Option`, so you could do something like `while let Some(bytes) = read_from_nl_socket(&mut socket, &mut buf)`. Not sure if there might be NLL requirements due to the borrows.\n\n---\n\n*[talpid-core/src/firewall/linux/mod.rs, line 157 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260#-LG8NHan7OR0PDDhfSwZ:-LG8NHan7OR0PDDhfSw_:bspz353) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/74311ef95a106535fc0cebc78c104b694fdbb149/talpid-core/src/firewall/linux/mod.rs#L157)):*\n> ```Rust\n>         trace!(\"Read {} bytes from netlink\", ret);\n>         while ret > 0 {\n>             match mnl::cb_run(&buf[..ret], 2, portid) {\n> ```\n\nIs it possible to add a `?` here? That would remove one of the match variants.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/mullvad/mullvadvpn-app/260)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"jvff"}}