[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class
From: |
Nicolas Saenz Julienne |
Subject: |
Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class |
Date: |
Mon, 28 Feb 2022 20:05:52 +0100 |
User-agent: |
Evolution 3.42.4 (3.42.4-1.fc35) |
Hi Stefan, thanks for the review.
On Thu, 2022-02-24 at 09:48 +0000, Stefan Hajnoczi wrote:
> On Mon, Feb 21, 2022 at 06:08:43PM +0100, Nicolas Saenz Julienne wrote:
> > diff --git a/qom/meson.build b/qom/meson.build
> > index 062a3789d8..c20e5dd1cb 100644
> > --- a/qom/meson.build
> > +++ b/qom/meson.build
> > @@ -4,6 +4,7 @@ qom_ss.add(files(
> > 'object.c',
> > 'object_interfaces.c',
> > 'qom-qobject.c',
> > + '../util/event-loop.c',
>
> This looks strange. I expected util/event-loop.c to be in
> util/meson.build and added to the util_ss SourceSet instead of qom_ss.
>
> What is the reason for this?
Sorry I meant to move it into the qom directory while cleaning up the series
but forgot about it.
That said, I can see how moving 'event-loop-backend' in qom_ss isn't the
cleanest.
So I tried moving it into util_ss, but for some reason nobody is calling
'type_init(even_loop_register_type)'. My guess is there's some compilation
quirk I'm missing.
Any suggestions? I wonder if util_ss is the right spot for 'event-loop-backend'
anyway, but I don't have a better idea.
> > ))
> >
> > qmp_ss.add(files('qom-qmp-cmds.c'))
> > diff --git a/util/event-loop.c b/util/event-loop.c
> > new file mode 100644
> > index 0000000000..f3e50909a0
> > --- /dev/null
> > +++ b/util/event-loop.c
>
> The naming is a little inconsistent. The filename "event-loop.c" does
> match the QOM type or typedef name event-loop-backend/EventLoopBackend.
>
> I suggest calling the source file event-loop-base.c and the QOM type
> "event-loop-base".
Agree.
> > @@ -0,0 +1,142 @@
> > +/*
> > + * QEMU event-loop backend
> > + *
> > + * Copyright (C) 2022 Red Hat Inc
> > + *
> > + * Authors:
> > + * Nicolas Saenz Julienne <nsaenzju@redhat.com>
>
> Most of the code is cut and pasted. It would be nice to carry over the
> authorship information too.
Yes, of course.
> > +struct EventLoopBackend {
> > + Object parent;
> > +
> > + /* AioContext poll parameters */
> > + int64_t poll_max_ns;
> > + int64_t poll_grow;
> > + int64_t poll_shrink;
>
> These parameters do not affect the main loop because it cannot poll. If
> you decide to keep them in the base class, please document that they
> have no effect on the main loop so users aren't confused. I would keep
> them unique to IOThread for now.
OK, I'll keep them unique then.
Thanks!
--
Nicolás Sáenz