Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Marc-André Lureau]

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"
> > (20230212204942.1905959-1-marcandre.lureau@redhat.com/">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