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

Re: [libvirt] [Qemu-devel] [PATCH v8 2/7] qapi: Introduce add-fd, remove-fd, query-fdsets





On 08/10/2012 01:57 AM, Eric Blake wrote:
On 08/09/2012 08:10 PM, 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

Note: These commands are not compatible with the existing getfd
and closefd QMP commands.

Signed-off-by: Corey Bryant <coreyb linux vnet ibm com>

+void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
+{
+    MonFdset *mon_fdset;
+    MonFdsetFd *mon_fdset_fd;
+    char fd_str[20];
+
+    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+        if (mon_fdset->id != fdset_id) {
+            continue;
+        }
+        QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
+            if (has_fd && mon_fdset_fd->fd != fd) {
+                continue;
+            }
+            mon_fdset_fd->removed = true;
+            if (has_fd) {
+                break;
+            }
+        }
+        monitor_fdset_cleanup(mon_fdset);
+        return;
+    }
+    snprintf(fd_str, sizeof(fd_str), "%" PRId64, fd);
+    error_set(errp, QERR_FD_NOT_FOUND, fd_str);

This error catches the case of fdset_id not found, but you never raise
an error when has_fd is true but fd is not found within fdset_id.  You
also don't raise an error for back-to-back remove-fd(fdset-id=1,fd=3),
because you aren't checking whether mon_fdset_fd->removed was already true.

I'm not sure which semantic is better, but I _am_ worried that we have
both semantics in teh same function (are we arguing that this command
succeeds when the fd is gone, even if it already was gone? Or are we
arguing that this command diagnoses failure on an attempt to remove
something that doesn't exist).  I guess I'm 60-40 towards always issuing
an error on an attempt to remove something that can't be found or is
already removed.


Thanks for catching this. The code used to fall through to the QERR_FD_NOT_FOUND error before I moved the return outside of the mon_fdset_fd loop. Anyway I intended to set QERR_FD_NOT_FOUND when either the fdset or fd is not found. I'll fix that.

I hadn't thought of back-to-back remove-fd's requiring an error. I could go either way on that. But since I'll be touching the code again I'll issue an error for that too.

+}
+
+FdsetInfoList *qmp_query_fdsets(Error **errp)
+{
+    MonFdset *mon_fdset;
+    MonFdsetFd *mon_fdset_fd;
+    FdsetInfoList *fdset_list = NULL;
+
+    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+        FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
+        FdsetFdInfoList *fdsetfd_list = NULL;
+
+        fdset_info->value = g_malloc0(sizeof(*fdset_info->value));
+        fdset_info->value->fdset_id = mon_fdset->id;
+
+        QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
+            FdsetFdInfoList *fdsetfd_info;
+
+            if (mon_fdset_fd->removed) {
+                continue;
+            }

This means that an fdset with all fds removed will show up as empty in
the output; I guess that's okay, as libvirt could use that to infer that
the dup()d fds are still in use.  The alternative is to keep track of
whether we have output any information about an fd within a set before
including the set itself in the overall output.


You're right, an empty set will be shown in this case. I chose the route of less code. :) I'm going to leave this as-is unless there's an objection.

+++ b/qapi-schema.json

+##
+# @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.
+#
+# @opaque: #optional A free-form string that can be used to describe the fd.
+#
+# Returns: @AddfdInfo on success
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdset_id does not exist, InvalidParameterValue

Missed one: s/_/-/


Gah! Sorry.. and thanks for catching.

+##
+# @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

and another s/_/-/


Ok

+SQMP
+query-fdsets
+-------------
+
+Return information describing all fd sets.
+
+Arguments: None
+
+Example:
+
+-> { "execute": "query-fdsets" }
+<- { "return": [
+       {
+         "fdset-id": 1

missing a comma


Ok. Perhaps I should just use a real example rather than editing the old one!

+         "fds": [
+           {
+             "fd": 21,

JSON does not permit trailing commas.


Ok

+           },
+           {
+             "fd": 23,

and again


Ok

+           }
+         ],
+       },
+       {
+         "fdset-id": 2

missing a comma


Ok

+         "fds": [
+           {
+             "fd": 22,

trailing comma


Ok

Also, it might be nice to include something like:

"opaque":"rdonly:/path/to/file"

in one of the examples to give a hint to the reader how to use opaque.


No problem I'll do that.

--
Regards,
Corey


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