Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace "smart routing" with "direct only" #6935

Merged

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented Oct 7, 2024


This change is Reviewable

Copy link

linear bot commented Oct 7, 2024

@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from 86dbf07 to 0eebd62 Compare October 7, 2024 10:23
@Serock3 Serock3 requested a review from hulthe October 7, 2024 10:52
@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from c3636e6 to 6eb7147 Compare October 7, 2024 11:02
@Serock3 Serock3 requested a review from Rawa October 7, 2024 11:19
@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from c79508b to 02809a6 Compare October 7, 2024 11:31
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 10 files at r1, 1 of 1 files at r3, 17 of 23 files at r5.
Reviewable status: 18 of 27 files reviewed, 1 unresolved discussion (waiting on @Serock3)


mullvad-types/src/wireguard.rs line 101 at r5 (raw file):
Proposal:

#[cfr_attr(target_os = "android", serde(skip))]

Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 18 of 27 files reviewed, all discussions resolved (waiting on @hulthe)


mullvad-types/src/wireguard.rs line 101 at r5 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Proposal:

#[cfr_attr(target_os = "android", serde(skip))]

Done

@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from 02809a6 to 85fecf1 Compare October 7, 2024 11:48
@raksooo raksooo requested a review from hulthe October 7, 2024 11:57
raksooo
raksooo previously approved these changes Oct 7, 2024
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 21 files at r2, 11 of 23 files at r5, 1 of 1 files at r7.
Reviewable status: 25 of 27 files reviewed, all discussions resolved (waiting on @hulthe)

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 10 files at r1, 21 of 23 files at r5, 1 of 1 files at r7.
Reviewable status: 25 of 27 files reviewed, 1 unresolved discussion (waiting on @hulthe and @Serock3)


mullvad-daemon/src/management_interface.rs line 359 at r7 (raw file):

        log::debug!("set_daita_direct_only({value})");
        let (tx, rx) = oneshot::channel();
        self.send_command_to_daemon(DaemonCommand::SetDaitaUseMultihopIfNecessary(tx, !value))?;

Nit: value is a bit vague. I would prefer if the variable name carried a hint on what the value represents, e.g. use_direct_only.

Code quote:

        let value = request.into_inner();
        log::debug!("set_daita_direct_only({value})");
        let (tx, rx) = oneshot::channel();
        self.send_command_to_daemon(DaemonCommand::SetDaitaUseMultihopIfNecessary(tx, !value))?;

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

daemon :lgtm:

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: 26 of 27 files reviewed, 1 unresolved discussion (waiting on @Serock3)

Pururun
Pururun previously approved these changes Oct 7, 2024
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

Pururun
Pururun previously approved these changes Oct 7, 2024
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch 3 times, most recently from d7b2776 to 4928e45 Compare October 7, 2024 13:11
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r9, 1 of 2 files at r12, all commit messages.
Reviewable status: 26 of 27 files reviewed, all discussions resolved (waiting on @raksooo)

@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch 2 times, most recently from 156909c to 444258c Compare October 7, 2024 13:31
hulthe
hulthe previously approved these changes Oct 7, 2024
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from 444258c to 120fa86 Compare October 7, 2024 13:36
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Serock3 and others added 11 commits October 7, 2024 15:40
Simplify the logic for feature indicators
We now simply show the "multihop" indicator there is an entry endpoint,
regardless of whether it was activated manually or through DAITA. This
reflects the intent to base the feature indicators on the current
connection and not the user settings. There is no special indicator for
"smart routing" or "direct only".
For android, it is set to true, as multihop is not supported.

Note that in the daemon, the setting is called
`use_multihop_if_necessary` and has the inverse meaning.
@Serock3 Serock3 force-pushed the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch from 120fa86 to 7770131 Compare October 7, 2024 13:42
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r13, 1 of 1 files at r14.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 merged commit bdd4675 into main Oct 7, 2024
65 checks passed
@Serock3 Serock3 deleted the rename-smart_routing-to-use_multihop_if_necessary-des-1292 branch October 7, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants