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 <[email protected]>
Cc: Kay Sievers <[email protected]>
Signed-off-by: Werner Fink <[email protected]>
Signed-off-by: Hannes Reinecke <[email protected]>
---
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
Hi
On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <[email protected]> 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 <[email protected]>
> Cc: Kay Sievers <[email protected]>
> Signed-off-by: Werner Fink <[email protected]>
> Signed-off-by: Hannes Reinecke <[email protected]>
> ---
> 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
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
On 02/05/2014 07:53 AM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <[email protected]> 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 <[email protected]>
>> Cc: Kay Sievers <[email protected]>
>> Signed-off-by: Werner Fink <[email protected]>
>> Signed-off-by: Hannes Reinecke <[email protected]>
>> ---
>> 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
Hi
On Wed, Feb 5, 2014 at 2:53 PM, Peter Hurley <[email protected]> wrote:
> On 02/05/2014 07:53 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <[email protected]> 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 <[email protected]>
>>> Cc: Kay Sievers <[email protected]>
>>> Signed-off-by: Werner Fink <[email protected]>
>>> Signed-off-by: Hannes Reinecke <[email protected]>
>>> ---
>>> 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
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 <[email protected]> wrote:
>> On 02/05/2014 07:53 AM, David Herrmann wrote:
>>>
>>> Hi
>>>
>>> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <[email protected]> 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 <[email protected]>
>>>> Cc: Kay Sievers <[email protected]>
>>>> Signed-off-by: Werner Fink <[email protected]>
>>>> Signed-off-by: Hannes Reinecke <[email protected]>
>>>> ---
>>>> 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?
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 <[email protected]>
> Cc: Kay Sievers <[email protected]>
> Signed-off-by: Werner Fink <[email protected]>
> Signed-off-by: Hannes Reinecke <[email protected]>
> ---
> 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
On 02/05/2014 01:53 PM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 11:11 AM, Hannes Reinecke <[email protected]> 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 <[email protected]>
>> Cc: Kay Sievers <[email protected]>
>> Signed-off-by: Werner Fink <[email protected]>
>> Signed-off-by: Hannes Reinecke <[email protected]>
>> ---
>> 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
[email protected] +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)