Message ID | 20171222134527.14467-7-berrange@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi On Fri, Dec 22, 2017 at 2:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote: > 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. That looks reasonable to me. If in the future we get rid of cur_mon somehow, the code should be fairly simple to adapt, but I don't think we need to worry about it now, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > qapi/sockets.json | 7 ++++ > tests/test-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 > ## > { 'enum': 'SocketAddressType', > diff --git a/tests/test-sockets.c b/tests/test-sockets.c > index 8a2962fe7b..8307e4774f 100644 > --- a/tests/test-sockets.c > +++ b/tests/test-sockets.c > @@ -49,7 +49,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; > @@ -80,7 +80,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; > @@ -110,6 +110,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; > @@ -129,10 +221,18 @@ int main(int argc, char **argv) > } > > if (has_ipv4) { > - 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 6a7b194428..8ac4eed122 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1029,9 +1029,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); > -- > 2.14.3 > >
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-sockets.c b/tests/test-sockets.c index 8a2962fe7b..8307e4774f 100644 --- a/tests/test-sockets.c +++ b/tests/test-sockets.c @@ -49,7 +49,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; @@ -80,7 +80,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; @@ -110,6 +110,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; @@ -129,10 +221,18 @@ int main(int argc, char **argv) } if (has_ipv4) { - 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 6a7b194428..8ac4eed122 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1029,9 +1029,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);
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. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- qapi/sockets.json | 7 ++++ tests/test-sockets.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++--- util/qemu-sockets.c | 16 ++++++-- 3 files changed, 126 insertions(+), 9 deletions(-)