Re: [PATCH 0/2] monitor openfd commands

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 6/11/20 6:17 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The monitors currently have a 'getfd' command that lets you pass an fd
> > via the monitor socket.  'openfd' is a new command that opens a file
> > and puts the fd in the same fd pool.  The file is opened RW and created
> > if it doesn't exist.
> > It makes it easy to test migration to and from a file.
> 
> We have two fd-passing mechanisms: getfd and add-fd.  add-fd is newer, and
> allows things like /dev/fdset/NNN to work anywhere a filename works.

Ewww I do dislike fake paths, they tend to be the source of great
security bugs.

>  I'm
> guessing that the issue here is that migration hasn't been tweaked to work
> nicely with the newer add-fd, but instead insists on the older getfd
> interface (where you have to use getfd to associate an fd with a name, then
> tell migration to use that special name, but the special name is via a
> different parameter than the normal filename parameter).  At which point
> openfd looks like it is just sugar to make getfd easier to use.

Yep, openfd is just intended to be sugar; it's a pain to use getfd at
runtime because you have to play all the passing fd's over socket magic.
My main reason here is because I wanted an easy way to migrate to
/dev/null for performance testing, but it would make life easier when
migrating to/from a file.

> Would it instead be worth modifying migration to work with add-fd?

Probably.  At the moment what we have is an 'fd:string' syntax for both
inbound and outbound migration.
The outbound migration looks up the string in the getfd index and uses
it. (via monitor_get_fd)

The inbound migration checks if the string starts with a number, if it
is then it uses it raw as the unix fd; else it passes it to getfd index.
(via monitor_fd_param).
(getfd disallows names that are numeric)

I can see a few solutions here:
  a) Teach qemu a new fdset:number syntax - it's a bit of a pain
     but is discoverable.
  b) Modify the getfd string lookup to parse /dev/fdset and go use
     the fdset

The problem with (b) is that the getfd mechanism doesn't have a
concept of open mode, and only has a single fd bound to a name,
so none of the existing parsing code would know which entry to use in an
fdset. (I'm assuming here no one created a getfd entry named
/dev/fdset/0 - although it seems legal).

The problem with (a) is that adding a new syntax is a bit more code;
but I guess it's probably more discoverable.

ANy preferences?

> add-fd need the same sort of sugar?

Yep, I'd like to have a way to open a file other than via scmrights.
I'd also need to add an HMP equivalent.

Dave

> 
> > 
> > Dr. David Alan Gilbert (2):
> >    qmp: Add 'openfd' command
> >    hmp: Add 'openfd' command
> > 
> >   hmp-commands.hx        | 16 +++++++++++++-
> >   include/monitor/hmp.h  |  1 +
> >   monitor/hmp-cmds.c     | 10 +++++++++
> >   monitor/misc.c         | 48 +++++++++++++++++++++++++++++++++---------
> >   qapi/misc.json         | 23 +++++++++++++++++++-
> >   tests/qtest/test-hmp.c |  2 ++
> >   6 files changed, 88 insertions(+), 12 deletions(-)
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK