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

Use a timeout when calling setxkbmap #1249

Merged
merged 6 commits into from
May 23, 2024
Merged

Use a timeout when calling setxkbmap #1249

merged 6 commits into from
May 23, 2024

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented May 23, 2024

Problem

  • When calling setxkbmap in development the root user might not have access to the :0 X display.
  • Maybe there is XDM running or another user is logged in. In that case setxkbmap prints
    Authorization required, but no authorization protocol specified
    
    error message in an infinite loop and gets stuck.
  • Additionally the $DISPLAY might be already set so we should honor that value (e.g. when you manually start the web server from terminal)

Solution

  • Use the subprocess crate to limit the execution time
  • Use the current DISPLAY value when already set

SSH X Forwarding Problem

We need to set the display for the setxkbmap call when running on the Live ISO because when running Agama from systemd service the DISPLAY is not set but we need to access the locally running web browser so we have to explicitly set the :0 value.

When the web server is started manually from a terminal we should keep the current value if it is already set. But using the current DISPLAY value might behave a bit unexpectedly when you login to the system via SSH with X forwarding enabled. In that case the setxkbmap call will query and set (!) keyboard of the remote X server. That might be quite confusing.

Sure, that is a corner case, but if you select some exotic layout with complete different mapping like Dvorak then it might not be trivial to restore your original layout. When using X forwarding the DISPLAY is set to something like localhost:10.0, so maybe we should use it only when it starts with : character which means the local X server? 🤔

Testing

  • Tested manually

When calling `setxkbmap` in development the `root` user might
not have access to the `:0` X display.

Maybe there is XDM running or another user is logged in. In that
case `setxkbmap` prints

  Authorization required, but no authorization protocol specified

error message in an infinite loop and gets stucked.

Additionally the $DISPLAY might be already set so we should honor
that value.
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.

It looks good. Thanks!

However, I would prefer that function to live in some kind of "helpers" module because it is not related to localization and it is not that "small" (apart from bringing quite some elements into scope).

rust/agama-server/src/l10n/l10n.rs Outdated Show resolved Hide resolved
rust/agama-server/src/l10n/l10n.rs Outdated Show resolved Hide resolved
@lslezak
Copy link
Contributor Author

lslezak commented May 23, 2024

However, I would prefer that function to live in some kind of "helpers" module

I was thinking about that as well, but then I decided to keep it here. It is quite specialized, if we want to move it we should make it more generic and support more use cases (like reading stderr, getting the exit code, etc...).

@imobachgs
Copy link
Contributor

However, I would prefer that function to live in some kind of "helpers" module

I was thinking about that as well, but then I decided to keep it here. It is quite specialized, if we want to move it we should make it more generic and support more use cases (like reading stderr, getting the exit code, etc...).

Well, it looks generic enough for our use case 😉 But I am fine with keeping it there for a while. Just let's try to not forget that it is there so we do not implement something similar again 😅

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.

When working with arguments, it is usually preferred to use &str instead of &String. With that approach, you could simplify things a bit. Please, see 8301f3e and feel free to cherry-pick it if you want.

@lslezak lslezak merged commit 6443351 into master May 23, 2024
2 of 3 checks passed
@lslezak lslezak deleted the setxkbmap_timeout branch May 23, 2024 16:01
@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.

2 participants