[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command





On 06/15/2012 11:14 AM, Luiz Capitulino wrote:
On Fri, 15 Jun 2012 11:04:16 -0400
Corey Bryant <coreyb linux vnet ibm com> wrote:



On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:02 -0400
Corey Bryant <coreyb linux vnet ibm com> wrote:

This patch adds the pass-fd QMP command using the QAPI framework.
Like the getfd command, it is used to pass a file descriptor via
SCM_RIGHTS.  However, the pass-fd command also returns the received
file descriptor, which is a difference in behavior from the getfd
command, which returns nothing.

The closefd command can be used to close a file descriptor that was
passed with the pass-fd command.

Note that when using getfd or pass-fd, there are some commands
(e.g. migrate with fd:name) that implicitly close the named fd.
When this is not the case, closefd must be used to avoid fd leaks.

Signed-off-by: Corey Bryant <coreyb linux vnet ibm com>
---
v2:
   -Introduce new QMP command to pass/return fd (lcapitulino redhat com)
   -Use passfd as command name (berrange redhat com)

v3:
   -Use pass-fd as command name (lcapitulino redhat com)
   -Fail pass-fd if fdname already exists (lcapitulino redhat com)
   -Add notes to QMP command describing behavior in more detail
    (lcapitulino redhat com, eblake redhat com)
   -Add note about fd leakage (eblake redhat com)

   monitor.c        |   33 +++++++++++++++++++++++++++++++++
   qapi-schema.json |   19 +++++++++++++++++++
   qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
   3 files changed, 86 insertions(+)

diff --git a/monitor.c b/monitor.c
index 1a7f7e7..6d99368 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
   }
   #endif

+int64_t qmp_pass_fd(const char *fdname, Error **errp)
+{
+    mon_fd_t *monfd;
+    int fd;
+
+    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
+    if (fd == -1) {
+        error_set(errp, QERR_FD_NOT_SUPPLIED);
+        return -1;
+    }
+
+    if (qemu_isdigit(fdname[0])) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                  "a name not starting with a digit");
+        return -1;
+    }
+
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
+        if (strcmp(monfd->name, fdname) == 0) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                      "a name that does not already exist");
+            return -1;
+        }
+    }

Returning the same error class for two different errors is not a good idea.
I think you have two options here. You could return QERR_INVALID_PARAMETER
for the "already exists" case or introduce QERR_FD_EXISTS. The later is
certainly nicer, but we were trying to avoid having too specific errors...


I'm not clear on what the problem is with returning the same error class
for two different errors.  Could you explain?  I don't have a problem
changing it if it's an issue.

Because an mngt app/user won't know if what has happened was that the fd name
is invalid or if the fd name already exists.


I agree, it makes sense so the management app can program based on the error class. It just seems like the error classes in general should be able to be used multiple times if it fits, especially if we're trying to avoid having too specific errors.

Maybe (in the future) something like a sub-class could be used? In other words to allow using the same error class with unique sub-classes.

I'll plan on updating to use a different error class for this.


+
+    monfd = g_malloc0(sizeof(mon_fd_t));
+    monfd->name = g_strdup(fdname);
+    monfd->fd = fd;

Maybe you could try to move this to a separate function to share code with
qmp_getfd()?


Sure, no problem.  I can do that.

+
+    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    return fd;
+}
+
   void qmp_getfd(const char *fdname, Error **errp)
   {
       mon_fd_t *monfd;
diff --git a/qapi-schema.json b/qapi-schema.json
index 26a6b84..ed99f23 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1864,6 +1864,25 @@
   { 'command': 'netdev_del', 'data': {'id': 'str'} }

   ##
+# @pass-fd:
+#
+# Pass a file descriptor via SCM rights and assign it a name
+#
+# @fdname: file descriptor name
+#
+# Returns: The QEMU file descriptor that was received
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdname is not valid, InvalidParameterType
+#
+# Since: 1.2.0
+#
+# Notes: If @fdname already exists, the command will fail.
+#        The 'closefd' command can be used to explicitly close the
+#        file descriptor when it is no longer needed.
+##
+{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
+
+##
   # @getfd:
   #
   # Receive a file descriptor via SCM rights and assign it a name
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e3cf3c5..c039947 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -869,6 +869,40 @@ Example:
   EQMP

       {
+        .name       = "pass-fd",
+        .args_type  = "fdname:s",
+        .params     = "pass-fd name",
+        .help       = "pass a file descriptor via SCM rights and assign it a name",
+        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
+    },
+
+SQMP
+pass-fd
+-------
+
+Pass a file descriptor via SCM rights and assign it a name.
+
+Arguments:
+
+- "fdname": file descriptor name (json-string)
+
+Return a json-int with the QEMU file descriptor that was received.
+
+Example:
+
+-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
+<- { "return": 42 }
+
+Notes:
+
+(1) If the name specified by the "fdname" argument already exists,
+    the command will fail.
+(2) The 'closefd' command can be used to explicitly close the file
+    descriptor when it is no longer needed.
+
+EQMP
+
+    {
           .name       = "getfd",
           .args_type  = "fdname:s",
           .params     = "getfd name",





--
Regards,
Corey



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]