Bug 277114 - x11/lightdm: fix tty counting in patch-src_x-server.c
Summary: x11/lightdm: fix tty counting in patch-src_x-server.c
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-desktop (Team)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-17 17:17 UTC by Dan Kotowski
Modified: 2024-02-26 19:06 UTC (History)
2 users (show)

See Also:
madpilot: maintainer-feedback+
madpilot: merge-quarterly+


Attachments
patch v1 (2.88 KB, patch)
2024-02-20 09:36 UTC, Guido Falsi
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Kotowski 2024-02-17 17:17:14 UTC
In patch-src_x-server.c we currently pass "ttyv%d", but we count TTYs in hex, not decimal and so it should be:

+        tty_text = g_strdup_printf ("/dev/ttyv%x", vt - 1);

This bug was identified in 221452, but the accepted patches bandage over the issue rather than fixing the root cause, which is the counting.

This patch was suggested and actually resolves the bug:
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=236106&action=diff

However, it is also possible that these are base32 numbers rather than base16 and points here: https://github.com/swaywm/wlroots/pull/1945

With this fixed, it may again be possible to pull back in val's consolekit2 patch as well: https://github.com/valpackett/ConsoleKit2/commit/18a058576d118dec428d81c7e2e3369d9ec939d0.patch
Comment 1 Guido Falsi freebsd_committer freebsd_triage 2024-02-18 16:46:56 UTC
Hi,

Thanks for pointing this out, I was definitely not aware of the issue being present in lightdm.

The basic idea in your patch looks fine, I need to investigate the console numbering logic. I'm reasonably sure they are base16, but I better check this.

I also think this needs to be proposed upstream, so it can be included in the official sources.

So please be patient while I do my research/testing.

Will report back with any news.
Comment 2 Guido Falsi freebsd_committer freebsd_triage 2024-02-18 18:31:29 UTC
About device numbering for virtual ttys, I've found no definitive data, looking at the source code.

Anyway defaults are 12 ttys for vt(4) and 16 for legacy sc(4). Both number them with sequential numbers and it looks like they are rendered in /dev like this:

> ls -1 /dev/ttyv*
/dev/ttyv0
/dev/ttyv1
/dev/ttyv2
/dev/ttyv3
/dev/ttyv4
/dev/ttyv5
/dev/ttyv6
/dev/ttyv7
/dev/ttyv8
/dev/ttyv9
/dev/ttyva
/dev/ttyvb


Now, the issue is what happens when we get more than 16, if it becomes ttyv00 -> ttyv0a or it goes ttyv0 -> ttyvf and then ttyv10 -> ttyv1f (my guess is the latter).

I'm not sure calling the naming scheme "base32" is correct though. These are anyway hex numbers. Base32 is a completely different thing.

I could not identify the kernel code actually doing the naming of these devices.


My best guess if that rendering there as hex numbers is the correct way to proceed.

I'm not sure I understand what should be changed in the consolekit patch. that patch is in a repo external to ports, and there is no such patch in the ports tree I can see. So you will need to suggest any changes there.


P.S.

