[v2,0/2] vt: bracketed paste and cursor position

A different kind of VT console update this time. These patches:

- add bracketed paste support to the VT console

- overcome a /dev/vcsa limitation with cursor position retrieval

In v1 those were submitted together to avoid merge conflicts. Those conflicts
no longer exist but here they are together again for coherence sake.

Changes from v1 (https://lore.kernel.org/all/20250514015554.19978-1-nico@fluxnic.net):

- Changed TIOCL_GETCURSORPOS to VT_GETCONSIZECSRPOS with proper structure.
  Moved to the VT_ space to benefit from unambigous pointer argument and
  vt_compat_ioctl() wrapper. Also motivated by the fact that usage require
  both display size and cursor position so those are joined in one syscall
  requiring a structure.

- Code simplifications suggested by Jiri.

diffstat:
 drivers/tty/vt/selection.c     | 31 +++++++++++++++++++++++++++----
 drivers/tty/vt/vt.c            | 15 +++++++++++++++
 drivers/tty/vt/vt_ioctl.c      | 16 ++++++++++++++++
 include/linux/console_struct.h |  1 +
 include/uapi/linux/tiocl.h     |  1 +
 include/uapi/linux/vt.h        |  9 +++++++++
 6 files changed, 69 insertions(+), 4 deletions(-)

On Thu, 15 May 2025, Jiri Slaby wrote:

> On 14. 05. 25, 21:42, Nicolas Pitre wrote:
> > From: Nicolas Pitre <npitre@baylibre.com>
> > 
> > The console dimension and cursor position are available through the
> > /dev/vcsa interface already. However the /dev/vcsa header format uses
> > single-byte fields therefore those values are clamped to 255.
> > 
> > As surprizing as this may seem, some people do use 240-column 67-row
> > screens (a 1920x1080 monitor with 8x16 pixel fonts) which is getting
> > close to the limit. Monitors with higher resolution are not uncommon
> > these days (3840x2160 producing a 480x135 character display) and it is
> > just a matter of time before someone with, say, a braille display using
> > the Linux VT console and BRLTTY on such a screen reports a bug about
> > missing and oddly misaligned screen content.
> > 
> > Let's add VT_GETCONSIZECSRPOS for the retrieval of console size and cursor
> > position without byte-sized limitations. The actual console size limit as
> > encoded in vt.c is 32767x32767 so using a short here is appropriate. Then
> > this can be used to get the cursor position when /dev/vcsa reports 255.
> > 
> > The screen dimension may already be obtained using TIOCGWINSZ and adding
> > the same information to VT_GETCONSIZECSRPOS might be redundant. However
> > applications that care about cursor position also care about display
> > size and having 2 separate system calls to obtain them separately is
> > wasteful. Also, the cursor position can be queried by writing "\e[6n" to
> > a tty and reading back the result but that may be done only by the actual
> > application using that tty and not a sideline observer.
> > 
> > Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> > ---
> >   drivers/tty/vt/vt_ioctl.c | 16 ++++++++++++++++
> >   include/uapi/linux/vt.h   |  9 +++++++++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index 4b91072f3a4e..83a3d49535e5 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -951,6 +951,22 @@ int vt_ioctl(struct tty_struct *tty,
> >    				(unsigned short __user *)arg);
> >    case VT_WAITEVENT:
> >   		return vt_event_wait_ioctl((struct vt_event __user *)arg);
> > +
> > +	case VT_GETCONSIZECSRPOS:
> > +	{
> > +		struct vt_consizecsrpos concsr;
> > +
> > +		console_lock();
> > +		concsr.con_cols = vc->vc_cols;
> > +		concsr.con_rows = vc->vc_rows;
> > +		concsr.csr_col = vc->state.x;
> > +		concsr.csr_row = vc->state.y;
> > +		console_unlock();
> 
> Makes a lot of sense!
> 
> > +		if (copy_to_user(up, &concsr, sizeof(concsr)))
> > +			return -EFAULT;
> > +		return 0;
> > +	}
> > +
> >    default:
> >    	return -ENOIOCTLCMD;
> >   	}
> > diff --git a/include/uapi/linux/vt.h b/include/uapi/linux/vt.h
> > index e9d39c48520a..e93c8910133b 100644
> > --- a/include/uapi/linux/vt.h
> > +++ b/include/uapi/linux/vt.h
> > @@ -84,4 +84,13 @@ struct vt_setactivate {
> >   
> >   #define VT_SETACTIVATE	0x560F	/* Activate and set the mode of a
> >   console */
> >   
> > +struct vt_consizecsrpos {
> > +	unsigned short con_rows;	/* number of console rows */
> > +	unsigned short con_cols;	/* number of console columns */
> > +	unsigned short csr_row;		/* current cursor's row */
> > +	unsigned short csr_col;		/* current cursor's column */
> 
> Use __u16 pls.

I beg to differ. Not because __u16 is fundamentally wrong. But 
everything else in this file uses only basic C types already and adding 
one struct with __u16 would look odd. And adding some include to define 
that type would be needed since there are currently no such includes in 
that file currently, and that could potentially cause issues with 
existing consumers of that header file that didn't expect extra 
definitions, etc. So I think that such a change, if it is to happen, 
should be done for the whole file at once and in a separate patch.

> > +};
> > +
> > +#define VT_GETCONSIZECSRPOS 0x5610  /* get console size and cursor position
> > */
> 
> Can we define that properly as
>   _IOR(0x56, 0x10, struct vt_consizecsrpos)
> ? Note this would still differ from "conflicting":
> #define VIDIOC_G_FBUF            _IOR('V', 10, struct v4l2_framebuffer)

Similarly as the reason above: given that no other definitions in that 
file use the _IO*() scheme for historical reasons, it is preferable to 
follow what's already there to avoid unsuspected confusion. The VT layer 
is pretty much unlykely to grow many additional ioctls in the 
foreseeable future so I'd lean towards keeping things simple and in line 
with the existing code.


Nicolas

On Fri, 16 May 2025, Jiri Slaby wrote:

> On 15. 05. 25, 18:02, Nicolas Pitre wrote:
> > On Thu, 15 May 2025, Jiri Slaby wrote:
> > 
> >> On 14. 05. 25, 21:42, Nicolas Pitre wrote:
> >>> From: Nicolas Pitre <npitre@baylibre.com>
> >>>
> >>> The console dimension and cursor position are available through the
> >>> /dev/vcsa interface already. However the /dev/vcsa header format uses
> >>> single-byte fields therefore those values are clamped to 255.
> >>>
> >>> As surprizing as this may seem, some people do use 240-column 67-row
> >>> screens (a 1920x1080 monitor with 8x16 pixel fonts) which is getting
> >>> close to the limit. Monitors with higher resolution are not uncommon
> >>> these days (3840x2160 producing a 480x135 character display) and it is
> >>> just a matter of time before someone with, say, a braille display using
> >>> the Linux VT console and BRLTTY on such a screen reports a bug about
> >>> missing and oddly misaligned screen content.
> >>>
> >>> Let's add VT_GETCONSIZECSRPOS for the retrieval of console size and cursor
> >>> position without byte-sized limitations. The actual console size limit as
> >>> encoded in vt.c is 32767x32767 so using a short here is appropriate. Then
> >>> this can be used to get the cursor position when /dev/vcsa reports 255.
> >>>
> >>> The screen dimension may already be obtained using TIOCGWINSZ and adding
> >>> the same information to VT_GETCONSIZECSRPOS might be redundant. However
> >>> applications that care about cursor position also care about display
> >>> size and having 2 separate system calls to obtain them separately is
> >>> wasteful. Also, the cursor position can be queried by writing "\e[6n" to
> >>> a tty and reading back the result but that may be done only by the actual
> >>> application using that tty and not a sideline observer.
> >>>
> >>> Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> >>> ---
> >>>    drivers/tty/vt/vt_ioctl.c | 16 ++++++++++++++++
> >>>    include/uapi/linux/vt.h   |  9 +++++++++
> >>>    2 files changed, 25 insertions(+)
> >>>
> >>> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> >>> index 4b91072f3a4e..83a3d49535e5 100644
> >>> --- a/drivers/tty/vt/vt_ioctl.c
> >>> +++ b/drivers/tty/vt/vt_ioctl.c
> >>> @@ -951,6 +951,22 @@ int vt_ioctl(struct tty_struct *tty,
> >>>     				(unsigned short __user *)arg);
> >>>     case VT_WAITEVENT:
> >>>    		return vt_event_wait_ioctl((struct vt_event __user *)arg);
> >>> +
> >>> +	case VT_GETCONSIZECSRPOS:
> >>> +	{
> >>> +		struct vt_consizecsrpos concsr;
> >>> +
> >>> +		console_lock();
> >>> +		concsr.con_cols = vc->vc_cols;
> >>> +		concsr.con_rows = vc->vc_rows;
> >>> +		concsr.csr_col = vc->state.x;
> >>> +		concsr.csr_row = vc->state.y;
> >>> +		console_unlock();
> >>
> >> Makes a lot of sense!
> >>
> >>> +		if (copy_to_user(up, &concsr, sizeof(concsr)))
> >>> +			return -EFAULT;
> >>> +		return 0;
> >>> +	}
> >>> +
> >>>     default:
> >>>     	return -ENOIOCTLCMD;
> >>>    	}
> >>> diff --git a/include/uapi/linux/vt.h b/include/uapi/linux/vt.h
> >>> index e9d39c48520a..e93c8910133b 100644
> >>> --- a/include/uapi/linux/vt.h
> >>> +++ b/include/uapi/linux/vt.h
> >>> @@ -84,4 +84,13 @@ struct vt_setactivate {
> >>>    
> >>>    #define VT_SETACTIVATE	0x560F	/* Activate and set the mode of a
> >>>    console */
> >>>    
> >>> +struct vt_consizecsrpos {
> >>> +	unsigned short con_rows;	/* number of console rows */
> >>> +	unsigned short con_cols;	/* number of console columns */
> >>> +	unsigned short csr_row;		/* current cursor's row */
> >>> +	unsigned short csr_col;		/* current cursor's column */
> >>
> >> Use __u16 pls.
> > 
> > I beg to differ. Not because __u16 is fundamentally wrong.
> 
> Fundamentaly wrong -- for what reason? These types are exactly what should be
> used in userspace APIs instead of bare C types.

Sorry, I meant "__u16 is not wrong fundamentally but..."

> > But
> > everything else in this file uses only basic C types already and adding
> > one struct with __u16 would look odd.
> 
> The whole file needs to be fixed, eventually. It's no reason to add another
> bad entry.

Nothing is actually _broken_ now, is it?

> > And adding some include to define
> > that type would be needed since there are currently no such includes in
> > that file currently, and that could potentially cause issues with
> > existing consumers of that header file that didn't expect extra
> > definitions, etc.
> 
> On one side yes, on the other side, sticking with ancient coding style while
> being afraid to include basic headers would be moot.
> 
> > So I think that such a change, if it is to happen,
> > should be done for the whole file at once and in a separate patch.
> 
> Let me bite the bullet and send something. (Likely on Mon -- now queued up in
> my queue for build tests).
> 
> >>> +};
> >>> +
> >>> +#define VT_GETCONSIZECSRPOS 0x5610  /* get console size and cursor
> >>> position
> >>> */
> >>
> >> Can we define that properly as
> >>    _IOR(0x56, 0x10, struct vt_consizecsrpos)
> >> ? Note this would still differ from "conflicting":
> >> #define VIDIOC_G_FBUF            _IOR('V', 10, struct v4l2_framebuffer)
> > 
> > Similarly as the reason above: given that no other definitions in that
> > file use the _IO*() scheme for historical reasons, it is preferable to
> > follow what's already there to avoid unsuspected confusion. The VT layer
> > is pretty much unlykely to grow many additional ioctls in the
> > foreseeable future so I'd lean towards keeping things simple and in line
> > with the existing code.
> 
> I tend to disagree. We should not follow bad practices only because they are
> already there.

Sometimes it is better to leave things as they are, especially when 
they've been static and stable for so long, as disturbing stuff in the 
name of removing "bad practices" may bring unforeseen consequences. 
Perfect is the enemy of good 
(https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good).


Nicolas