[pve-devel] [PATCH container 1/2] setup getty: ensure the correct services are enabled

[Fabian Grünbichler]

On Thu, Jul 18, 2019 at 10:45:43AM +0200, Thomas Lamprecht wrote:
> On 7/18/19 10:23 AM, Fabian Grünbichler wrote:
> > On Wed, Jul 17, 2019 at 12:08:24PM +0200, Thomas Lamprecht wrote:
> > 
> > kind of meta, but these two subs (setup_systemd_console and
> > setup_container_getty) are very very similar now, maybe it would make
> > sense to just move the console unit content setting into its own sub,
> > and unify the rest where only the unit names differ (e.g., by passing
> > that in as parameter, or deciding it based on ostype)? alternatively,
> > just the iteration and some of the duplicate constants could be pulled
> > out into private helpers/vars.
> 
> yes can make sense, note also that "setup_systemd_console" is only used by
> SUSE, and that is a bit broken in general, i.e., the network seup works
> not fully correct (their docs and wicked are quite lacking, IMO), I'm not
> too sure, but it could make sense to drop suse support completely...
> (i.e., note now that it's depreacated and remove with 7.0 if the situation
> did not get better and nobody stepped up to do some work needed there)
> 
> 
> >> +
> >> +	# ensure that not two gettys are using the same tty!
> >> +	$self->ct_unlink("$getty_target_fn/getty\@$i.service");
> > 
> > here you unlink a different service instance (tty$i vs $i) than gets
> > linked in setup_systemd_console - is this intentional? doesn't this then
> > mean we actually have three different combos we have to take a look at?
> 
> No it should have been "tty$i", just a copy paste error of mine, thanks
> for noticing.
> > 
> > maybe a more complete solution would be to remove all getty instances
> > first, and then setup $ttycount ones?
> 
> Hmm, do you think at a situation where more than 7 (our max tty count)
> are present in the CT? Else we already do this, albeit a bit implicitly
> (or better said in a single pass).

no, I was thinking IFF there are multiple naming schemes in the wild, it
might just be easier to make a clean slate and populate it from there
rather than attempting to match all of them. if only getty at tty$i and
container-getty@$i exist, the current approach is fine.

> 
> > 
> >> +
> >> +	if ($i > $ttycount) {
> >> +	    $self->ct_unlink($tty_service_lnk);
> >> +	} else {
> >> +	    if (!$self->ct_is_symlink($tty_service_lnk)) {
> > 
> > this means masked units don't get enabled - intentionally?
> > 
> 
> I intentionally copied the hunk we had in setup_systemd_console, yes,
> but I also had the feeling that the situation with "masking" was not
> really thought out when that got added there.
> 
> IMO, it does not make sense, this just produces strange situations,
> mask from tty's > tty count get removed and the ones from configured
> (and thus wanted ones) not.. will re do for a v2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel