[PATCH] tty: Set correct tty name in 'active' sysfs attribute

[PATCH] tty: Set correct tty name in 'active' sysfs attribute

[Hannes Reinecke]

The 'active' sysfs attribute should refer to the currently
active tty devices the console is running on, not the currently
active console.
The console structure doesn't refer to any device in sysfs,
only the tty the console is running on has.
So we need to print out the tty names in 'active', not
the console names.

Cc: Lennart Poettering <lennart@poettering.net>
Cc: Kay Sievers <kay@vrfy.org>
Signed-off-by: Werner Fink <werner@suse.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/tty/tty_io.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c74a00a..17db8ca 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device *dev,
 		if (i >= ARRAY_SIZE(cs))
 			break;
 	}
-	while (i--)
+	while (i--) {
+		const struct tty_driver *driver;
+		const char *name = cs[i]->name;
+		int index = cs[i]->index;
+
+		driver = cs[i]->device(cs[i], &index);
+		if (driver) {
+			index += driver->name_base;
+			name = driver->name;
+		}
 		count += sprintf(buf + count, "%s%d%c",
-				 cs[i]->name, cs[i]->index, i ? ' ':'\n');
+				 name, index, i ? ' ':'\n');
+	}
 	console_unlock();
 
 	return count;
-- 
1.7.12.4

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[David Herrmann]

Hi

On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <hare@suse.de> wrote:
> The 'active' sysfs attribute should refer to the currently
> active tty devices the console is running on, not the currently
> active console.
> The console structure doesn't refer to any device in sysfs,
> only the tty the console is running on has.
> So we need to print out the tty names in 'active', not
> the console names.
>
> Cc: Lennart Poettering <lennart@poettering.net>
> Cc: Kay Sievers <kay@vrfy.org>
> Signed-off-by: Werner Fink <werner@suse.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/tty/tty_io.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index c74a00a..17db8ca 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device *dev,
>                 if (i >= ARRAY_SIZE(cs))
>                         break;
>         }
> -       while (i--)
> +       while (i--) {
> +               const struct tty_driver *driver;
> +               const char *name = cs[i]->name;
> +               int index = cs[i]->index;
> +
> +               driver = cs[i]->device(cs[i], &index);
> +               if (driver) {
> +                       index += driver->name_base;
> +                       name = driver->name;
> +               }
>                 count += sprintf(buf + count, "%s%d%c",
> -                                cs[i]->name, cs[i]->index, i ? ' ':'\n');
> +                                name, index, i ? ' ':'\n');
> +       }

Nice catch and indeed, systemd already relies on these names to be
identical to their char-dev name. Fortunately, VTs and most serial
devices register the console with the same name as the TTY, so we're
fine.
Two minor nitpicks:
1) Could you use tty_line_name() instead of sprintf()? It's in the
same file and avoids duplicating the name_base logic.
2) Does it make sense to print the console-name if ->device() returns
NULL? Seems weird if we print console-names and tty-names in the same
attribute. It's unlikely that it causes problems, but there might be
conflicts.

Thanks
David

>         console_unlock();
>
>         return count;
> --
> 1.7.12.4
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[Peter Hurley]

On 02/05/2014 07:53 AM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <hare@suse.de> wrote:
>> The 'active' sysfs attribute should refer to the currently
>> active tty devices the console is running on, not the currently
>> active console.
>> The console structure doesn't refer to any device in sysfs,
>> only the tty the console is running on has.
>> So we need to print out the tty names in 'active', not
>> the console names.
>>
>> Cc: Lennart Poettering <lennart@poettering.net>
>> Cc: Kay Sievers <kay@vrfy.org>
>> Signed-off-by: Werner Fink <werner@suse.de>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/tty/tty_io.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index c74a00a..17db8ca 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device *dev,
>>                  if (i >= ARRAY_SIZE(cs))
>>                          break;
>>          }
>> -       while (i--)
>> +       while (i--) {
>> +               const struct tty_driver *driver;
>> +               const char *name = cs[i]->name;
>> +               int index = cs[i]->index;
>> +
>> +               driver = cs[i]->device(cs[i], &index);
>> +               if (driver) {
>> +                       index += driver->name_base;
>> +                       name = driver->name;
>> +               }
>>                  count += sprintf(buf + count, "%s%d%c",
>> -                                cs[i]->name, cs[i]->index, i ? ' ':'\n');
>> +                                name, index, i ? ' ':'\n');
>> +       }
>
> Nice catch and indeed, systemd already relies on these names to be
> identical to their char-dev name. Fortunately, VTs and most serial
> devices register the console with the same name as the TTY, so we're
> fine.

