Page MenuHomeFreeBSD

Implement "vidcontrol -h <history_size>" for vt(4)

Authored by on Aug 1 2017, 10:10 PM.


Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Looks good. Thanks.

553 ↗(On Diff #31450)

Idea to use function, to have way offload it to hardware.
We still don't support hw offload, but maybe :)

This revision is now accepted and ready to land.Aug 2 2017, 9:32 AM
1296–1298 ↗(On Diff #31450)

The error string certainly didn't agree with the previous size < 0 test, but is vidcontrol -h 0 actually not permissible -- is it not possible to specify no history?

1296–1298 ↗(On Diff #31450)

It is technically possible to specify no history, but it will have the same effect as running vidcontrol -h <x> for all x <= screen_height. From sys/dev/vt/vt_buf.c:

vtbuf_sethistory_size(struct vt_buf *vb, unsigned int size)
	term_pos_t p;

	/* With same size */
	p.tp_row = vb->vb_scr_size.tp_row;
	p.tp_col = vb->vb_scr_size.tp_col;
	vtbuf_grow(vb, &p, size);

vtbuf_grow(struct vt_buf *vb, const term_pos_t *p, unsigned int history_size)
	term_char_t *old, *new, **rows, **oldrows, **copyrows, *row, *oldrow;
	unsigned int w, h, c, r, old_history_size;
	size_t bufsize, rowssize;
	int history_full;

	history_size = MAX(history_size, p->tp_row);

Since history_size will be at least screen_height large, should it be permissible to specify a history size of 0 or be consistent with the previous error string?

1296–1298 ↗(On Diff #31450)

Hmm, good point, although it might be useful to allow -h 0 to mean minimum history size.

2209 ↗(On Diff #31450)

ah, further to my comment on vidcontrol.c for vidcontrol -h 0 to mean minimal history we'd need to make this < 0

2209 ↗(On Diff #31450)

Also we'll want some maximum on the history size in order to prevent excessive memory allocation for the buffer.

2209 ↗(On Diff #31450)

syscons(4) enforces the maximum on the history size and returns EINVAL if the check fails, so it would make sense to do a similar check in vt(4) rather than one in the vidcontrol(1) tool right?

2209 ↗(On Diff #31450)

Yes, the check should be in the kernel; if it were only in vidcontrol someone could build a vidcontrol(1) without the check. We might want to return ERANGE instead though, it seems to be a better fit and would allow a better error message to be returned to the user.

(This may not be the exact intended use of ERANGE according to POSIX but for FreeBSD-specific ioctls we have some freedom to use errnos as we see fit.)

This test can be combined with the existing by just casting to an unsigned int, removing the need to test for < 0. edited edge metadata.
  • Add a check for the maximum history size. This maximum is set to the same value as the maximum in syscons(4).
  • Fix the error string on a negative number argument
This revision now requires review to proceed.Aug 15 2017, 4:25 PM

In my wipbsd-12 branch as 8838eabfacb87fbf3781d9a5a79de88ffb859215

This revision was automatically updated to reflect the committed changes.