Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
12.05.2021 12:59, Daniel P. Berrangé wrote: On Wed, May 12, 2021 at 12:40:03PM +0300, Roman Kagan wrote: On Mon, Apr 19, 2021 at 10:34:49AM +0100, 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 --- 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, Agreed. and we ought to fix this problem by making monitor_cur() work better in the scenario required. Would it make sense instead to resolve the fdstr into actual file descriptor number in the context where monitor_cur() works and makes sense, prior to passing it to the connection thread? Yes, arguably the root problem is caused by the util/qemu-sockets.c code directly referring to the current monitor. This has resultde in the slightly strange scenario where we have two distinct semantics for the 'fd' SocketAddressType @fd: decimal is for file descriptor number, otherwise a file descriptor name. Named file descriptors are permitted in monitor commands, in combination with the 'getfd' command. Decimal file descriptors are permitted at startup or other contexts where no monitor context is active. Now these distinct semantics kinda make sense from the POV of the management application, but we've let the dual semantics propagate all the way down our I/O stack. Folowing your idea, we could have 'socket_address_resolve_monitor_fd' method which takes a 'SocketAddress' and returns a new 'SocketAddress' with the real FD filled in. We then call this method in any codepath which is getting a 'SocketAddress' struct from the monitor. The util/qemu-sockets.c code then only has to think about real FDs, and the monitor_get_fd() call only occurs right at the top level. Reasonable, I'll try this way -- Best regards, Vladimir
Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
On Wed, May 12, 2021 at 12:40:03PM +0300, Roman Kagan wrote: > On Mon, Apr 19, 2021 at 10:34:49AM +0100, 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 > > > --- > > > 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, > > Agreed. > > > and we ought to fix this problem by > > making monitor_cur() work better in the scenario required. > > Would it make sense instead to resolve the fdstr into actual file > descriptor number in the context where monitor_cur() works and makes > sense, prior to passing it to the connection thread? Yes, arguably the root problem is caused by the util/qemu-sockets.c code directly referring to the current monitor. This has resultde in the slightly strange scenario where we have two distinct semantics for the 'fd' SocketAddressType @fd: decimal is for file descriptor number, otherwise a file descriptor name. Named file descriptors are permitted in monitor commands, in combination with the 'getfd' command. Decimal file descriptors are permitted at startup or other contexts where no monitor context is active. Now these distinct semantics kinda make sense from the POV of the management application, but we've let the dual semantics propagate all the way down our I/O stack. Folowing your idea, we could have 'socket_address_resolve_monitor_fd' method which takes a 'SocketAddress' and returns a new 'SocketAddress' with the real FD filled in. We then call this method in any codepath which is getting a 'SocketAddress' struct from the monitor. The util/qemu-sockets.c code then only has to think about real FDs, and the monitor_get_fd() call only occurs right at the top level. 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 :|
Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
On Mon, Apr 19, 2021 at 10:34:49AM +0100, 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 > > --- > > 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, Agreed. > and we ought to fix this problem by > making monitor_cur() work better in the scenario required. Would it make sense instead to resolve the fdstr into actual file descriptor number in the context where monitor_cur() works and makes sense, prior to passing it to the connection thread? Roman.
Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
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 --- 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
Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
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 > --- > 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. 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 :|
[PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly
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
---
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);
/**
* qio_channel_socket_connect_sync:
* @ioc: the socket channel object
@@ -88,6 +105,9 @@ qio_channel_socket_new_fd(int fd,
* will run in the foreground so the caller will not regain
* execution control until the connection is established or
* an error occurs.
+ *
+ * This a wrapper, calling qio_channel_socket_connect_sync_mon()
+ * with @mon=NULL.
*/
int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7d1f813576..cdf6f2413b 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -41,7 +41,7 @@ int unix_listen(const char *path, Error **errp);
int unix_connect(const char *path, Error **errp);
SocketAddress *socket_parse(const char *str, Error **errp);
-int socket_connect(SocketAddress *addr, Error **errp);
+int socket_connect(SocketAddress *addr, Monitor *mon, Error **errp);
int socket_listen(SocketAddress *addr, int num, Error **errp);
void socket_listen_cleanup(int fd, Error **errp);
int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index de259f7eed..9dc42ca29d 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -135,14 +135,15 @@ qio_channel_socket_new_fd(int fd,
}
-int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
-SocketAddress *addr,
-Error **errp)
+int qio_channel_socket_connect_sync_mon(QIOChannelSocket *ioc,
+SocketAddress *addr,
+Monitor *mon,
+Error **errp)
{
int fd;
trace_qio_channel_socket_connect_sync(ioc, addr);
-fd = socket_connect(addr, errp);
+fd = socket_connect(addr, mon, errp);
if (fd < 0) {
trace_qio_channel_socket_connect_fail(ioc);
return -1;
@@ -158,6 +159,14 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
}
+int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
+SocketAddress *addr,
+Error **errp)
+{
+return qio_channel_socket_connect_sync_mon(ioc, addr, NULL, errp);
+}
+
+
static void qio_channel_socket_connect_worker(QIOTask *task,
gpointer opaque)
{
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index 72b9246529..d902ecede7 100644
--- a/tests/unit/test-util-sockets.c
+++ b/tests/unit/test-util-sockets.c
@@ -90,7 +90,7 @@ static void test_socket_fd_pass_name_good(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup(mon_fdname);
-fd = socket_connect(&addr, &error_abort);
+fd = socket_connect(&addr, NULL, &error_abort);
g_assert_cmpint(fd, !=, -1);
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
@@ -122,7 +122,7 @@ static void test_socket_fd_pass_name_bad(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup(mon_fdname);
-fd = socket_connect(&addr, &err);
+fd = socket_connect(&addr, NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -149,7 +149,7 @@ static void test_socket_fd_pass_name_nomon(void)
addr.type = S
