Earnestly / sx

Start an xorg server
MIT License
231 stars 16 forks source link

portability fix #11

Closed prez closed 4 years ago

prez commented 4 years ago

ubase ps doesn't have a -o option

Earnestly commented 4 years ago

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ps.html

I'm not supporting software that doesn't follow the standard, fix ubase instead.

prez commented 4 years ago

I'm aware that POSIX mandates -o. And apologies, I read https://github.com/Earnestly/sx/pull/4 (and thus your argument against using tty) only after creating this PR. Are there any systems without a devtmpfs running Xorg?

prez commented 4 years ago

How about tty | tr -c -d [:digit:]?

Earnestly commented 4 years ago

Nope sorry, I'm not going to start working around incomplete software. POSIX defines a floor and if ubase can't even reach that level then I'm not going lower for it.

I also don't want to make assumptions where I can avoid them. Assuming devtmpfs includes assuming that it is mounted on /dev and that all those paths are hardcoded. There's no reason for this.

The use of the tty here is to couple the login console to the DISPLAY and vt arguments such that Xorg starts on the same console (where the user has authentication). There may be a better way go about this as I already don't like assuming it will be tty(n). This strategy also causes issues for the BSDs which is unfortunate.

prez commented 4 years ago

I understand your sentiment of not wanting to work around incomplete software very well, and in fact, I'd do exactly the same if this was my software. It is reasonable to specify a lowest common denominator, and exhaust all the possibilities provided by it in order to achieve optimality within that framework.

Could you please explain how (both of) the alternative solution(s) I provided are worse in the sense that they assume more, or are less efficient (except for the additional process in option 2)? Does ttyname() require devtmpfs to be present?

What is required in order for this to work on the BSDs? Maybe we can find a solution that both increases the portability of sx, while still leaving ubase users like myself satisfied.

Earnestly commented 4 years ago

I don't want to make assumptions about /dev as well as tty(n). Currently if tty was used on a terminal emulator you would get /dev/pts/2 for example, which would be passed as is. With your code it would be transformed to 2.

I don't particularly like the situation where I have to try and divine this number out of the tty. If there was a way to get this information in a more reliable/direct way I'd much prefer that. (Even if the solution means somehow obviating the need to set DISPLAY or use vt.)

I have an issue open about the BSD situation but I don't believe it is solvable.

prez commented 4 years ago

I don't particularly like the situation where I have to try and divine this number out of the tty

But isn't this precisely what the current code is doing?

which would be passed as is

It would yield pts/2. Then, am I correct in assuming that you want to divide out the number in case we're on a tty, and discard the upmost folder otherwise, without assuming that it is /dev? That seems solvable. Please forgive my ignorance, I am merely trying to understand.

Earnestly commented 4 years ago

But isn't this precisely what the current code is doing?

Yes, that's what I don't like.

It would yield pts/2.

I'm refering to the use of tr -cd '[:digit:]' resulting in 2.

Then, am I correct in assuming that you want to divide out the number in case we're on a tty [...]

I don't particularly want to detect whether the user is on a "tty" or "pts". What I would fundamentally like to have is $XDG_VTNR, but produced in a manner that is portable, or using standard interfaces/tools.

Earnestly commented 4 years ago

It appears pam_systemd determines XDG_VTNR from this routine:

/* from https://github.com/systemd/systemd/blob/master/src/basic/path-util.h#L160 */
static inline const char *skip_dev_prefix(const char *p) {
        const char *e;

        /* Drop any /dev prefix if there is any */

        e = path_startswith(p, "/dev/");

        return e ?: p;
}

int vtnr_from_tty(const char *tty) {
        int i, r;

        assert(tty);

        tty = skip_dev_prefix(tty);

        if (!startswith(tty, "tty") )
                return -EINVAL;

        if (tty[3] < '0' || tty[3] > '9')
                return -EINVAL;

        r = safe_atoi(tty+3, &i);
        if (r < 0)
                return r;

        if (i < 0 || i > 63)
                return -EINVAL;

        return i;
}

Hmmm.

Earnestly commented 4 years ago

Alright, POSIX defines both /dev and /dev/tty so I feel comfortable assuming this. Can you update PR #11 to use tty and tty=${tty#/dev/tty} instead?

Make sure you assign tty=$(tty) first as ${$(tty)#/dev/tty} is a zsh specific feature, e.g.

tty=$(tty)
tty=${tty#/dev/tty}

PS: This is still not ideal but if pam_systemd can assume this, so can I. At least tty works on OpenBSD too unlike ps -o tty= $$.

prez commented 4 years ago

Done.

as ${$(tty)#/dev/tty} is a zsh specific feature

Thanks, I wasn't aware of that. shellcheck ate it up, I should file an issue there I guess.

Earnestly commented 4 years ago

Thanks \o