To get more than 12 vtys with vt and 16 with sc one needs to recompile the kernel changing a kernel config variable, so I guess the number of people with more than these terminals to be quite low.
Comment 3 Gleb Popov freebsd_committer freebsd_triage 2024-02-18 19:27:14 UTC
(In reply to Guido Falsi from comment #2)

> I could not identify the kernel code actually doing the naming of these devices.

If I'm not mistaken, here it is: https://github.com/freebsd/freebsd-src/blob/d0bf8b5aaccbe082d3728279620b8f8474d9ca8a/sys/dev/vt/vt_core.c#L1854

The %r modifier uses "current" radix, which is obviously 16 in our case.

> Base32 is a completely different thing.

Not completely. Base32 is the same encoding as base16 with only difference that we're using a radix of 32. Which would mean that after ttyvf it would be ttyvg and then up to some 32-th character (probably a capital one).

> I'm not sure I understand what should be changed in the consolekit patch.

I was hacking ConsoleKit a lot during last autumn and this patch doesn't seem to be really needed from my POV. The code it changes already follow the same path, IIRC.
Comment 4 Dan Kotowski 2024-02-18 20:30:22 UTC
I bumped VT_MAXWINDOWS from 12 to 120, and sure enough:

$ git -C /usr/src diff
diff --git a/sys/dev/vt/vt.h b/sys/dev/vt/vt.h
index 56a28c0420c7..ff80062ea4bc 100644
--- a/sys/dev/vt/vt.h
+++ b/sys/dev/vt/vt.h
@@ -55,7 +55,7 @@
 #ifdef MAXCONS
 #define        VT_MAXWINDOWS   MAXCONS
 #else
-#define        VT_MAXWINDOWS   12
+#define        VT_MAXWINDOWS   120
 #endif
 #endif

$ ls /dev/ttyv*
/dev/ttyv0  /dev/ttyv1i /dev/ttyv25 /dev/ttyv2p /dev/ttyv3c /dev/ttyvc
/dev/ttyv1  /dev/ttyv1j /dev/ttyv26 /dev/ttyv2q /dev/ttyv3d /dev/ttyvd
/dev/ttyv10 /dev/ttyv1k /dev/ttyv27 /dev/ttyv2r /dev/ttyv3e /dev/ttyve
/dev/ttyv11 /dev/ttyv1l /dev/ttyv28 /dev/ttyv2s /dev/ttyv3f /dev/ttyvf
/dev/ttyv12 /dev/ttyv1m /dev/ttyv29 /dev/ttyv2t /dev/ttyv3g /dev/ttyvg
/dev/ttyv13 /dev/ttyv1n /dev/ttyv2a /dev/ttyv2u /dev/ttyv3h /dev/ttyvh
/dev/ttyv14 /dev/ttyv1o /dev/ttyv2b /dev/ttyv2v /dev/ttyv3i /dev/ttyvi
/dev/ttyv15 /dev/ttyv1p /dev/ttyv2c /dev/ttyv3  /dev/ttyv3j /dev/ttyvj
/dev/ttyv16 /dev/ttyv1q /dev/ttyv2d /dev/ttyv30 /dev/ttyv3k /dev/ttyvk
/dev/ttyv17 /dev/ttyv1r /dev/ttyv2e /dev/ttyv31 /dev/ttyv3l /dev/ttyvl
/dev/ttyv18 /dev/ttyv1s /dev/ttyv2f /dev/ttyv32 /dev/ttyv3m /dev/ttyvm
/dev/ttyv19 /dev/ttyv1t /dev/ttyv2g /dev/ttyv33 /dev/ttyv3n /dev/ttyvn
/dev/ttyv1a /dev/ttyv1u /dev/ttyv2h /dev/ttyv34 /dev/ttyv4  /dev/ttyvo
/dev/ttyv1b /dev/ttyv1v /dev/ttyv2i /dev/ttyv35 /dev/ttyv5  /dev/ttyvp
/dev/ttyv1c /dev/ttyv2  /dev/ttyv2j /dev/ttyv36 /dev/ttyv6  /dev/ttyvq
/dev/ttyv1d /dev/ttyv20 /dev/ttyv2k /dev/ttyv37 /dev/ttyv7  /dev/ttyvr
/dev/ttyv1e /dev/ttyv21 /dev/ttyv2l /dev/ttyv38 /dev/ttyv8  /dev/ttyvs
/dev/ttyv1f /dev/ttyv22 /dev/ttyv2m /dev/ttyv39 /dev/ttyv9  /dev/ttyvt
/dev/ttyv1g /dev/ttyv23 /dev/ttyv2n /dev/ttyv3a /dev/ttyva  /dev/ttyvu
/dev/ttyv1h /dev/ttyv24 /dev/ttyv2o /dev/ttyv3b /dev/ttyvb  /dev/ttyvv
Comment 5 Gleb Popov freebsd_committer freebsd_triage 2024-02-18 20:50:48 UTC
Yes, I was mistaken. Here's the relevant code as pointed out by eugen@ https://github.com/freebsd/freebsd-src/blob/d0bf8b5aaccbe082d3728279620b8f8474d9ca8a/sys/kern/subr_terminal.c#L234

Is uses radix=32 there, so it is indeed base32.
Comment 6 Guido Falsi freebsd_committer freebsd_triage 2024-02-18 22:55:47 UTC
(In reply to Gleb Popov from comment #5)

Thanks all for the explanations. Now I understand what we are talking about.

The base32 things makes things a little more complicated since, AFAIK, there is no %r format in userland printf, I'll take a better look tomorrow anyway.
Comment 7 Guido Falsi freebsd_committer freebsd_triage 2024-02-19 16:54:37 UTC
I have created a patch to the upstream code I plan to try to send as a pull request to them, you can find my commit here:

https://github.com/madpilot78/lightdm/commit/3d3a18b096ede48b618561489ab49de019242bd8

I'm still testing it. (may not apply directly to the release code, since there have been changes to the file since)

I'll post it as a ports tree patch once I'm done with my basic testing.


Any suggestions about this patch welcome.
Comment 8 Guido Falsi freebsd_committer freebsd_triage 2024-02-19 16:56:27 UTC
New link, I rebased my branch:

https://github.com/madpilot78/lightdm/commit/d12687c9da53a6575b4fcbba5808fc38508bd020
Comment 9 Guido Falsi freebsd_committer freebsd_triage 2024-02-19 16:59:51 UTC
(In reply to Guido Falsi from comment #8)

Maybe it would be cleaner if I put the logic in its own function.

I'll refactor it later, maybe.
Comment 10 Guido Falsi freebsd_committer freebsd_triage 2024-02-20 09:36:42 UTC
Created attachment 248626 [details]
patch v1

Hi,

I've attached the patch I'm using. It works fine here.

Please try to test it and report back. My intention is to commit this to the ports tree.
Comment 11 Guido Falsi freebsd_committer freebsd_triage 2024-02-20 09:37:36 UTC
Posted upstream as https://github.com/canonical/lightdm/pull/343
Comment 12 Dan Kotowski 2024-02-20 12:31:48 UTC
(In reply to Guido Falsi from comment #10)

Testing - this solves the issue for me
Comment 13 Guido Falsi freebsd_committer freebsd_triage 2024-02-26 16:19:45 UTC
I plan to commit this shortly, and I think this is also a candidate for merging to quarterly.
Comment 14 commit-hook freebsd_committer freebsd_triage 2024-02-26 19:04:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=c88dffc0c8b6f2c82a522d8eb2dcd2b49e9a4015

commit c88dffc0c8b6f2c82a522d8eb2dcd2b49e9a4015
Author:     Guido Falsi <madpilot@FreeBSD.org>
AuthorDate: 2024-02-26 19:00:03 +0000
Commit:     Guido Falsi <madpilot@FreeBSD.org>
CommitDate: 2024-02-26 19:00:03 +0000

    x11/lightdm: Fix ttyv numbering logic

    Adapt code from wlroots to correctly generate ttyv device names for
    numbers beyond 9.

    Upstream pull request: https://github.com/canonical/lightdm/pull/343

    PR:             277114
    Obtained from:  https://github.com/swaywm/wlroots/commit/fc6c0ca12e941d5d7d567834bff3ab7df9447001 (inspired by)
    MFH:            2024Q1

 x11/lightdm/Makefile                   |  2 +-
 x11/lightdm/files/patch-src_x-server.c | 60 ++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 7 deletions(-)
Comment 15 commit-hook freebsd_committer freebsd_triage 2024-02-26 19:06:12 UTC
A commit in branch 2024Q1 references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=79674f5097cfec61c7e981ede019bb449f6e28e6

commit 79674f5097cfec61c7e981ede019bb449f6e28e6
Author:     Guido Falsi <madpilot@FreeBSD.org>
AuthorDate: 2024-02-26 19:00:03 +0000
Commit:     Guido Falsi <madpilot@FreeBSD.org>
CommitDate: 2024-02-26 19:04:59 +0000

    x11/lightdm: Fix ttyv numbering logic

    Adapt code from wlroots to correctly generate ttyv device names for
    numbers beyond 9.

    Upstream pull request: https://github.com/canonical/lightdm/pull/343

    PR:             277114
    Obtained from:  https://github.com/swaywm/wlroots/commit/fc6c0ca12e941d5d7d567834bff3ab7df9447001 (inspired by)
    MFH:            2024Q1

    (cherry picked from commit c88dffc0c8b6f2c82a522d8eb2dcd2b49e9a4015)

 x11/lightdm/Makefile                   |  2 +-
 x11/lightdm/files/patch-src_x-server.c | 60 ++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 7 deletions(-)
Comment 16 Guido Falsi freebsd_committer freebsd_triage 2024-02-26 19:06:40 UTC
Patch committed and merged.

Thanks!