Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Sep 22, 2017 at 11:38:31AM +0200, Paolo Bonzini wrote:
> On 22/09/2017 11:36, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
> >> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> >>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >>>> IOThread is a general framework that contains IO loop environment and a
> >>>> real thread behind.  It's also good to be used internally inside qemu.
> >>>> Provide some helpers for it to create iothreads to be used internally.
> >>>>
> >>>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>> ---
> >>>>  include/sysemu/iothread.h |  8 ++++++++
> >>>>  iothread.c                | 21 +++++++++++++++++++++
> >>>>  2 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >>>> index d2985b3..b07663f 100644
> >>>> --- a/include/sysemu/iothread.h
> >>>> +++ b/include/sysemu/iothread.h
> >>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> >>>>  void iothread_stop_all(void);
> >>>>  GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>>>  
> >>>> +/*
> >>>> + * Helpers used to allocate iothreads for internal use.  These
> >>>> + * iothreads will not be seen by monitor clients when query using
> >>>> + * "query-iothreads".
> >>>> + */
> >>>> +IOThread *iothread_create(const char *id, Error **errp);
> >>>> +void iothread_destroy(IOThread *iothread);
> >>>> +
> >>>>  #endif /* IOTHREAD_H */
> >>>> diff --git a/iothread.c b/iothread.c
> >>>> index 44c8944..74e400c 100644
> >>>> --- a/iothread.c
> >>>> +++ b/iothread.c
> >>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >>>>  
> >>>>      return iothread->worker_context;
> >>>>  }
> >>>> +
> >>>> +static Object *iothread_get_internal_parent(void)
> >>>> +{
> >>>> +    return container_get(object_get_root(), "/internal-iothreads");
> >>>> +}
> >>>
> >>> I tend to think we might benefit from having this generalized in the
> >>> QOM API instead. We have object_get_objects_root() for things that
> >>> are created by the mgmt app / user via CLI / QMP.  A parallel method
> >>> object_get_internal_root() could be useful for cases like this where
> >>> we want to create user-creatable objects, but not have them be
> >>> visible to the mgmt app / user, as that would confuse the mgmt app.
> >>>
> >>> Example for this scenario - libvirt calls query-iothreads to identify
> >>> IO thread PIDs, and would get very unhappy if the IOThread used by
> >>> the monitor would appear in that response, which is why Peter has
> >>> put it under /internal-iothreads. I think this scenario will apply
> >>> more broadly, so benefit from us having a general helper in QOM.
> >>
> >> Yeah, I can split the patch if we want it to be exposed to public.
> >>
> >> In that case, would the name "object_get_internal_root" be good?
> > 
> > Yeah that's fine with me, but lets see if Paolo has any thoughts from
> > the QOM side.
> 
> No, I don't.  However, I wonder if query-iothreads should have an
> argument to include internal iothreads.

I guess it depends whether we consider use of iothreads for migration
to be a private impl detail that may be ripped out & replaced at any
time, or a semi-public detail that apps may rely on.  Personally I
would suggest it should be a private impl detail of migration code
that is never exposed to anything outside QEMU, so we have flexibility
to change it at will later.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|