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

[Daniel P . Berrangé]

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"
> > (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?
> 
> >> 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
-- 
|: 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 :|