[PATCH 0/2] monitor openfd commands

[PATCH 0/2] monitor openfd commands

[Dr. David Alan Gilbert (git)]

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.

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(-)

-- 
2.26.2

[PATCH 1/2] qmp: Add 'openfd' command

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The existing 'getfd' command imports an fd from the monitor via
SCM rights.
This command allows qemu to open the file for itself; this is convenient
primarily in testing, or with simple QMP clients.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 monitor/misc.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 qapi/misc.json | 23 ++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index f5207cd242..d538af592a 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1230,22 +1230,20 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
     }
 }
 
-void qmp_getfd(const char *fdname, Error **errp)
+/*
+ * Add a named fd to the monitor list.
+ * Returns true on success.
+ */
+static bool addfd(const char *fdname, int fd, Error **errp)
 {
     mon_fd_t *monfd;
-    int fd, tmp_fd;
-
-    fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
-    if (fd == -1) {
-        error_setg(errp, QERR_FD_NOT_SUPPLIED);
-        return;
-    }
+    int tmp_fd;
 
     if (qemu_isdigit(fdname[0])) {
         close(fd);
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
                    "a name not starting with a digit");
-        return;
+        return false;
     }
 
     qemu_mutex_lock(&cur_mon->mon_lock);
@@ -1259,7 +1257,7 @@ void qmp_getfd(const char *fdname, Error **errp)
         qemu_mutex_unlock(&cur_mon->mon_lock);
         /* Make sure close() is outside critical section */
         close(tmp_fd);
-        return;
+        return true;
     }
 
     monfd = g_malloc0(sizeof(mon_fd_t));
@@ -1268,6 +1266,36 @@ void qmp_getfd(const char *fdname, Error **errp)
 
     QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
     qemu_mutex_unlock(&cur_mon->mon_lock);
+
+    return true;
+}
+
+void qmp_getfd(const char *fdname, Error **errp)
+{
+    int fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
+    if (fd == -1) {
+        error_setg(errp, QERR_FD_NOT_SUPPLIED);
+        return;
+    }
+
+    if (!addfd(fdname, fd, errp)) {
+        close(fd);
+    }
+}
+
+void qmp_openfd(const char *fdname, const char *filename, Error **errp)
+{
+    int fd;
+
+    fd = open(filename, O_RDWR | O_CREAT, S_IRWXU);
+    if (fd == -1) {
+        error_setg_errno(errp, errno, "Cannot open file '%s'", filename);
+        return;
+    }
+
+    if (!addfd(fdname, fd, errp)) {
+        close(fd);
+    }
 }
 
 void qmp_closefd(const char *fdname, Error **errp)
diff --git a/qapi/misc.json b/qapi/misc.json
index 99b90ac80b..baec07358e 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -952,7 +952,7 @@
 ##
 # @closefd:
 #
-# Close a file descriptor previously passed via SCM rights
+# Close a named file descriptor
 #
 # @fdname: file descriptor name
 #
@@ -968,6 +968,27 @@
 ##
 { 'command': 'closefd', 'data': {'fdname': 'str'} }
 
+##
+# @openfd:
+#
+# Open a file descriptor.  The file is opened read-write.
+#
+# @fdname: file descriptor name
+# @filename: file name
+#
+# Returns: Nothing on success
+#
+# Since: 5.1
+#
+# Example:
+#
+# -> { "execute": "openfd", "arguments": { "fdname": "null",
+#                                          "filename": "/dev/null" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'openfd', 'data': {'fdname': 'str', 'filename': 'str'} }
+
 ##
 # @MemoryInfo:
 #
-- 
2.26.2

Re: [PATCH 1/2] qmp: Add 'openfd' command

[Eric Blake]

On 6/11/20 6:17 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The existing 'getfd' command imports an fd from the monitor via
> SCM rights.
> This command allows qemu to open the file for itself; this is convenient
> primarily in testing, or with simple QMP clients.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   monitor/misc.c | 48 ++++++++++++++++++++++++++++++++++++++----------
>   qapi/misc.json | 23 ++++++++++++++++++++++-
>   2 files changed, 60 insertions(+), 11 deletions(-)
> 

