Page MenuHomeFreeBSD

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

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

Details

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ray accepted this revision.Aug 2 2017, 9:32 AM

Looks good. Thanks.

sys/dev/vt/vt_buf.c
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
emaste added inline comments.Aug 14 2017, 9:10 PM
usr.sbin/vidcontrol/vidcontrol.c
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?

usr.sbin/vidcontrol/vidcontrol.c
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:

void
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);
}

void
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?

emaste added inline comments.Aug 14 2017, 10:42 PM
usr.sbin/vidcontrol/vidcontrol.c
1296–1298 ↗(On Diff #31450)

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

emaste added inline comments.Aug 15 2017, 12:53 PM
sys/dev/vt/vt_core.c
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

emaste added inline comments.Aug 15 2017, 1:13 PM
sys/dev/vt/vt_core.c
2209 ↗(On Diff #31450)

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

sys/dev/vt/vt_core.c
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?

emaste added inline comments.Aug 15 2017, 1:56 PM
sys/dev/vt/vt_core.c
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.

smahadevan_freebsdfoundation.org 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.