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

Add mtu for network connections #1101

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jcronenberg
Copy link
Contributor

Problem

No mtu support for network connections

Solution

Add mtu for network connections

Testing

  • Modified existing unit tests
  • Tested manually

@coveralls
Copy link

coveralls commented Mar 18, 2024

Coverage Status

coverage: 74.364% (-0.1%) from 74.464%
when pulling 1febd18 on jcronenberg:add_mtu_upstream
into e63f418 on openSUSE:master.

)]);
let ethernet_config = HashMap::from([
(
"assigned-mac-address",
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we could do this assignation generally and not based in the specific connection type taking into account that in case of empty it could be removed later when the connection is cleaned up just to avoid the code duplication...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe something like this?

    match &conn.config {
        ConnectionConfig::Wireless(wireless) => {
            connection_dbus.insert("type", WIRELESS_KEY.into());
            result.extend(wireless_config_to_dbus(wireless));
        }
        // ...
    }

    let conn_config = HashMap::from([
        (
            "assigned-mac-address",
            Value::new(conn.mac_address.to_string()),
        ),
        ("mtu", Value::new(conn.mtu)),
    ]);
    if conn.is_ethernet() {
        result.insert(ETHERNET_KEY, conn_config);
    } else if let ConnectionConfig::Wireless(_) = conn.config {
        result.get_mut(WIRELESS_KEY).unwrap().extend(conn_config);
    }

or like this?

    match &conn.config {
        ConnectionConfig::Wireless(wireless) => {
            connection_dbus.insert("type", WIRELESS_KEY.into());
            result.extend(wireless_config_to_dbus(wireless));
        }
        // ...
    }

    add_base_conn_values(conn, &mut result);
    // ...
}

fn add_base_conn_values(conn: &Connection, conn_result: &mut NestedHash) {
    let conn_type = if conn.is_ethernet() {
        ETHERNET_KEY
    } else if let ConnectionConfig::Wireless(_) = conn.config {
        WIRELESS_KEY
    } else {
        return;
    };
    let conn_config = HashMap::from([
        (
            "assigned-mac-address",
            Value::new(conn.mac_address.to_string()),
        ),
        ("mtu", Value::new(conn.mtu)),
    ]);
    if let Some(conn_entry) = conn_result.get_mut(conn_type) {
        conn_entry.extend(conn_config);
    } else {
        conn_result.insert(conn_type, conn_config);
    }
}

Generally without identifying connection type I don't think is possible, because it needs to be inserted at the *_KEY key.
The above solutions at least avoid the duplicate HashMap generation code. I personally don't really have a preference 🙂

@teclator
Copy link
Contributor

In general LGTM

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.

None yet

4 participants