> +++ b/qapi/misc.json
> @@ -952,7 +952,7 @@
>   ##
>   # @closefd:
>   #
> -# Close a file descriptor previously passed via SCM rights
> +# Close a named file descriptor
>   #
>   # @fdname: file descriptor name
>   #
> @@ -968,6 +968,27 @@
>   ##
>   { 'command': 'closefd', 'data': {'fdname': 'str'} }
>   
> +##
> +# @openfd:
> +#
> +# Open a file descriptor.  The file is opened read-write.

Should this mention that this is shorthand for opening the file 
externally then using getfd on that fd?  And if we add that 
cross-reference, should we add the reverse reference in the docs for getfd?

> +#
> +# @fdname: file descriptor name
> +# @filename: file name
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 5.1
> +#
> +# Example:
> +#
> +# -> { "execute": "openfd", "arguments": { "fdname": "null",
> +#                                          "filename": "/dev/null" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'openfd', 'data': {'fdname': 'str', 'filename': 'str'} }
> +
>   ##
>   # @MemoryInfo:
>   #
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH 2/2] hmp: Add 'openfd' command

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Wire up the hmp 'openfd' to open a file and add it to the monitor's
fd stash.
Example usage:

(qemu) openfd mig "my.mig"
(qemu) migrate -d "fd:mig"

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx        | 16 +++++++++++++++-
 include/monitor/hmp.h  |  1 +
 monitor/hmp-cmds.c     | 10 ++++++++++
 tests/qtest/test-hmp.c |  2 ++
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 28256209b5..c10d5b3668 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1606,7 +1606,7 @@ ERST
         .name       = "closefd",
         .args_type  = "fdname:s",
         .params     = "closefd name",
-        .help       = "close a file descriptor previously passed via SCM rights",
+        .help       = "close a named file descriptor",
         .cmd        = hmp_closefd,
     },
 
@@ -1617,6 +1617,20 @@ SRST
   used by another monitor command.
 ERST
 
+    {
+        .name       = "openfd",
+        .args_type  = "fdname:s,filename:s",
+        .params     = "openfd name filename",
+        .help       = "open the named file (read write) and assign it a name",
+        .cmd        = hmp_openfd,
+    },
+
+SRST
+``openfd`` *fdname* *filename*
+  Open the named file and store it using the name *fdname* for later use by
+  other monitor comands.
+ERST
+
     {
         .name       = "block_passwd",
         .args_type  = "device:B,password:s",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index c986cfd28b..7beaf3eab7 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -83,6 +83,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
 void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
+void hmp_openfd(Monitor *mon, const QDict *qdict);
 void hmp_sendkey(Monitor *mon, const QDict *qdict);
 void hmp_screendump(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9c61e769ca..62a3a8a514 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1659,6 +1659,16 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+void hmp_openfd(Monitor *mon, const QDict *qdict)
+{
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    const char *filename = qdict_get_str(qdict, "filename");
+    Error *err = NULL;
+
+    qmp_openfd(fdname, filename, &err);
+    hmp_handle_error(mon, err);
+}
+
 void hmp_sendkey(Monitor *mon, const QDict *qdict)
 {
     const char *keys = qdict_get_str(qdict, "keys");
diff --git a/tests/qtest/test-hmp.c b/tests/qtest/test-hmp.c
index b8b1271b9e..66a4f34348 100644
--- a/tests/qtest/test-hmp.c
+++ b/tests/qtest/test-hmp.c
@@ -57,6 +57,8 @@ static const char *hmp_cmds[] = {
     "o /w 0 0x1234",
     "object_add memory-backend-ram,id=mem1,size=256M",
     "object_del mem1",
+    "openfd null \"/dev/null\"",
+    "closefd null", /* purposely after openfd */
     "pmemsave 0 4096 \"/dev/null\"",
     "p $pc + 8",
     "qom-list /",
-- 
2.26.2

Re: [PATCH 0/2] monitor openfd commands

[Eric Blake]

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. 
  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.

Would it instead be worth modifying migration to work with add-fd?  Or 
does add-fd need the same sort of sugar?

> 
> 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

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