Re: the big term patch

[Roland McGrath]

This stuff all looks great.  As usual, Marcus, you rule.

It would be ideal to separate the commits into: bottomhalf interface
changes, using argp, and adding the hurdio bottomhalf.  But if you don't
want to bother then I don't really care.  But please do clean up the log
entries not to make reference to intermediate states and changes from
intermediate states along the way, just describe the the changes from the
existing code to the code you are checking in with each batch described by
a log entry.

I haven't really scoured the code in detail, but if it works cleanly for
you we can get it in now and change nits later as I notice them in reading
over the code.

Something I realized is that before your changes the CIGNORE behavior was
totally wrong.  When CIGNORE is given in a set_state call (TIOCSETA*), the
c_cflag and c_*speed fields should not be used at all, and the bottomhalf
set_bits hook should never even be called.  This is consistent with the BSD
implementation (Linux does not have CIGNORE).  The existing Hurd behavior
of clobbering termstate is wrong.

In general, we should check for anything like this in the code.  For
anything that can have an error, the global state should not be tweaked
before the bottomhalf call is attempted, and the bottomhalf call should
have an argument for the new state being put in.  Only in the call succeeds
should term's state variables be updated.  Maybe you have covered this already.

Adding the init hook is a good thing.

Off-hand nits:

Please always use "type\nfunction (...)" instead of "type function".
devio_init can be static.

mdmstate should return error_t and take an int * argument to fill in.  The
underlying access can fail and then it should not necessarily just return
0.  e.g. for hurdio all things that boil down to tioctl calls ought to
ignore them if they return EOPNOTSUPP/MIG_BAD_ID/ENOSYS but propagate other
errors to their callers (EIO or whatever it might be).

It never hurts to do bcopy -> memcpy while you're there if you feel like it.

Use -p to diff.

For get_bottom_type, let's just add an enum term_bottom_type field to
bottomhalf.

The struct bottomhalfs ought to be declared const.

At some point I would like to move the bottomhalf's and the rest of term
away from using global variables.  The termflags et al should move into a
state structure that the trivfs hook will point to.  For the bottomhalf's,
we could have either a union or an allocated void * in there for their
private data, and have the state pointer passed in all the hooks.  (All of
this is just like storeio's struct dev.)  As well as plain style, this gets
us closer to being able to use fsys_delegate for multiple (root-owned)
terminals to share a process for the term translator.

It might be good to have a stub bottomhalf (similar to the "unknown" store
type I added), where every hook is just a no-op or returns EIO.  It's not
good for much, but it's handy for testing sometimes and you can give it an
option where it prints out for every hook call or something.  Putting that
underneath the hurdio bottomhalf is a good way to test that every termios
operation is passing down in the proper fashion.  (Also I figure if we did
implement fsys_delegate, then /servers/term would be a term node with a
stub bottomhalf so the only thing it's good for is stat or delegate.)

You talked about using FIFOs and that seems pretty good for debugging
purposes (sorry it didn't occur to me in the first place you wouldn't have
been using two of them or I would have forewarned you on that).  But we
can't necessarily always trust pflocal when suspicious things are
happening, and it made me think that it would be most useful to write a
simple translator to be your own test harness.  That is, a trivfs
filesystem where io_read just iterates through a list of canned test data
to reply with.  Or you could make it like a modified streamio but rather
than a device-reading thread you have a thread driven by a list of delays
and characters to deliver (if you get that fancy, you can have it read the
test script from a file and make it flexible what loads you drive it with).
But the real benefit over just FIFOs or a plain file for testing purposes
is that you can implement the various tioctl calls to work or fail in
interesting ways that you want to test from above.

If you are on random feature patrol, seems like we might as well give term
the --readonly/--writeonly options (and --writable and all aliases)
streamio has.  I don't know why you'd want to use them in term, but then I
don't know off hand why you'd want them in streamio either. :)
I guess we might as well have --rdev too.