On Fri, 6 Mar 2026 at 14:49, Daniel P. Berrangé <[email protected]> wrote: > > On Fri, Mar 06, 2026 at 01:57:45PM +0000, Peter Maydell wrote: > > On Wed, 4 Mar 2026 at 10:34, Daniel P. Berrangé <[email protected]> wrote: > > > It is subtle, as the QIOTask object deals with 2 separate callbacks. > > > > > > The callback that is provided to qio_task_new() is what is invoked when > > > the task is marked as completed. This is guaranteed to always be > > > invoked in main event loop context. > > > > > > The callback that is provided to qio_task_run_in_thread() is what runs > > > in the throwaway background thread. When this callback returns, the > > > QIOTask creates a glib idle source in the main event loop to run the > > > completion callback. > > > > > > > > > So wrt char-socket.c, tcp_chr_connect_client_task will run in the > > > background thread, but it only calls QIOChannel APIs so is safe. > > > > > > The qemu_chr_socket_connected method will run in main event loop > > > context, so it does not need locking wrt other chardev I/O > > > callbacks. > > > > This is starting to sound like the problem is the test code. The > > test code is calling qemu_chr_new() and qemu_chr_fe_set_handlers() > > from a random thread it created by directly calling g_thread_new(). > > So these calls from this thread do not have any kind of lock and > > can run in parallel with the qemu_chr_socket_connected method from > > the main event loop. > > > > I guess we need to take a lock here, but which one? > > > > > > How is this intended to work ? Is the test case code incorrect to > > > > assume > > > > it can work with the chardev it got back from qemu_chr_new(), or should > > > > the chardev be handling this case? > > > > > > The test is really rather had to understand, but I feel like something > > > in the test ought to be looking at the CHR_EVENT_OPENED event to > > > identify when the connection is actually established ? > > > > The way to get a CHR_EVENT_OPENED would be to set the frontend > > handlers with qemu_chr_fe_set_handlers(). It's the call to set > > the frontend handlers that is already going wrong. (But the test > > doesn't really need to care about the EVENT_OPENED: it can > > just wait til it gets the "you have some data to read" callback.) > > I'm rather feeling that the test program would be better off not > using chardevs at all, and instead using QIOChannelSocket directly. > The chardevs are horribly complex when trying to deal with > connection oriented semantics, as they are really not a connection > based API; the event stuff is a hacky afterthought that barely > gets us by.
Perhaps. But that sounds like a lot more work than "identify what lock one is supposed to be holding to validly call the qemu_chr* functions and make sure we hold it in test_server_connect()" :-) -- PMM
