Race VT+X11 on -current
ClosedPublic

Authored by hselasky on May 8 2015, 10:06 AM.

Details

Reviewers
emaste

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
hselasky updated this revision to Diff 5273.May 8 2015, 10:06 AM
hselasky retitled this revision from to Race VT+X11 on -current.
hselasky updated this object.
hselasky edited the test plan for this revision. (Show Details)
hselasky set the repository for this revision to rS FreeBSD src repository.
ngie added a subscriber: ngie.May 8 2015, 10:14 AM

So this was the actual bug?

2760 /* Ask holding process to free window and switch to console window */
2761 vt_proc_window_switch(vd->vd_windows[VT_CONSWINDOW]);

Also, if you're going to do this, could you please remove this conditional?

2777 /* Switch back to saved window */
2778 if (vd->vd_savedwindow != NULL)
2779 vt_proc_window_switch(vd->vd_savedwindow);

sys/dev/vt/vt_core.c
456

Is there a better debug message that can be provided for this and the item below, e.g. "Cannot switch vw (is NULL)", etc (otherwise I can't distinguish this error from the other, can I)?.

468

Is there a reason why this is returning 0 and the previous error condition is returning EINVAL?

hselasky added inline comments.May 8 2015, 10:47 AM
sys/dev/vt/vt_core.c
456

I'll update it.

468

It is not a bug to switch the terminal to the same index like before.

hselasky updated this revision to Diff 5274.May 8 2015, 10:53 AM
emaste added a subscriber: emaste.May 8 2015, 1:19 PM
emaste added a comment.May 8 2015, 1:40 PM

I think this is probably fine, but do we still have a case where a switch is in progress -- i.e., curvw->vw_switch_to == vw, but curvw != vw?

Good catch:

Do you want me to add a case for that? Right now that is not triggering a problem.

--HPS

emaste added a comment.May 8 2015, 2:16 PM

Good catch:

Do you want me to add a case for that? Right now that is not triggering a problem.

--HPS

I think it would be a very rare problem in practice, but should be considered. Would if (vw == curvw || vw == curvw->vw_switch_to == vw) be an appropriate test? Or maybe two separate tests so different diagnostics can be used in each.

I see one more case:

if (curvw->vw_switch_to_ != vw && curvw->vw_switch_to != NULL) {

return (EINVAL); /* other switch in progress */

}

I'll compile and test and then commit shortly.

hselasky updated this revision to Diff 5283.May 8 2015, 3:59 PM

Final patch.

emaste accepted this revision.May 8 2015, 4:02 PM
emaste added a reviewer: emaste.

this looks fine to me

This revision is now accepted and ready to land.May 8 2015, 4:02 PM
hselasky closed this revision.Oct 23 2015, 8:47 AM

Committed as r282645.