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

Re: [libvirt] [PATCH v7 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets





On 08/07/2012 12:49 PM, Eric Blake wrote:
On 08/07/2012 09:58 AM, Corey Bryant wrote:
This patch adds support that enables passing of file descriptors
to the QEMU monitor where they will be stored in specified file
descriptor sets.

A file descriptor set can be used by a client like libvirt to
store file descriptors for the same file.  This allows the
client to open a file with different access modes (O_RDWR,
O_WRONLY, O_RDONLY) and add/remove the passed fds to/from an fd
set as needed.  This will allow QEMU to (in a later patch in this
series) "open" and "reopen" the same file by dup()ing the fd in
the fd set that corresponds to the file, where the fd has the
matching access mode flag that QEMU requests.

The new QMP commands are:
   add-fd: Add a file descriptor to an fd set
   remove-fd: Remove a file descriptor from an fd set
   query-fdsets: Return information describing all fd sets


+
+# @AddfdInfo:
+#
+# Information about a file descriptor that was added to an fd set.
+#
+# @fdset_id: The ID of the fd set that @fd was added to.
+#
+# @fd: The file descriptor that was received via SCM rights and
+#      added to the fd set.
+#
+# Since: 1.2.0

We're not very consistent on '1.2' vs. '1.2.0' in since listings, but
that's probably worth a global cleanup closer to hard freeze.


I'll make a note of it.  Or does Luiz usually do a cleanup?

+##
+{ 'type': 'AddfdInfo', 'data': {'fdset_id': 'int', 'fd': 'int'} }

This is a new command, so s/fdset_id/fdset-id/


Ok

+
+##
+# @add-fd:
+#
+# Add a file descriptor, that was passed via SCM rights, to an fd set.
+#
+# @fdset_id: #optional The ID of the fd set to add the file descriptor to.
+#
+# Returns: @AddfdInfo on success
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdset_id does not exist, FdSetNotFound
+#
+# Notes: The list of fd sets is shared by all monitor connections.
+#
+#        If @fdset_id is not specified, a new fd set will be created.
+#
+# Since: 1.2.0
+##
+{ 'command': 'add-fd', 'data': {'*fdset_id': 'int'},

Again, s/fdset_id/fdset-id/


Ok

+  'returns': 'AddfdInfo' }
+
+##
+# @remove-fd:
+#
+# Remove a file descriptor from an fd set.
+#
+# @fdset_id: The ID of the fd set that the file descriptor belongs to.
+#
+# @fd: #optional The file descriptor that is to be removed.
+#
+# Returns: Nothing on success
+#          If @fdset_id or @fd is not found, FdNotFound
+#
+# Since: 1.2.0
+#
+# Notes: The list of fd sets is shared by all monitor connections.
+#
+#        File descriptors that are removed:
+#        o will not be closed until the reference count corresponding
+#          to @fdset_id reaches zero.
+#        o will not be available for use after successful completion
+#          of the remove-fd command.
+#
+#        If @fd is not specified, all file descriptors in @fdset_id
+#        will be removed.
+##
+{ 'command': 'remove-fd', 'data': {'fdset_id': 'int', '*fd': 'int'} }

And again.


Ok

+
+##
+# @FdsetFdInfo:
+#
+# Information about a file descriptor that belongs to an fd set.
+#
+# @fd: The file descriptor value.
+#
+# @removed: If true, the remove-fd command has been issued for this fd.
+#
+# Since: 1.2.0
+##
+{ 'type': 'FdsetFdInfo', 'data': {'fd': 'int', 'removed': 'bool'} }

Is it worth providing any additional information?  For example, knowing
whether the fd is O_RDONLY, O_WRONLY, or O_RDWR might be beneficial to
management apps trying to discover what fds are already present after a
reconnection, in order to decide whether to close them without having to
resort to /proc/$qemupid/fdinfo/nnn lookups.  It might even be worth
marking such information optional, present only when 'removed':false.


It makes sense but I'd like to limit the new functionality at this point so that I can get this support into QEMU 1.2. Can this be added as a follow up patch?

+
+##
+# @FdsetInfo:
+#
+# Information about an fd set.
+#
+# @fdset_id: The ID of the fd set.
+#
+# @refcount: A count of the number of outstanding dup() references to
+#            this fd set.
+#
+# @in_use: If true, a monitor is connected and has access to this fd set.
+#
+# @fds: A list of file descriptors that belong to this fd set.
+#
+# Since: 1.2.0
+##
+{ 'type': 'FdsetInfo',
+  'data': {'fdset_id': 'int', 'refcount': 'int', 'in_use': 'bool',
+           'fds': ['FdsetFdInfo']} }

s/fdset_id/fdset-id/; s/in_use/in-use/


Ok

+
+##
+# @query-fdsets:
+#
+# Return information describing all fd sets.
+#
+# Returns: A list of @FdsetInfo
+#
+# Since: 1.2.0
+#
+# Notes: The list of fd sets is shared by all monitor connections.
+#
+#        File descriptors are not closed until @refcount is zero,
+#        and either @in_use is false or @removed is true.
+#
+##
+{ 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
diff --git a/qerror.c b/qerror.c
index 92c4eff..63a0aa1 100644
--- a/qerror.c
+++ b/qerror.c
@@ -148,6 +148,10 @@ static const QErrorStringTable qerror_table[] = {
          .desc      = "No file descriptor supplied via SCM_RIGHTS",
      },
      {
+        .error_fmt = QERR_FDSET_NOT_FOUND,
+        .desc      = "File descriptor set with ID '%(id)' not found",
+    },

Conflicts with Luiz's error cleanups.  I'm not sure whether libvirt
needs to know this specific error, so I'd rather leave it generic for now.


Ok.  I'll try to use something more generic.

+++ b/qmp-commands.hx
@@ -926,6 +926,137 @@ Example:

  EQMP

+     {
+        .name       = "add-fd",
+        .args_type  = "fdset_id:i?",
+        .params     = "add-fd fdset_id",

More fallout from s/_/-/ in the QMP naming, throughout this file.


Ok

--
Regards,
Corey



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