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

Merged
merged 3 commits into from
Jun 11, 2024

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

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

In general it looks good. Just a minor comment about a call to unwrap.

let mut wireless_dbus = wireless_config_to_dbus(wireless);
wireless_dbus
.get_mut(WIRELESS_KEY)
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would avoid unwrapping (and potentially crashing). You can use a default value or ignore it completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean currently this is impossible, but for future-proofing, sure.

@imobachgs imobachgs merged commit b2d7fca into agama-project:master Jun 11, 2024
1 check passed
@jcronenberg jcronenberg deleted the add_mtu_upstream branch June 12, 2024 10:28
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
imobachgs added a commit that referenced this pull request Jun 27, 2024
Prepare for releasing Agama 9. It includes the following pull requests:

- #1101
- #1202
- #1228
- #1231
- #1236
- #1238
- #1239
- #1240
- #1242
- #1243
- #1244
- #1245
- #1246
- #1247
- #1248
- #1249
- #1250
- #1251
- #1252
- #1253
- #1254
- #1255
- #1256
- #1257
- #1258
- #1259
- #1260
- #1261
- #1264
- #1265
- #1267
- #1268
- #1269
- #1270
- #1271
- #1272
- #1273
- #1274
- #1279
- #1280
- #1284
- #1285
- #1286
- #1287
- #1288
- #1289
- #1290
- #1291
- #1292
- #1293
- #1294
- #1295
- #1296
- #1298
- #1299
- #1300
- #1301
- #1302
- #1303
- #1304
- #1305
- #1306
- #1307
- #1308
- #1309
- #1310
- #1311
- #1312
- #1313
- #1314
- #1315
- #1316
- #1317
- #1318
- #1319
- #1320
- #1321
- #1322
- #1323
- #1324
- #1325
- #1326
- #1328
- #1329
- #1331
- #1332
- #1334
- #1338
- #1340
- #1341
- #1342
- #1343
- #1344
- #1345
- #1348
- #1349
- #1351
- #1352
- #1353
- #1354
- #1355
- #1356
- #1357
- #1358
- #1359
- #1360
- #1361
- #1362
- #1363
- #1365
- #1366
- #1367
- #1368
- #1371
- #1372
- #1374
- #1375
- #1376
- #1379
- #1380
- #1381
- #1383
- #1384
- #1385
- #1386
- #1387
- #1388
- #1389
- #1391
- #1392
- #1394
- #1395
- #1397
- #1398
- #1399
- #1400
- #1403
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.

4 participants