Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly

[Vladimir Sementsov-Ogievskiy]

19.04.2021 12:34, Daniel P. Berrangé wrote:
> On Fri, Apr 16, 2021 at 11:08:59AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Detecting monitor by current coroutine works bad when we are not in
> > coroutine context. And that's exactly so in nbd reconnect code, where
> > qio_channel_socket_connect_sync() is called from thread.
> >
> > Add a possibility to pass monitor by hand, to be used in the following
> > commit.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   include/io/channel-socket.h    | 20 ++++++++++++++++++++
> >   include/qemu/sockets.h         |  2 +-
> >   io/channel-socket.c            | 17 +++++++++++++----
> >   tests/unit/test-util-sockets.c | 16 ++++++++--------
> >   util/qemu-sockets.c            | 10 +++++-----
> >   5 files changed, 47 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > index e747e63514..6d0915420d 100644
> > --- a/include/io/channel-socket.h
> > +++ b/include/io/channel-socket.h
> > @@ -78,6 +78,23 @@ qio_channel_socket_new_fd(int fd,
> >                             Error **errp);
> >   
> >   
> > +/**
> > + * qio_channel_socket_connect_sync_mon:
> > + * @ioc: the socket channel object
> > + * @addr: the address to connect to
> > + * @mon: current monitor. If NULL, it will be detected by
> > + *       current coroutine.
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Attempt to connect to the address @addr. This method
> > + * will run in the foreground so the caller will not regain
> > + * execution control until the connection is established or
> > + * an error occurs.
> > + */
> > +int qio_channel_socket_connect_sync_mon(QIOChannelSocket *ioc,
> > +                                        SocketAddress *addr,
> > +                                        Monitor *mon,
> > +                                        Error **errp);
>
> I don't really like exposing the concept of the QEMU monitor in
> the IO layer APIs. IMHO these ought to remain completely separate
> subsystems from the API pov, and we ought to fix this problem by
> making monitor_cur() work better in the scenario required.

Hmm..

I can add thread_mon hash-table to monitor/monitor.c, so we can set monitor for 
current thread in same way like for coroutine. And monitor_cur will look first at 
coroutine->monitor hash map and then to thread->monitor. Then, I'll pass needed 
monitor link to my specific thread, and thread will call 
monitor_set_cur_for_thread(), an then qio_channel_socket_connect_sync() will work 
correctly.

David, Markus, is it OK?

--
Best regards,
Vladimir