Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Marc-André Lureau]

Hi

On Tue, Aug 28, 2018 at 12:20 PM Peter Xu <address@hidden> wrote:
>
> On Tue, Aug 28, 2018 at 11:09:28AM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Aug 28, 2018 at 5:52 AM Peter Xu <address@hidden> wrote:
> > >
> > > On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> > > > terminal3270 is uses the front-end side of the chardev. It shouldn't
> > > > create sources from backend side context. Fwiw, send_timing_mark_cb
> > > > calls qemu_chr_fe_write_all() which should be thread safe.
> > > >
> > > > This partially reverts changes from commit
> > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > >
> > > I don't fully understand too on why "It shouldn't create sources from
> > > backend side context".  Could you explain a bit?
> >
> > A frontend shouldn't use a backend function, and doesn't have to use
> > the backend gmaincontext.
> >
> > >
> > > If you don't want the backend gcontext to be exposed to the frontend,
> > > IMHO the simplest thing is just to use the NULL gcontext in
> > > qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
> > > much point on re-using the GSource tags since IMHO GSource pointer is
> > > always superior to the old tag system.
> >
> > Using the tag or the GSource * doesn't change much afaik.
>
> I did some convertions in the past year to convert tag -> GSource and
> that shows some differences when we want non-default gcontexts...
> Otherwise I won't bother. :)

It doesn't need a specific context here, does it?

>
> > Not only
> > this is not a hot path, but glib gmain code does create the tag on
> > attach anyway. You may be marginally faster on remove/detach by
> > avoiding one lookup.
> >
> > (btw, I think both tag and GSource* are as old)
>
> I am not strong on this, but my rule is that if I don't have explicit
> reason to change a code I keep it there.  Here I don't see an obvious
> reason to switch back to the tag things (not to mention that I still
> think GSource is better, always).

As I explained, you shouldn't use backend fonctions in the frontend,
and there is no reason to use the backend context. I try to keep all
the sources created around chardev under control, and this one is
incorrect imho.

Using g_timeout_add() is convenient, since it does all the work you do
manually by creating a source, setting callback, attaching, unref etc.
And it uses the default context, which is in general the right thing.

I can do all the manual work and keep the GSource* around, but I don't
think that helps in any way here.


>
> Thanks,
>
> --
> Peter Xu



-- 
Marc-André Lureau