Hi On Mon, Feb 11, 2019 at 11:27 AM Peter Xu <pet...@redhat.com> wrote: > > On Mon, Feb 11, 2019 at 10:57:01AM +0100, Marc-André Lureau wrote: > > Hi > > > > On Mon, Feb 11, 2019 at 8:13 AM Peter Xu <pet...@redhat.com> wrote: > > > > > > On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote: > > > > On Wed, 6 Feb 2019 18:43:25 +0100 > > > > Marc-André Lureau <marcandre.lur...@redhat.com> wrote: > > > > > > > > > terminal3270 uses the front-end side of the chardev. It shouldn't > > > > > create sources from backend side context (with backend > > > > > functions). > > > > > > > > > > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be > > > > > thread safe. > > > > > > > > > > This partially reverts changes from commit > > > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304. > > > > > > > > > > CC: Peter Xu <pet...@redhat.com> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > > --- > > > > > hw/char/terminal3270.c | 15 ++++++--------- > > > > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > > > > > I've verified that 3270 continues to work as before. > > > > > > > > Acked-by: Cornelia Huck <coh...@redhat.com> > > > > > > A pure question: is it broken before this patch? > > > > No > > > > > > > > Asked since I don't understand this patch and what it tries to fix... > > > > A front-end shouldn't use backend functions. > > > > > E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing > > > NULL as chardev context so AFAICT this patch changes nothing from > > > functional-wise for now. Meanwhile, if we pass in some non-NULL into > > > it in the future, IMHO this patch would break it instead of fixing > > > anything... anything I've missed? > > > > > > > If the frontend switches the context and creates timers with this > > context, it should do it without using the backend functions. > > I see your point (assuming that concurrent writes are safe). But IMHO > we should first discuss on how to fix the existing multi-threading > issues (since it's not really safe yet). After all this patch offers
Yes, various people are looking at this problem. We need to fix it. > nothing real for us for now, and if one day we want to run the chardev > in a single thread again then this will just break again unnoticed. > I assume you are not speaking about that patch in particular, when you say it will "break again unnoticed". This patch helps when you do whole-tree code review or change over a particular aspect of the backend code. In my case, I wrote this patch because I did a review of all the backend context users. And this one is a "bad" user, I think you agree by now. So let's apply this change. And let's focus on multi-thread bugs in respective threads. thanks -- Marc-André Lureau
