Rework the `mullvad status listen` command #6678

Serock3 · 2024-08-26T09:34:37Z

Rework the mullvad status listen command

Format the tunnel states with indentation similar to other output from the CLI
Without the --verbose flag, fields are left out if they are empty or updated to the same value (e.g. when going from connected -> connected with new features, "Relay" is not printed again)
Print feature indicators for connecting and connected states

Example output:

Disconnected
Connecting
    Visible location:       Netherlands, Amsterdam
    Features:               Quantum Resistance, Udp2Tcp
    Relay:                  nl-ams-wg-202
    Relay (updated):        nl-ams-wg-006
    Relay (updated):        nl-ams-wg-007
    Features (updated):     Bridge Mode
    Relay (updated):        nl-ams-ovpn-001 via fr-par-br-001
Connected
    Visible location (updated):Netherlands, Amsterdam. IPv4: 185.65.134.138

Example output (with --verbose flag):

Disconnected
    Visible location:       Sweden, Uppsala. IPv4: 158.174.237.162
Connecting
    Bridge type:            None
    Tunnel type:            WireGuard
    Relay:                  nl-ams-wg-007 ([2a03:1b20:3:f011::f701]:443/UDP)
    Visible location:       Netherlands, Amsterdam
    Tunnel interface:       None
    Features:               Quantum Resistance, Udp2Tcp
Connecting
    Relay (updated):        nl-ams-wg-201 ([2a02:6ea0:c034:1::a30f]:443/UDP)
    Tunnel interface:       None
    Bridge type:            None
    Tunnel type:            WireGuard
    Features:               Quantum Resistance, Udp2Tcp
    Visible location:       Netherlands, Amsterdam
Connecting
    Relay:                  nl-ams-wg-201 ([2a02:6ea0:c034:1::a30f]:443/UDP)
    Tunnel interface:       None
    Tunnel type:            WireGuard
    Features:               Quantum Resistance, Udp2Tcp
    Visible location:       Netherlands, Amsterdam
    Bridge type:            None
Connecting
    Tunnel interface:       None
    Visible location:       Netherlands, Amsterdam
    Bridge type:            Shadowsocks
    Tunnel type (updated):  OpenVPN
    Relay (updated):        nl-ams-ovpn-005 (185.65.134.75:443/TCP) via fr-par-br-001 (193.32.126.117:1234/TCP)
    Features (updated):     Bridge Mode
Connected
    Relay:                  nl-ams-ovpn-005 (185.65.134.75:443/TCP) via fr-par-br-001 (193.32.126.117:1234/TCP)
    Bridge type:            Shadowsocks
    Tunnel type:            OpenVPN
    Visible location:       Netherlands, Amsterdam
    Features:               Bridge Mode
    Tunnel interface:       tun0
Connected
    Features:               Bridge Mode
    Relay:                  nl-ams-ovpn-005 (185.65.134.75:443/TCP) via fr-par-br-001 (193.32.126.117:1234/TCP)
    Tunnel type:            OpenVPN
    Visible location (updated):Netherlands, Amsterdam. IPv4: 185.65.134.189
    Bridge type:            Shadowsocks
    Tunnel interface:       tun0

This change is

hulthe

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @Serock3)

mullvad-cli/src/format.rs line 31 at r1 (raw file):

    // graphical frontends updating the drawn state with an identical one is
    // invisible, so this is only an issue for the CLI.
    match (&previous_state, &state) {

I like what you're doing here, and I generally prefer not nesting match statements if possible, but in this case the patterns are quite verbose. Have you considered only matching on state, and matching again on previous_state in the individual branches where needed? That could also help to get rid of the unreachable! statement below.

mullvad-cli/src/format.rs line 96 at r1 (raw file):

                }
                _ => {}
            }

nit: if let would look better here imo

mullvad-cli/src/format.rs line 101 at r1 (raw file):

                println!(
                    "Updated features: {}",
                    format_feature_indicators(feature_indicators)

Maybe we should do a pass over impl Display for FeatureIndicator and make sure we match what the graphical client is showing

mullvad-cli/src/format.rs line 211 at r2 (raw file):

}

pub fn print_location(location: &GeoIpLocation) {

This prints "Your connection appears to be from ", but it's sometimes used to print where you are connected to

Serock3

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @hulthe)

mullvad-cli/src/format.rs line 31 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I like what you're doing here, and I generally prefer not nesting match statements if possible, but in this case the patterns are quite verbose. Have you considered only matching on state, and matching again on previous_state in the individual branches where needed? That could also help to get rid of the unreachable! statement below.

I swapped to nested matches, but I also basically re-wrote the entire thing. It ended up much nicer in my opinion. Take a look!

mullvad-cli/src/format.rs line 101 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Maybe we should do a pass over impl Display for FeatureIndicator and make sure we match what the graphical client is showing

Don't think that the GUI can make use the Display impl, but it's a good idea anyway, so I did it.

mullvad-cli/src/format.rs line 211 at r2 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

This prints "Your connection appears to be from ", but it's sometimes used to print where you are connected to

I replaced it with the much simpler "Visible location"

hulthe

Reviewed 1 of 2 files at r4.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @Serock3)

mullvad-cli/src/format.rs line 31 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I swapped to nested matches, but I also basically re-wrote the entire thing. It ended up much nicer in my opinion. Take a look!

