| Message ID | 20180205152455.12088-8-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable passing pre-opened chardev socket FD | expand |
On 02/05/2018 09:24 AM, Daniel P. Berrangé wrote: > From: "Daniel P. Berrange" <berrange@redhat.com> > > The SocketAddress 'fd' kind accepts the name of a file descriptor passed > to the monitor with the 'getfd' command. This makes it impossible to use > the 'fd' kind in cases where a monitor is not available. This can apply in > handling command line argv at startup, or simply if internal code wants to > use SocketAddress and pass a numeric FD it has acquired from elsewhere. > > Fortunately the 'getfd' command mandated that the FD names must not start > with a leading digit. We can thus safely extend semantics of the > SocketAddress 'fd' kind, to allow a purely numeric name to reference an > file descriptor that QEMU already has open. There will be restrictions on > when each kind can be used. > > In codepaths where we are handling a monitor command (ie cur_mon != NULL), > we will only support use of named file descriptors as before. Use of FD > numbers is still not permitted for monitor commands. > > In codepaths where we are not handling a monitor command (ie cur_mon == > NULL), we will not support named file descriptors. Instead we can reference > FD numers explicitly. This allows the app spawning QEMU to intentionally > "leak" a pre-opened socket to QEMU and reference that in a SocketAddress > definition, or for code inside QEMU to pass pre-opened FDs around. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > qapi/sockets.json | 7 +++ > tests/test-util-sockets.c | 112 +++++++++++++++++++++++++++++++++++++++++++--- > util/qemu-sockets.c | 16 +++++-- > 3 files changed, 126 insertions(+), 9 deletions(-) > > diff --git a/qapi/sockets.json b/qapi/sockets.json > index ac022c6ad0..fc81d8d5e8 100644 > --- a/qapi/sockets.json > +++ b/qapi/sockets.json > @@ -123,6 +123,13 @@ > # > # @unix: Unix domain socket > # > +# @vsock: VMCI address > +# > +# @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. > +# > # Since: 2.9 There doesn't seem to be any way to introspect if we support decimal fds from the command line; is that going to be a problem?
On Mon, Feb 05, 2018 at 01:42:50PM -0600, Eric Blake wrote: > On 02/05/2018 09:24 AM, Daniel P. Berrangé wrote: > > From: "Daniel P. Berrange" <berrange@redhat.com> > > > > The SocketAddress 'fd' kind accepts the name of a file descriptor passed > > to the monitor with the 'getfd' command. This makes it impossible to use > > the 'fd' kind in cases where a monitor is not available. This can apply in > > handling command line argv at startup, or simply if internal code wants to > > use SocketAddress and pass a numeric FD it has acquired from elsewhere. > > > > Fortunately the 'getfd' command mandated that the FD names must not start > > with a leading digit. We can thus safely extend semantics of the > > SocketAddress 'fd' kind, to allow a purely numeric name to reference an > > file descriptor that QEMU already has open. There will be restrictions on > > when each kind can be used. > > > > In codepaths where we are handling a monitor command (ie cur_mon != NULL), > > we will only support use of named file descriptors as before. Use of FD > > numbers is still not permitted for monitor commands. > > > > In codepaths where we are not handling a monitor command (ie cur_mon == > > NULL), we will not support named file descriptors. Instead we can reference > > FD numers explicitly. This allows the app spawning QEMU to intentionally > > "leak" a pre-opened socket to QEMU and reference that in a SocketAddress > > definition, or for code inside QEMU to pass pre-opened FDs around. > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > qapi/sockets.json | 7 +++ > > tests/test-util-sockets.c | 112 +++++++++++++++++++++++++++++++++++++++++++--- > > util/qemu-sockets.c | 16 +++++-- > > 3 files changed, 126 insertions(+), 9 deletions(-) > > > > diff --git a/qapi/sockets.json b/qapi/sockets.json > > index ac022c6ad0..fc81d8d5e8 100644 > > --- a/qapi/sockets.json > > +++ b/qapi/sockets.json > > @@ -123,6 +123,13 @@ > > # > > # @unix: Unix domain socket > > # > > +# @vsock: VMCI address > > +# > > +# @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. > > +# > > # Since: 2.9 > > There doesn't seem to be any way to introspect if we support decimal fds > from the command line; is that going to be a problem? Libvirt needs to know when it can use it, so any suggestions ? Regards, Daniel
On 02/06/2018 03:13 AM, Daniel P. Berrangé wrote: >>> +++ b/qapi/sockets.json >>> @@ -123,6 +123,13 @@ >>> # >>> # @unix: Unix domain socket >>> # >>> +# @vsock: VMCI address >>> +# >>> +# @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. >>> +# >>> # Since: 2.9 >> >> There doesn't seem to be any way to introspect if we support decimal fds >> from the command line; is that going to be a problem? > > Libvirt needs to know when it can use it, so any suggestions ? Patch 9/9 modified qemu_chardev_opts; does that change reflect through query-command-line-options? Not all QemuOpts changes are introspectible yet, and Markus has been trying to tackle that, but if this particular one works, we can use that as our witness (after all, if I understand correctly, the new feature you are adding here is NOT affecting the QMP usage, but is an enhancement for command-line usage).
On Tue, Feb 06, 2018 at 08:48:43AM -0600, Eric Blake wrote: > On 02/06/2018 03:13 AM, Daniel P. Berrangé wrote: > > > > > +++ b/qapi/sockets.json > > > > @@ -123,6 +123,13 @@ > > > > # > > > > # @unix: Unix domain socket > > > > # > > > > +# @vsock: VMCI address > > > > +# > > > > +# @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. > > > > +# > > > > # Since: 2.9 > > > > > > There doesn't seem to be any way to introspect if we support decimal fds > > > from the command line; is that going to be a problem? > > > > Libvirt needs to know when it can use it, so any suggestions ? > > Patch 9/9 modified qemu_chardev_opts; does that change reflect through > query-command-line-options? Not all QemuOpts changes are introspectible > yet, and Markus has been trying to tackle that, but if this particular one > works, we can use that as our witness (after all, if I understand correctly, > the new feature you are adding here is NOT affecting the QMP usage, but is > an enhancement for command-line usage). Yeah, 'fd' does appear there, so that's good enough for now. Regards, Daniel
diff --git a/qapi/sockets.json b/qapi/sockets.json index ac022c6ad0..fc81d8d5e8 100644 --- a/qapi/sockets.json +++ b/qapi/sockets.json @@ -123,6 +123,13 @@ # # @unix: Unix domain socket # +# @vsock: VMCI address +# +# @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. +# # Since: 2.9 ## { 'enum': 'SocketAddressType', diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 06eb0e4a28..acadd85e8f 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -73,7 +73,7 @@ Monitor *cur_mon; void monitor_init(Chardev *chr, int flags) {} -static void test_socket_fd_pass_good(void) +static void test_socket_fd_pass_name_good(void) { SocketAddress addr; int fd; @@ -104,7 +104,7 @@ static void test_socket_fd_pass_good(void) cur_mon = NULL; } -static void test_socket_fd_pass_bad(void) +static void test_socket_fd_pass_name_bad(void) { SocketAddress addr; Error *err = NULL; @@ -134,6 +134,98 @@ static void test_socket_fd_pass_bad(void) cur_mon = NULL; } +static void test_socket_fd_pass_name_nomon(void) +{ + SocketAddress addr; + Error *err = NULL; + int fd; + + g_assert(cur_mon == NULL); + + addr.type = SOCKET_ADDRESS_TYPE_FD; + addr.u.fd.str = g_strdup("myfd"); + + fd = socket_connect(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + fd = socket_listen(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + g_free(addr.u.fd.str); +} + + +static void test_socket_fd_pass_num_good(void) +{ + SocketAddress addr; + int fd, sfd; + + g_assert(cur_mon == NULL); + sfd = qemu_socket(AF_INET, SOCK_STREAM, 0); + g_assert_cmpint(sfd, >, STDERR_FILENO); + + addr.type = SOCKET_ADDRESS_TYPE_FD; + addr.u.fd.str = g_strdup_printf("%d", sfd); + + fd = socket_connect(&addr, &error_abort); + g_assert_cmpint(fd, ==, sfd); + + fd = socket_listen(&addr, &error_abort); + g_assert_cmpint(fd, ==, sfd); + + g_free(addr.u.fd.str); + close(sfd); +} + +static void test_socket_fd_pass_num_bad(void) +{ + SocketAddress addr; + Error *err = NULL; + int fd, sfd; + + g_assert(cur_mon == NULL); + sfd = dup(STDOUT_FILENO); + + addr.type = SOCKET_ADDRESS_TYPE_FD; + addr.u.fd.str = g_strdup_printf("%d", sfd); + + fd = socket_connect(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + fd = socket_listen(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + g_free(addr.u.fd.str); + close(sfd); +} + +static void test_socket_fd_pass_num_nocli(void) +{ + SocketAddress addr; + Error *err = NULL; + int fd; + + cur_mon = g_malloc(1); /* Fake a monitor */ + + addr.type = SOCKET_ADDRESS_TYPE_FD; + addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO); + + fd = socket_connect(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + fd = socket_listen(&addr, &err); + g_assert_cmpint(fd, ==, -1); + error_free_or_abort(&err); + + g_free(addr.u.fd.str); +} + + int main(int argc, char **argv) { bool has_ipv4, has_ipv6; @@ -156,10 +248,18 @@ int main(int argc, char **argv) test_fd_is_socket_bad); g_test_add_func("/util/socket/is-socket/good", test_fd_is_socket_good); - g_test_add_func("/socket/fd-pass/good", - test_socket_fd_pass_good); - g_test_add_func("/socket/fd-pass/bad", - test_socket_fd_pass_bad); + g_test_add_func("/socket/fd-pass/name/good", + test_socket_fd_pass_name_good); + g_test_add_func("/socket/fd-pass/name/bad", + test_socket_fd_pass_name_bad); + g_test_add_func("/socket/fd-pass/name/nomon", + test_socket_fd_pass_name_nomon); + g_test_add_func("/socket/fd-pass/num/good", + test_socket_fd_pass_num_good); + g_test_add_func("/socket/fd-pass/num/bad", + test_socket_fd_pass_num_bad); + g_test_add_func("/socket/fd-pass/num/nocli", + test_socket_fd_pass_num_nocli); } return g_test_run(); diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index e75b2b1f57..5aba1484bc 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1008,9 +1008,19 @@ fail: static int socket_get_fd(const char *fdstr, Error **errp) { - int fd = monitor_get_fd(cur_mon, fdstr, errp); - if (fd < 0) { - return -1; + int fd; + if (cur_mon) { + fd = monitor_get_fd(cur_mon, fdstr, errp); + if (fd < 0) { + return -1; + } + } else { + if (qemu_strtoi(fdstr, NULL, 10, &fd) < 0) { + error_setg_errno(errp, errno, + "Unable to parse FD number %s", + fdstr); + return -1; + } } if (!fd_is_socket(fd)) { error_setg(errp, "File descriptor '%s' is not a socket", fdstr);