What device did this trip over?

Also, this file is not private to systemd. Maybe these changes should
be forked into a different sysfs attribute, "active_devices"?

Regards,
Peter Hurley

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[David Herrmann]

Hi

On Wed, Feb 5, 2014 at 2:53 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 02/05/2014 07:53 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>
>>> The 'active' sysfs attribute should refer to the currently
>>> active tty devices the console is running on, not the currently
>>> active console.
>>> The console structure doesn't refer to any device in sysfs,
>>> only the tty the console is running on has.
>>> So we need to print out the tty names in 'active', not
>>> the console names.
>>>
>>> Cc: Lennart Poettering <lennart@poettering.net>
>>> Cc: Kay Sievers <kay@vrfy.org>
>>> Signed-off-by: Werner Fink <werner@suse.de>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/tty/tty_io.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index c74a00a..17db8ca 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device
>>> *dev,
>>>                  if (i >= ARRAY_SIZE(cs))
>>>                          break;
>>>          }
>>> -       while (i--)
>>> +       while (i--) {
>>> +               const struct tty_driver *driver;
>>> +               const char *name = cs[i]->name;
>>> +               int index = cs[i]->index;
>>> +
>>> +               driver = cs[i]->device(cs[i], &index);
>>> +               if (driver) {
>>> +                       index += driver->name_base;
>>> +                       name = driver->name;
>>> +               }
>>>                  count += sprintf(buf + count, "%s%d%c",
>>> -                                cs[i]->name, cs[i]->index, i ? '
>>> ':'\n');
>>> +                                name, index, i ? ' ':'\n');
>>> +       }
>>
>>
>> Nice catch and indeed, systemd already relies on these names to be
>> identical to their char-dev name. Fortunately, VTs and most serial
>> devices register the console with the same name as the TTY, so we're
>> fine.
>
>
> What device did this trip over?

I haven't seen one so far, but to me it's a coincident, not something
we should rely on.

> Also, this file is not private to systemd. Maybe these changes should
> be forked into a different sysfs attribute, "active_devices"?

What's the use-case to return the name of the console-driver? There is
no way for user-space to read active console-drivers anywhere so I
think returning the TTY makes more sense. We already have working
user-space that can spawn gettys on active consoles via this file. I
am open to change this to "active_devices" as the existing interface
was clearly not designed to return the device-names.

However, given the fact that both matched so far, I think changing the
existing interface to the only user I am aware of is better than
adding a new interface just to keep this unused attribute. But
obviously it's the maintainer's/your decision and you might know
user-space which requires the console-names instead of the tty-names.
So please let us know which way to go as we would like to see a
reliable way to match active consoles to TTY devices for automated
getty-startup.

Thanks
David

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[Peter Hurley]

This patch won't get very far if not addressed to the actual maintainers

[ +cc Greg Kroah-Hartman, Jiri Slaby]

On 02/05/2014 09:05 AM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 2:53 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 02/05/2014 07:53 AM, David Herrmann wrote:
>>>
>>> Hi
>>>
>>> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>
>>>> The 'active' sysfs attribute should refer to the currently
>>>> active tty devices the console is running on, not the currently
>>>> active console.
>>>> The console structure doesn't refer to any device in sysfs,
>>>> only the tty the console is running on has.
>>>> So we need to print out the tty names in 'active', not
>>>> the console names.
>>>>
>>>> Cc: Lennart Poettering <lennart@poettering.net>
>>>> Cc: Kay Sievers <kay@vrfy.org>
>>>> Signed-off-by: Werner Fink <werner@suse.de>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>    drivers/tty/tty_io.c | 14 ++++++++++++--
>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>>> index c74a00a..17db8ca 100644
>>>> --- a/drivers/tty/tty_io.c
>>>> +++ b/drivers/tty/tty_io.c
>>>> @@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device
>>>> *dev,
>>>>                   if (i >= ARRAY_SIZE(cs))
>>>>                           break;
>>>>           }
>>>> -       while (i--)
>>>> +       while (i--) {
>>>> +               const struct tty_driver *driver;
>>>> +               const char *name = cs[i]->name;
>>>> +               int index = cs[i]->index;
>>>> +
>>>> +               driver = cs[i]->device(cs[i], &index);
>>>> +               if (driver) {
>>>> +                       index += driver->name_base;
>>>> +                       name = driver->name;
>>>> +               }
>>>>                   count += sprintf(buf + count, "%s%d%c",
>>>> -                                cs[i]->name, cs[i]->index, i ? '
>>>> ':'\n');
>>>> +                                name, index, i ? ' ':'\n');
>>>> +       }
>>>
>>>
>>> Nice catch and indeed, systemd already relies on these names to be
>>> identical to their char-dev name. Fortunately, VTs and most serial
>>> devices register the console with the same name as the TTY, so we're
>>> fine.
>>
>>
>> What device did this trip over?
>
> I haven't seen one so far, but to me it's a coincident, not something
> we should rely on.
>
>> Also, this file is not private to systemd. Maybe these changes should
>> be forked into a different sysfs attribute, "active_devices"?
>
> What's the use-case to return the name of the console-driver? There is
> no way for user-space to read active console-drivers anywhere so I
> think returning the TTY makes more sense. We already have working
> user-space that can spawn gettys on active consoles via this file. I
> am open to change this to "active_devices" as the existing interface
> was clearly not designed to return the device-names.
>
> However, given the fact that both matched so far, I think changing the
> existing interface to the only user I am aware of is better than
> adding a new interface just to keep this unused attribute. But
> obviously it's the maintainer's/your decision and you might know
> user-space which requires the console-names instead of the tty-names.
> So please let us know which way to go as we would like to see a
> reliable way to match active consoles to TTY devices for automated
> getty-startup.