Nice, i like the changes you made

mullvad-cli/src/format.rs line 202 at r4 (raw file):

            (Some(value), Some(_)) if verbose => print_option!(name, value),
            (None, None) if verbose => print_option!(name, "None"),
            (None, Some(_)) => print_option!(format!("{name} (updated):"), "None"),

The colon here is redundant, print_option already prints a colon

Also it looks like some of the option names are too long (> 24 characters) with the " (updated)" postfix, which breaks the space alignment of print_option

mullvad-types/src/features.rs line 72 at r4 (raw file):

impl FeatureIndicator {
    const fn to_str(&self) -> &str {

Should return a &'static str

hulthe

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @Serock3)

mullvad-cli/src/format.rs line 152 at r4 (raw file):

    feature_indicators: Option<&FeatureIndicators>,
    verbose: bool,
) -> HashMap<&'static str, Option<String>> {

Hmm. since you're using a HashMap, this will print stuff in a random order no?

Serock3

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @hulthe)

mullvad-cli/src/format.rs line 152 at r4 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Hmm. since you're using a HashMap, this will print stuff in a random order no?

Yup, considered sorting the output, but didn't see a good reason for it. Do you think it's necessary?

mullvad-cli/src/format.rs line 202 at r4 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

The colon here is redundant, print_option already prints a colon

Also it looks like some of the option names are too long (> 24 characters) with the " (updated)" postfix, which breaks the space alignment of print_option

Fixed the colon.

Serock3

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @hulthe)

mullvad-types/src/features.rs line 72 at r4 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Should return a &'static str

Done.

hulthe

Reviewable status: 1 of 5 files reviewed, all discussions resolved

hulthe

Reviewed 1 of 1 files at r6.
Reviewable status: 1 of 5 files reviewed, all discussions resolved

hulthe

Reviewed all commit messages.
Reviewable status: 1 of 5 files reviewed, all discussions resolved

Serock3 force-pushed the cli-status-listen-rework branch from 31ab848 to d66aae5 Compare August 26, 2024 11:40

Serock3 requested a review from hulthe August 27, 2024 09:12

hulthe reviewed Sep 2, 2024

View reviewed changes

Serock3 force-pushed the cli-status-listen-rework branch 5 times, most recently from d786bff to aee338f Compare September 3, 2024 08:45

Serock3 self-assigned this Sep 3, 2024

Serock3 marked this pull request as ready for review September 3, 2024 08:45

Serock3 commented Sep 3, 2024

View reviewed changes

Serock3 force-pushed the cli-status-listen-rework branch from 63dbedc to dc38595 Compare September 3, 2024 09:41

hulthe requested changes Sep 3, 2024

View reviewed changes

hulthe reviewed Sep 3, 2024

View reviewed changes

Serock3 commented Sep 3, 2024

View reviewed changes

Serock3 force-pushed the cli-status-listen-rework branch from dc38595 to f95f737 Compare September 3, 2024 16:22

Serock3 commented Sep 3, 2024

View reviewed changes

hulthe approved these changes Sep 9, 2024

View reviewed changes

Serock3 force-pushed the cli-status-listen-rework branch from f95f737 to c6da720 Compare September 16, 2024 09:00

hulthe previously approved these changes Sep 16, 2024

View reviewed changes

Serock3 dismissed hulthe’s stale review via f95f737 September 16, 2024 12:20

Serock3 force-pushed the cli-status-listen-rework branch 2 times, most recently from f95f737 to 15c5b5b Compare September 16, 2024 12:23

Serock3 added 3 commits September 16, 2024 14:29

Add Display and is_empty to FeatureIndicators

281438b

Refactor mullvad status listen command

9182b4a

Add to changelog

485e931

Serock3 force-pushed the cli-status-listen-rework branch from 15c5b5b to 485e931 Compare September 16, 2024 12:29

hulthe approved these changes Sep 16, 2024

View reviewed changes

Serock3 merged commit c3e95c0 into main Sep 16, 2024
53 of 54 checks passed

Serock3 deleted the cli-status-listen-rework branch September 16, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the `mullvad status listen` command #6678

Rework the `mullvad status listen` command #6678

Serock3 commented Aug 26, 2024 •

edited

Loading

hulthe left a comment

Serock3 left a comment

hulthe left a comment

hulthe left a comment

Serock3 left a comment

Serock3 left a comment

hulthe left a comment

hulthe left a comment

hulthe left a comment

Rework the mullvad status listen command #6678

Rework the mullvad status listen command #6678

Conversation

Serock3 commented Aug 26, 2024 • edited Loading

hulthe left a comment

Choose a reason for hiding this comment

Serock3 left a comment

Choose a reason for hiding this comment

hulthe left a comment

Choose a reason for hiding this comment

hulthe left a comment

Choose a reason for hiding this comment

Serock3 left a comment

Choose a reason for hiding this comment

Serock3 left a comment

Choose a reason for hiding this comment

hulthe left a comment

Choose a reason for hiding this comment

hulthe left a comment

Choose a reason for hiding this comment

hulthe left a comment

Choose a reason for hiding this comment

Rework the `mullvad status listen` command #6678

Rework the `mullvad status listen` command #6678

Serock3 commented Aug 26, 2024 •

edited

Loading