Re: [Qemu-devel] chardev's and fd's in monitors

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Tue, Oct 18, 2016 at 02:52:13PM +0100, Dr. David Alan Gilbert wrote:
>> * Daniel P. Berrange (address@hidden) wrote:
>> > On Tue, Oct 18, 2016 at 02:25:25PM +0100, Dr. David Alan Gilbert wrote:
>> > > * Daniel P. Berrange (address@hidden) wrote:
>> > > > On Tue, Oct 18, 2016 at 12:32:02PM +0100, Dr. David Alan Gilbert wrote:
>> > > > > * Daniel P. Berrange (address@hidden) wrote:
>> > > > > > On Wed, Oct 12, 2016 at 08:15:02PM +0100, Dr. David Alan Gilbert 
>> > > > > > wrote:
>> > > > > > > Hi,
>> > > > > > >   I had a look at a couple of readline like libraries;
>> > > > > > > editline and linenoise.  A difficulty with using them is that
>> > > > > > > they both want fd's or FILE*'s; editline takes either but
>> > > > > > > from a brief look I think it's expecting to extract the fd.
>> > > > > > > That makes them tricky to integrate into qemu, where
>> > > > > > > the chardev's hide a whole bunch of non-fd things; in particular
>> > > > > > > tls, mux, ringbuffers etc.
>> > > > > > > 
>> > > > > > > If we could get away with just a FILE* then we could use 
>> > > > > > > fopencookie,
>> > > > > > > but that's GNU only.
>> > > > > > > 
>> > > > > > > Is there any sane way of shepherding all chardev's into having an
>> > > > > > > fd?
>> > > > > > 
>> > > > > > The entire chardev abstraction model exists precisely because we 
>> > > > > > cannot
>> > > > > > make all chardevs look like a single fd. Even those which are fd 
>> > > > > > based
>> > > > > > may have separate FDs for input and output.
>> > > > > 
>> > > > > Note that editline takes separate in/out streams, but it does want 
>> > > > > those streams
>> > > > > to be FILE*'s.
>> > > > > 
>> > > > > > IMHO the only viable approach would be to enhance 
>> > > > > > linenoise/editline to
>> > > > > > not assume use of fd* or FILE * abstractions.
>> > > > > 
>> > > > > I think if it came to that then we'd probably end up sticking with 
>> > > > > what we
>> > > > > had for a very long time; I'd assume it would take a long time before
>> > > > > any mods we made to the libraries would come around to be generally 
>> > > > > useful.
>> > > > > 
>> > > > > > BTW, what is the actual thread issue you are facing ? Chardevs at 
>> > > > > > least
>> > > > > > ought to be usable from a separate thread, as long as each distinct
>> > > > > > chardev object instance was only used from one thread at a time ?
>> > > > > 
>> > > > > Marc-André pointed that out; I hadn't realised they were thread safe.
>> > > > > But what are the rules? You say 'only used from one thread at a 
>> > > > > time' -
>> > > > > what happens if we have a mux and the different streams to the mux 
>> > > > > come
>> > > > > from different threads?
>> > > > 
>> > > > Well there is no mutex locking on the CharDriverState objects, so the
>> > > > exact rule is "you mustn't do anything from multiple threads that will
>> > > > race on contents of CharDriverState". That's too fuzzy to be useful to
>> > > > developers though, so I think the only sensible option right now is to
>> > > > say any "top level" CharDriverState should only be touch from one 
>> > > > thread
>> > > > at a time. IOW, if you have a mux, that that rule would apply to the
>> > > > mux itself and the various children it owns as if they were a single
>> > > > unnit.
>> > > 
>> > > OK; I think we're probably saved by the big lock at the moment, so that
>> > > all device emulation that outputs text is probably holding it and the 
>> > > monitor
>> > > is also.  What about something like an error_report from a different 
>> > > thread
>> > > while something is happening in the monitor?
>> > 
>> > If we moved execution of monitor commands to separate thread from the
>> > thread handling monitor I/O, then we'd have to modify error_report so
>> > that it queued the text in some manner, such that it was only then
>> > fed back to the client once the command thread completed. Alternatively
>> > we'd have to introduced locking in the Monitor object, that serialized
>> > access to the underling CharDriverState I/O funcs.
>> 
>> I already use error_report's in places in migration threads of various
>> types; I'm not sure if that's a problem.
>
> Unless those places are protected by the big qemu lock, that sounds
> not good. error_report calls into error_vprintf which checks the
> 'cur_mon' global "Monitor" pointer. This variable is updated at
> runtime - eg in qmp_human_monitor_command(), monitor_qmp_read(),
> monitor_read(), etc. So if migration threads outside the BQL are
> calling error_report() that could well cause problems. If you
> are lucky messages will merely end up going to stderr instead of
> the monitor, but in worst case I wouldn't be surprised if there
> is a crash possibility in some race conditions.

cur_mon dates back to single-threaded times.

The idea is to print to the monitor when running within an HMP command,
else to stderr.

The current solution is to set cur_mon around monitor commands.  Fine
with a single thread, not fine at all with multiple threads.

Making cur_mon thread-local should fix things.

If you do want to report errors from another thread in a monitor, you
should use error_setg() & friends to get them into the monitor, in my
opinion.  Asynchronously barfing output to a monitor doesn't strike me
as a sensible design.  Not least because it doesn't work at all with
QMP!  If an error message is important enough for the human monitor's
user to make use route it to the human monitor, why is hiding it from
the QMP client okay?

If I'm wrong and it is sensible, we need locking.