Sure, I get it. Just wanted to point out the obvious right up front
so that if it turns out there is another userspace dependency and
this gets rewound, possibly across future -stable kernels, the
fallout could get ugly.

Regards,
Peter Hurley

PS - The "active" attribute won't be unused; old systemd's will
still depend on it, yes?

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[Greg KH]

On Wed, Feb 05, 2014 at 11:11:46AM +0100, Hannes Reinecke wrote:
> The 'active' sysfs attribute should refer to the currently
> active tty devices the console is running on, not the currently
> active console.
> The console structure doesn't refer to any device in sysfs,
> only the tty the console is running on has.
> So we need to print out the tty names in 'active', not
> the console names.
> 
> Cc: Lennart Poettering <lennart@poettering.net>
> Cc: Kay Sievers <kay@vrfy.org>
> Signed-off-by: Werner Fink <werner@suse.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/tty/tty_io.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Would have been nice to actually cc: the tty maintainer(s) :(

scripts/get_maintainer.pl is your friend...

greg k-h

Re: [systemd-devel] [PATCH] tty: Set correct tty name in 'active' sysfs attribute

[Hannes Reinecke]

On 02/05/2014 01:53 PM, David Herrmann wrote:
> Hi
> 
> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <hare@suse.de> wrote:
>> The 'active' sysfs attribute should refer to the currently
>> active tty devices the console is running on, not the currently
>> active console.
>> The console structure doesn't refer to any device in sysfs,
>> only the tty the console is running on has.
>> So we need to print out the tty names in 'active', not
>> the console names.
>>
>> Cc: Lennart Poettering <lennart@poettering.net>
>> Cc: Kay Sievers <kay@vrfy.org>
>> Signed-off-by: Werner Fink <werner@suse.de>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/tty/tty_io.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index c74a00a..17db8ca 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -3545,9 +3545,19 @@ static ssize_t show_cons_active(struct device *dev,
>>                 if (i >= ARRAY_SIZE(cs))
>>                         break;
>>         }
>> -       while (i--)
>> +       while (i--) {
>> +               const struct tty_driver *driver;
>> +               const char *name = cs[i]->name;
>> +               int index = cs[i]->index;
>> +
>> +               driver = cs[i]->device(cs[i], &index);
>> +               if (driver) {
>> +                       index += driver->name_base;
>> +                       name = driver->name;
>> +               }
>>                 count += sprintf(buf + count, "%s%d%c",
>> -                                cs[i]->name, cs[i]->index, i ? ' ':'\n');
>> +                                name, index, i ? ' ':'\n');
>> +       }
> 
> Nice catch and indeed, systemd already relies on these names to be
> identical to their char-dev name. Fortunately, VTs and most serial
> devices register the console with the same name as the TTY, so we're
> fine.
> Two minor nitpicks:
> 1) Could you use tty_line_name() instead of sprintf()? It's in the
> same file and avoids duplicating the name_base logic.
Ok. Not that it makes the patch nicer, but hey.

> 2) Does it make sense to print the console-name if ->device() returns
> NULL? Seems weird if we print console-names and tty-names in the same
> attribute. It's unlikely that it causes problems, but there might be
> conflicts.
> 
This is basically a fallback; this is the old behaviour, which still
might be called for when coming across a tty which just has a stub
for the ->device callback.
It's not that the '->device' callback is used that frequently, so I
wouldn't be surprised here.

Meanwhile I've sent a new patch, reviews are welcome there.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)