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

Move MAX_DISPLAYS to a configuration option, fix a quirk or bug in MAX_DISPLAYS #41

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

Conversation

mzpqnxow
Copy link

  • Fixed what appeared to be a bug or a quirk in how MAX_DISPLAYS is handled

When MAX_DISPLAYS is 1000 and X11DisplayOffset is 10, only 990 displays
were actually attempted because the X11DisplayOffset wasn't taken
into consideration. I didn't notice any clear documentation on how this
is supposed to work since it's an internal variable.

  • Moved MAX_DISPLAYS to a default value and added MaxDisplays

Added a new MaxDisplays configuration option that override MAX_DISPLAYS

This is very useful in environments where OpenSSH is used as a
gateway for a large number of users, as in the environment where
I manage OpenSSH. The current hard limit of 1000 is not enough
in some cases, and is not configurable.

…andled

When MAX_DISPLAYS is 1000 and X11DisplayOffset is 10, only 990 displays
were actually attempted because the X11DisplayOffset wasn't taken
into consideration. I didn't notice any clear documentation on how this
is supposed to work since it's an internal variable.

- Moved MAX_DISPLAYS to a default value and added MaxDisplays

Added a new MaxDisplays configuration option that override MAX_DISPLAYS

This is very useful in environments where OpenSSH is used as a
gateway for a large number of users, as in the environment where
I manage OpenSSH. The current hard limit of 1000 is not enough
in some cases, and is not configurable.
@Jakuje
Copy link
Contributor

Jakuje commented Jun 2, 2016

This seems reasonable feature, but there are few comments to the code:

  • The option should certainly have some documentation, basically item in manual page for sshd_config should be enough. Also note about the option in manual page for sshd is a good practice. If we will accept this in Match blocks, then it should be mentioned also there.
  • The constant MAX_DISPLAYS should probably be DEFAULT_MAX_DISPLAYS for consistency.
  • I would leave option parsing on goto parse_int; as in different cases. Using a2port seems confusing.
  • There is used M_CP_INTOPT(max_displays);, but the option is not marked as available in Match blocks (SSHCFG_GLOBAL flag). This needs also to clear up.
  • I would consider prefixing the option with X11_, as the other X11 related options are
  • There is also some whitespace noise in the changed lines, that would be nice to clean up.

Please, consider the notes and address the issues above. OpenSSH team does not accept contributions on github, so when we will have patch ready, it would be good to fill upstream bug on https://bugzilla.mindrot.org/

@mzpqnxow
Copy link
Author

mzpqnxow commented Jun 2, 2016

Thanks for the notes, very helpful. I will make the suggested changes and additions and then resubmit to the mindrot mailing list for the OpenSSH team to review.

- renamed to X11MaxDisplays / x11_max_displays
- fixed formatting
- moved sMaxX11Displays to SSHCFG_ALL alongside X11DisplayOffset
- added X11MaxDisplays to sshd_config man page
@mzpqnxow
Copy link
Author

mzpqnxow commented Jun 4, 2016

Thanks for the input Jakuje, I've fixed the issues you mentioned up and added documentation. I will attach the patch to https://bugzilla.mindrot.org/show_bug.cgi?id=2580

restyled-io bot referenced this pull request in johnsonjh/j-hpn-ssh Feb 23, 2021
Added contributing section to README.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants