| Message ID | 20230207142535.1153722-9-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Teach 'getfd' QMP command to import win32 sockets | expand |
On 7/2/23 15:25, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > A process with enough capabilities can duplicate a socket to QEMU. > Modify 'getfd' to import it and add it to the monitor fd list, so it can > be later used by other commands. > > Note that we actually store the SOCKET in the FD list, appropriate care > must now be taken to use the correct socket functions (similar approach > is taken by our io/ code and in glib, this is internal and shouldn't > affect the QEMU/QMP users) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/misc.json | 16 ++++++++-- > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > monitor/hmp-cmds.c | 6 +++- > 3 files changed, 81 insertions(+), 20 deletions(-) > +void qmp_getfd(const char *fdname, > +#ifdef WIN32 > + const char *wsa_info, Rename as 'optional_b64_context' and remove #ifdef'ry? Preferrably change qmp_getfd() prototype and use close_fd() in a preliminary patch. Otherwise LGTM. > +#endif > + Error **errp) > +{ > + Monitor *cur_mon = monitor_cur(); > + int fd; > + > +#ifdef WIN32 > + if (wsa_info) { > + g_autofree WSAPROTOCOL_INFOW *info = NULL; > + gsize len; > + SOCKET sk; > + > + info = (void *)g_base64_decode(wsa_info, &len); > + if (len != sizeof(*info)) { > + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value"); > + return; > + } > + > + sk = WSASocketW(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, > + FROM_PROTOCOL_INFO, info, 0, 0); > + if (sk == INVALID_SOCKET) { > + g_autofree gchar *emsg = g_win32_error_message(WSAGetLastError()); > + error_setg(errp, "Couldn't create socket: %s", emsg); > + return; > + } > + > + return monitor_add_fd(cur_mon, sk, fdname, errp); > + } > +#endif > + > + fd = qemu_chr_fe_get_msgfd(&cur_mon->chr); > + if (fd == -1) { > + error_setg(errp, "No file descriptor supplied via SCM_RIGHTS"); > + return; > + } > + > + return monitor_add_fd(cur_mon, fd, fdname, errp); > }
On Tue, Feb 07, 2023 at 06:25:33PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > A process with enough capabilities can duplicate a socket to QEMU. > Modify 'getfd' to import it and add it to the monitor fd list, so it can > be later used by other commands. > > Note that we actually store the SOCKET in the FD list, appropriate care > must now be taken to use the correct socket functions (similar approach > is taken by our io/ code and in glib, this is internal and shouldn't > affect the QEMU/QMP users) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/misc.json | 16 ++++++++-- > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > monitor/hmp-cmds.c | 6 +++- > 3 files changed, 81 insertions(+), 20 deletions(-) > > diff --git a/qapi/misc.json b/qapi/misc.json > index 27ef5a2b20..cd36d8befb 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -249,10 +249,18 @@ > ## > # @getfd: > # > -# Receive a file descriptor via SCM rights and assign it a name > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. > +# > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is > +# considered as a kind of "file descriptor" in QMP context, for historical > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. > # > # @fdname: file descriptor name > # > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. This is a clever trick, but it also feels pretty gross from POV of QMP design normal practice, which would be to define a struct in QAPI to represent the WSAPROTOCOL_INFOW contents. The main downside would be that its more verbose to convert between the windows and QAPI structs. > @@ -270,7 +278,11 @@ > # <- { "return": {} } > # > ## > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } > +{ 'command': 'getfd', 'data': { > + 'fdname': 'str', > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} > + } > +} snip > +void qmp_getfd(const char *fdname, > +#ifdef WIN32 > + const char *wsa_info, > +#endif > + Error **errp) > +{ > + Monitor *cur_mon = monitor_cur(); > + int fd; > + > +#ifdef WIN32 > + if (wsa_info) { > + g_autofree WSAPROTOCOL_INFOW *info = NULL; > + gsize len; > + SOCKET sk; > + > + info = (void *)g_base64_decode(wsa_info, &len); > + if (len != sizeof(*info)) { > + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value"); > + return; > + } With regards, Daniel
Hi On Tue, Feb 7, 2023 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Feb 07, 2023 at 06:25:33PM +0400, marcandre.lureau@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > A process with enough capabilities can duplicate a socket to QEMU. > > Modify 'getfd' to import it and add it to the monitor fd list, so it can > > be later used by other commands. > > > > Note that we actually store the SOCKET in the FD list, appropriate care > > must now be taken to use the correct socket functions (similar approach > > is taken by our io/ code and in glib, this is internal and shouldn't > > affect the QEMU/QMP users) > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qapi/misc.json | 16 ++++++++-- > > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > > monitor/hmp-cmds.c | 6 +++- > > 3 files changed, 81 insertions(+), 20 deletions(-) > > > > diff --git a/qapi/misc.json b/qapi/misc.json > > index 27ef5a2b20..cd36d8befb 100644 > > --- a/qapi/misc.json > > +++ b/qapi/misc.json > > @@ -249,10 +249,18 @@ > > ## > > # @getfd: > > # > > -# Receive a file descriptor via SCM rights and assign it a name > > +# On UNIX, receive a file descriptor via SCM rights and assign it a > name. > > +# > > +# On Windows, (where ancillary socket fd-passing isn't an option yet), > add a > > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() > via > > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A > SOCKET is > > +# considered as a kind of "file descriptor" in QMP context, for > historical > > +# reasons and simplicity. QEMU takes care to use socket functions > appropriately. > > # > > # @fdname: file descriptor name > > # > > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since > 8.0. > > This is a clever trick, but it also feels pretty gross from > POV of QMP design normal practice, which would be to define > a struct in QAPI to represent the WSAPROTOCOL_INFOW contents. > > The main downside would be that its more verbose to convert > between the windows and QAPI structs. WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved files, it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays, GUID, and utf16 string. QAPI-fying is going to be painful for no real gain. It is opaque and simply given back to WSASocketW. Markus, did you have a chance to look at the series? Can you review/comment before I do further work? thanks > > @@ -270,7 +278,11 @@ > > # <- { "return": {} } > > # > > ## > > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } > > +{ 'command': 'getfd', 'data': { > > + 'fdname': 'str', > > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} > > + } > > +} > > snip > > > +void qmp_getfd(const char *fdname, > > +#ifdef WIN32 > > + const char *wsa_info, > > +#endif > > + Error **errp) > > +{ > > + Monitor *cur_mon = monitor_cur(); > > + int fd; > > + > > +#ifdef WIN32 > > + if (wsa_info) { > > + g_autofree WSAPROTOCOL_INFOW *info = NULL; > > + gsize len; > > + SOCKET sk; > > + > > + info = (void *)g_base64_decode(wsa_info, &len); > > + if (len != sizeof(*info)) { > > + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value"); > > + return; > > + } > > > With 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 :| > >
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > A process with enough capabilities can duplicate a socket to QEMU. > Modify 'getfd' to import it and add it to the monitor fd list, so it can > be later used by other commands. > > Note that we actually store the SOCKET in the FD list, appropriate care > must now be taken to use the correct socket functions (similar approach > is taken by our io/ code and in glib, this is internal and shouldn't > affect the QEMU/QMP users) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/misc.json | 16 ++++++++-- > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > monitor/hmp-cmds.c | 6 +++- > 3 files changed, 81 insertions(+), 20 deletions(-) > > diff --git a/qapi/misc.json b/qapi/misc.json > index 27ef5a2b20..cd36d8befb 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -249,10 +249,18 @@ > ## > # @getfd: > # > -# Receive a file descriptor via SCM rights and assign it a name > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. > +# > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is > +# considered as a kind of "file descriptor" in QMP context, for historical > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. The Windows part explains things in terms of the C socket API. Less than ideal for the QEMU QMP Reference Manual, isn't it? I don't know nearly enough about this stuff to suggest concrete improvements... What does this command do under Windows before this patch? Fail always? Wrap your lines a bit earlier, please. > # > # @fdname: file descriptor name > # > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. > +# No way around passing a binary blob? > # Returns: Nothing on success > # > # Since: 0.14 > @@ -270,7 +278,11 @@ > # <- { "return": {} } > # > ## > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } > +{ 'command': 'getfd', 'data': { > + 'fdname': 'str', > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} > + } > +} What happens when QEMU runs on a Windows host and the client doesn't pass @wsa-info? > > ## > # @closefd:
Hi Markus On Fri, Feb 17, 2023 at 1:49 PM Markus Armbruster <armbru@redhat.com> wrote: > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > A process with enough capabilities can duplicate a socket to QEMU. > > Modify 'getfd' to import it and add it to the monitor fd list, so it can > > be later used by other commands. > > > > Note that we actually store the SOCKET in the FD list, appropriate care > > must now be taken to use the correct socket functions (similar approach > > is taken by our io/ code and in glib, this is internal and shouldn't > > affect the QEMU/QMP users) > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qapi/misc.json | 16 ++++++++-- > > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > > monitor/hmp-cmds.c | 6 +++- > > 3 files changed, 81 insertions(+), 20 deletions(-) > > > > diff --git a/qapi/misc.json b/qapi/misc.json > > index 27ef5a2b20..cd36d8befb 100644 > > --- a/qapi/misc.json > > +++ b/qapi/misc.json > > @@ -249,10 +249,18 @@ > > ## > > # @getfd: > > # > > -# Receive a file descriptor via SCM rights and assign it a name > > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. > > +# > > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a > > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via > > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is > > +# considered as a kind of "file descriptor" in QMP context, for historical > > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. > > The Windows part explains things in terms of the C socket API. Less > than ideal for the QEMU QMP Reference Manual, isn't it? I don't know > nearly enough about this stuff to suggest concrete improvements... We don't have to, after all we don't explain how to use sendmsg/cmsg stuff to pass FDs. I will drop the part about "A SOCKET is considered as a kind of "file descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not mix SOCKET and fd space" (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/) merged. > > What does this command do under Windows before this patch? Fail always? Without ancillary data support on Windows, you can't make it work. > > Wrap your lines a bit earlier, please. > > > # > > # @fdname: file descriptor name > > # > > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. > > +# > > No way around passing a binary blob? WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields, it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays, GUID, and utf16 string. QAPI'fying that structure back and forth would be tedious and error-prone. Better to treat it as an opaque blob imho. > > > # Returns: Nothing on success > > # > > # Since: 0.14 > > @@ -270,7 +278,11 @@ > > # <- { "return": {} } > > # > > ## > > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } > > +{ 'command': 'getfd', 'data': { > > + 'fdname': 'str', > > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} > > + } > > +} > > What happens when QEMU runs on a Windows host and the client doesn't > pass @wsa-info? It attempts to get the fd from the last recv, but it will fail on Windows, this is not available.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi Markus > > On Fri, Feb 17, 2023 at 1:49 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > A process with enough capabilities can duplicate a socket to QEMU. >> > Modify 'getfd' to import it and add it to the monitor fd list, so it can >> > be later used by other commands. >> > >> > Note that we actually store the SOCKET in the FD list, appropriate care >> > must now be taken to use the correct socket functions (similar approach >> > is taken by our io/ code and in glib, this is internal and shouldn't >> > affect the QEMU/QMP users) >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > --- >> > qapi/misc.json | 16 ++++++++-- >> > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- >> > monitor/hmp-cmds.c | 6 +++- >> > 3 files changed, 81 insertions(+), 20 deletions(-) >> > >> > diff --git a/qapi/misc.json b/qapi/misc.json >> > index 27ef5a2b20..cd36d8befb 100644 >> > --- a/qapi/misc.json >> > +++ b/qapi/misc.json >> > @@ -249,10 +249,18 @@ >> > ## >> > # @getfd: >> > # >> > -# Receive a file descriptor via SCM rights and assign it a name >> > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. >> > +# >> > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a >> > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via >> > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is >> > +# considered as a kind of "file descriptor" in QMP context, for historical >> > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. >> >> The Windows part explains things in terms of the C socket API. Less >> than ideal for the QEMU QMP Reference Manual, isn't it? I don't know >> nearly enough about this stuff to suggest concrete improvements... > > We don't have to, after all we don't explain how to use sendmsg/cmsg > stuff to pass FDs. > > I will drop the part about "A SOCKET is considered as a kind of "file > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not > mix SOCKET and fd space" > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/) > merged. Would it make sense to rebase this series on top of that one, so we can have simpler documentation from the start? >> What does this command do under Windows before this patch? Fail always? > > Without ancillary data support on Windows, you can't make it work. Yes, but how does it fail? Hmm, you actually answer that below. >> Wrap your lines a bit earlier, please. >> >> > # >> > # @fdname: file descriptor name >> > # >> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. >> > +# >> >> No way around passing a binary blob? > > WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields, > it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays, > GUID, and utf16 string. > > QAPI'fying that structure back and forth would be tedious and > error-prone. Better to treat it as an opaque blob imho. I worry about potential consequences of baking Windows ABI into QMP. What if the memory representation of this struct changes? Such ABI changes are unpleasant, but they are not impossible. >> > # Returns: Nothing on success >> > # >> > # Since: 0.14 >> > @@ -270,7 +278,11 @@ >> > # <- { "return": {} } >> > # >> > ## >> > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } >> > +{ 'command': 'getfd', 'data': { >> > + 'fdname': 'str', >> > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} >> > + } >> > +} >> >> What happens when QEMU runs on a Windows host and the client doesn't >> pass @wsa-info? > > It attempts to get the fd from the last recv, but it will fail on > Windows, this is not available. So it fails exactly like it fails on a POSIX host when you execute getfd without passing along a file descriptor with SCM_RIGHTS. Correct?
On Mon, Feb 20, 2023 at 09:26:24AM +0100, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi Markus > > > > On Fri, Feb 17, 2023 at 1:49 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> marcandre.lureau@redhat.com writes: > >> > >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > >> > A process with enough capabilities can duplicate a socket to QEMU. > >> > Modify 'getfd' to import it and add it to the monitor fd list, so it can > >> > be later used by other commands. > >> > > >> > Note that we actually store the SOCKET in the FD list, appropriate care > >> > must now be taken to use the correct socket functions (similar approach > >> > is taken by our io/ code and in glib, this is internal and shouldn't > >> > affect the QEMU/QMP users) > >> > > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > --- > >> > qapi/misc.json | 16 ++++++++-- > >> > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > >> > monitor/hmp-cmds.c | 6 +++- > >> > 3 files changed, 81 insertions(+), 20 deletions(-) > >> > > >> > diff --git a/qapi/misc.json b/qapi/misc.json > >> > index 27ef5a2b20..cd36d8befb 100644 > >> > --- a/qapi/misc.json > >> > +++ b/qapi/misc.json > >> > @@ -249,10 +249,18 @@ > >> > ## > >> > # @getfd: > >> > # > >> > -# Receive a file descriptor via SCM rights and assign it a name > >> > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. > >> > +# > >> > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a > >> > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via > >> > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is > >> > +# considered as a kind of "file descriptor" in QMP context, for historical > >> > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. > >> > >> The Windows part explains things in terms of the C socket API. Less > >> than ideal for the QEMU QMP Reference Manual, isn't it? I don't know > >> nearly enough about this stuff to suggest concrete improvements... > > > > We don't have to, after all we don't explain how to use sendmsg/cmsg > > stuff to pass FDs. > > > > I will drop the part about "A SOCKET is considered as a kind of "file > > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not > > mix SOCKET and fd space" > > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/) > > merged. > > Would it make sense to rebase this series on top of that one, so we > can have simpler documentation from the start? > > >> What does this command do under Windows before this patch? Fail always? > > > > Without ancillary data support on Windows, you can't make it work. > > Yes, but how does it fail? Hmm, you actually answer that below. > > >> Wrap your lines a bit earlier, please. > >> > >> > # > >> > # @fdname: file descriptor name > >> > # > >> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. > >> > +# > >> > >> No way around passing a binary blob? > > > > WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields, > > it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays, > > GUID, and utf16 string. > > > > QAPI'fying that structure back and forth would be tedious and > > error-prone. Better to treat it as an opaque blob imho. > > I worry about potential consequences of baking Windows ABI into QMP. > > What if the memory representation of this struct changes? > > Such ABI changes are unpleasant, but they are not impossible. IIUC, the Windows API aims to be append only. So any need to change this struct would instead result in creating a new struct + new corresponding API. FWIW, there's also a WSAPROTOCOL_INFOA version of this struct which has an ascii string instead of utf16 string. I'm not especially happy about encoding a struct as a blob either, but in this case I'm coming around to the view that it is probably the least worst option. With regards, Daniel
Hi On Mon, Feb 20, 2023 at 12:26 PM Markus Armbruster <armbru@redhat.com> wrote: > > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi Markus > > > > On Fri, Feb 17, 2023 at 1:49 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> marcandre.lureau@redhat.com writes: > >> > >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > >> > A process with enough capabilities can duplicate a socket to QEMU. > >> > Modify 'getfd' to import it and add it to the monitor fd list, so it can > >> > be later used by other commands. > >> > > >> > Note that we actually store the SOCKET in the FD list, appropriate care > >> > must now be taken to use the correct socket functions (similar approach > >> > is taken by our io/ code and in glib, this is internal and shouldn't > >> > affect the QEMU/QMP users) > >> > > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > --- > >> > qapi/misc.json | 16 ++++++++-- > >> > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- > >> > monitor/hmp-cmds.c | 6 +++- > >> > 3 files changed, 81 insertions(+), 20 deletions(-) > >> > > >> > diff --git a/qapi/misc.json b/qapi/misc.json > >> > index 27ef5a2b20..cd36d8befb 100644 > >> > --- a/qapi/misc.json > >> > +++ b/qapi/misc.json > >> > @@ -249,10 +249,18 @@ > >> > ## > >> > # @getfd: > >> > # > >> > -# Receive a file descriptor via SCM rights and assign it a name > >> > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. > >> > +# > >> > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a > >> > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via > >> > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is > >> > +# considered as a kind of "file descriptor" in QMP context, for historical > >> > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. > >> > >> The Windows part explains things in terms of the C socket API. Less > >> than ideal for the QEMU QMP Reference Manual, isn't it? I don't know > >> nearly enough about this stuff to suggest concrete improvements... > > > > We don't have to, after all we don't explain how to use sendmsg/cmsg > > stuff to pass FDs. > > > > I will drop the part about "A SOCKET is considered as a kind of "file > > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not > > mix SOCKET and fd space" > > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/) > > merged. > > Would it make sense to rebase this series on top of that one, so we > can have simpler documentation from the start? Sure, if only I had more reviews/acks... > > >> What does this command do under Windows before this patch? Fail always? > > > > Without ancillary data support on Windows, you can't make it work. > > Yes, but how does it fail? Hmm, you actually answer that below. > > >> Wrap your lines a bit earlier, please. > >> > >> > # > >> > # @fdname: file descriptor name > >> > # > >> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. > >> > +# > >> > >> No way around passing a binary blob? > > > > WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields, > > it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays, > > GUID, and utf16 string. > > > > QAPI'fying that structure back and forth would be tedious and > > error-prone. Better to treat it as an opaque blob imho. > > I worry about potential consequences of baking Windows ABI into QMP. > > What if the memory representation of this struct changes? > > Such ABI changes are unpleasant, but they are not impossible. This is unlikely, the API users are typically sharing that structure between processes since it was introduced, back in 2000. (see also Daniel reply) > > >> > # Returns: Nothing on success > >> > # > >> > # Since: 0.14 > >> > @@ -270,7 +278,11 @@ > >> > # <- { "return": {} } > >> > # > >> > ## > >> > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } > >> > +{ 'command': 'getfd', 'data': { > >> > + 'fdname': 'str', > >> > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} > >> > + } > >> > +} > >> > >> What happens when QEMU runs on a Windows host and the client doesn't > >> pass @wsa-info? > > > > It attempts to get the fd from the last recv, but it will fail on > > Windows, this is not available. > > So it fails exactly like it fails on a POSIX host when you execute getfd > without passing along a file descriptor with SCM_RIGHTS. Correct? Correct, I get something like: Error { class: GenericError, desc: "No file descriptor supplied via SCM_RIGHTS", id: None }
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Feb 20, 2023 at 12:26 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >> > Hi Markus >> > >> > On Fri, Feb 17, 2023 at 1:49 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> >> >> marcandre.lureau@redhat.com writes: >> >> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> > >> >> > A process with enough capabilities can duplicate a socket to QEMU. >> >> > Modify 'getfd' to import it and add it to the monitor fd list, so it can >> >> > be later used by other commands. >> >> > >> >> > Note that we actually store the SOCKET in the FD list, appropriate care >> >> > must now be taken to use the correct socket functions (similar approach >> >> > is taken by our io/ code and in glib, this is internal and shouldn't >> >> > affect the QEMU/QMP users) >> >> > >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> > --- >> >> > qapi/misc.json | 16 ++++++++-- >> >> > monitor/fds.c | 79 ++++++++++++++++++++++++++++++++++++---------- >> >> > monitor/hmp-cmds.c | 6 +++- >> >> > 3 files changed, 81 insertions(+), 20 deletions(-) >> >> > >> >> > diff --git a/qapi/misc.json b/qapi/misc.json >> >> > index 27ef5a2b20..cd36d8befb 100644 >> >> > --- a/qapi/misc.json >> >> > +++ b/qapi/misc.json >> >> > @@ -249,10 +249,18 @@ >> >> > ## >> >> > # @getfd: >> >> > # >> >> > -# Receive a file descriptor via SCM rights and assign it a name >> >> > +# On UNIX, receive a file descriptor via SCM rights and assign it a name. >> >> > +# >> >> > +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a >> >> > +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via >> >> > +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is >> >> > +# considered as a kind of "file descriptor" in QMP context, for historical >> >> > +# reasons and simplicity. QEMU takes care to use socket functions appropriately. >> >> >> >> The Windows part explains things in terms of the C socket API. Less >> >> than ideal for the QEMU QMP Reference Manual, isn't it? I don't know >> >> nearly enough about this stuff to suggest concrete improvements... >> > >> > We don't have to, after all we don't explain how to use sendmsg/cmsg >> > stuff to pass FDs. >> > >> > I will drop the part about "A SOCKET is considered as a kind of "file >> > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not >> > mix SOCKET and fd space" >> > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/) >> > merged. >> >> Would it make sense to rebase this series on top of that one, so we >> can have simpler documentation from the start? > > Sure, if only I had more reviews/acks... I'll try to have a look. >> >> What does this command do under Windows before this patch? Fail always? >> > >> > Without ancillary data support on Windows, you can't make it work. >> >> Yes, but how does it fail? Hmm, you actually answer that below. [...] >> >> > # Returns: Nothing on success >> >> > # >> >> > # Since: 0.14 >> >> > @@ -270,7 +278,11 @@ >> >> > # <- { "return": {} } >> >> > # >> >> > ## >> >> > -{ 'command': 'getfd', 'data': {'fdname': 'str'} } >> >> > +{ 'command': 'getfd', 'data': { >> >> > + 'fdname': 'str', >> >> > + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} >> >> > + } >> >> > +} >> >> >> >> What happens when QEMU runs on a Windows host and the client doesn't >> >> pass @wsa-info? >> > >> > It attempts to get the fd from the last recv, but it will fail on >> > Windows, this is not available. >> >> So it fails exactly like it fails on a POSIX host when you execute getfd >> without passing along a file descriptor with SCM_RIGHTS. Correct? > > Correct, I get something like: > Error { class: GenericError, desc: "No file descriptor supplied via > SCM_RIGHTS", id: None } Works for me, thanks!
diff --git a/qapi/misc.json b/qapi/misc.json index 27ef5a2b20..cd36d8befb 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -249,10 +249,18 @@ ## # @getfd: # -# Receive a file descriptor via SCM rights and assign it a name +# On UNIX, receive a file descriptor via SCM rights and assign it a name. +# +# On Windows, (where ancillary socket fd-passing isn't an option yet), add a +# socket that was duplicated to QEMU process with WSADuplicateSocketW() via +# WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET is +# considered as a kind of "file descriptor" in QMP context, for historical +# reasons and simplicity. QEMU takes care to use socket functions appropriately. # # @fdname: file descriptor name # +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0. +# # Returns: Nothing on success # # Since: 0.14 @@ -270,7 +278,11 @@ # <- { "return": {} } # ## -{ 'command': 'getfd', 'data': {'fdname': 'str'} } +{ 'command': 'getfd', 'data': { + 'fdname': 'str', + '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'} + } +} ## # @closefd: diff --git a/monitor/fds.c b/monitor/fds.c index 03c5e97c35..a876e1128e 100644 --- a/monitor/fds.c +++ b/monitor/fds.c @@ -29,6 +29,7 @@ #include "qapi/qmp/qerror.h" #include "qemu/ctype.h" #include "qemu/cutils.h" +#include "qemu/sockets.h" #include "sysemu/runstate.h" /* file descriptors passed via SCM_RIGHTS */ @@ -61,36 +62,38 @@ struct MonFdset { static QemuMutex mon_fdsets_lock; static QLIST_HEAD(, MonFdset) mon_fdsets; -void qmp_getfd(const char *fdname, Error **errp) +static void close_fd(int fd) { - Monitor *cur_mon = monitor_cur(); - mon_fd_t *monfd; - int fd, tmp_fd; - - fd = qemu_chr_fe_get_msgfd(&cur_mon->chr); - if (fd == -1) { - error_setg(errp, "No file descriptor supplied via SCM_RIGHTS"); - return; + if (fd_is_socket(fd)) { + closesocket(fd); + } else { + close(fd); } +} + +static void monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int tmp_fd; if (qemu_isdigit(fdname[0])) { - close(fd); + close_fd(fd); error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", "a name not starting with a digit"); return; } - qemu_mutex_lock(&cur_mon->mon_lock); - QLIST_FOREACH(monfd, &cur_mon->fds, next) { + qemu_mutex_lock(&mon->mon_lock); + QLIST_FOREACH(monfd, &mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } tmp_fd = monfd->fd; monfd->fd = fd; - qemu_mutex_unlock(&cur_mon->mon_lock); + qemu_mutex_unlock(&mon->mon_lock); /* Make sure close() is outside critical section */ - close(tmp_fd); + close_fd(tmp_fd); return; } @@ -98,8 +101,50 @@ void qmp_getfd(const char *fdname, Error **errp) monfd->name = g_strdup(fdname); monfd->fd = fd; - QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); - qemu_mutex_unlock(&cur_mon->mon_lock); + QLIST_INSERT_HEAD(&mon->fds, monfd, next); + qemu_mutex_unlock(&mon->mon_lock); +} + +void qmp_getfd(const char *fdname, +#ifdef WIN32 + const char *wsa_info, +#endif + Error **errp) +{ + Monitor *cur_mon = monitor_cur(); + int fd; + +#ifdef WIN32 + if (wsa_info) { + g_autofree WSAPROTOCOL_INFOW *info = NULL; + gsize len; + SOCKET sk; + + info = (void *)g_base64_decode(wsa_info, &len); + if (len != sizeof(*info)) { + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value"); + return; + } + + sk = WSASocketW(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, + FROM_PROTOCOL_INFO, info, 0, 0); + if (sk == INVALID_SOCKET) { + g_autofree gchar *emsg = g_win32_error_message(WSAGetLastError()); + error_setg(errp, "Couldn't create socket: %s", emsg); + return; + } + + return monitor_add_fd(cur_mon, sk, fdname, errp); + } +#endif + + fd = qemu_chr_fe_get_msgfd(&cur_mon->chr); + if (fd == -1) { + error_setg(errp, "No file descriptor supplied via SCM_RIGHTS"); + return; + } + + return monitor_add_fd(cur_mon, fd, fdname, errp); } void qmp_closefd(const char *fdname, Error **errp) @@ -120,7 +165,7 @@ void qmp_closefd(const char *fdname, Error **errp) g_free(monfd); qemu_mutex_unlock(&cur_mon->mon_lock); /* Make sure close() is outside critical section */ - close(tmp_fd); + close_fd(tmp_fd); return; } diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 34bd8c67d7..46930bc1a9 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -197,7 +197,11 @@ void hmp_getfd(Monitor *mon, const QDict *qdict) const char *fdname = qdict_get_str(qdict, "fdname"); Error *err = NULL; - qmp_getfd(fdname, &err); + qmp_getfd(fdname, +#ifdef WIN32 + NULL, +#endif + &err); hmp_handle_error(mon, err); }