[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 05/10] qmp: 'add_client' actually expects sockets
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 05/10] qmp: 'add_client' actually expects sockets |
Date: |
Tue, 14 Feb 2023 14:25:39 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
> actually expect a socket kind or will fail in different ways at runtime.
>
> Throw an error early if the given 'add_client' fd is not a socket, and
> close it to avoid leaks.
>
> This allows to replace the close() call with a more correct & portable
> closesocket() version.
>
> (this will allow importing sockets on Windows with a specialized command
> in the following patch, while keeping the remaining monitor associated
> sockets/add_client code & usage untouched)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> monitor/qmp-cmds.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 859012aef4..2dae6bb10f 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -14,6 +14,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/sockets.h"
> #include "monitor-internal.h"
> #include "monitor/qdev.h"
> #include "monitor/qmp-helpers.h"
> @@ -139,11 +140,17 @@ void qmp_add_client(const char *protocol, const char
> *fdname,
> return;
> }
>
> + if (!fd_is_socket(fd)) {
> + error_setg(errp, "add_client expects a socket");
> + close(fd);
> + return;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(protocol_table); i++) {
> if (!strcmp(protocol, protocol_table[i].name)) {
> if (!protocol_table[i].add_client(fd, has_skipauth, skipauth,
> has_tls, tls, errp)) {
> - close(fd);
> + closesocket(fd);
> }
> return;
> }
> @@ -151,7 +158,7 @@ void qmp_add_client(const char *protocol, const char
> *fdname,
>
> if (!qmp_add_client_char(fd, has_skipauth, skipauth, has_tls, tls,
> protocol, errp)) {
> - close(fd);
> + closesocket(fd);
> }
> }
Please update add_client's doc comment in qapi/misc.json to state
explicitly that a socket is required.
- [PATCH v3 00/10] Teach 'getfd' QMP command to import win32 sockets, marcandre . lureau, 2023/02/07
- [PATCH v3 01/10] tests: fix path separator, use g_build_filename(), marcandre . lureau, 2023/02/07
- [PATCH v3 02/10] char: do not double-close fd when failing to add client, marcandre . lureau, 2023/02/07
- [PATCH v3 03/10] tests/docker: fix a win32 error due to portability, marcandre . lureau, 2023/02/07
- [PATCH v3 04/10] osdep: implement qemu_socketpair() for win32, marcandre . lureau, 2023/02/07
- [PATCH v3 05/10] qmp: 'add_client' actually expects sockets, marcandre . lureau, 2023/02/07
- Re: [PATCH v3 05/10] qmp: 'add_client' actually expects sockets,
Markus Armbruster <=
- [PATCH v3 06/10] monitor: release the lock before calling close(), marcandre . lureau, 2023/02/07
[PATCH v3 07/10] qapi: implement conditional command arguments, marcandre . lureau, 2023/02/07