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

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





On 08/07/2012 02:16 PM, Stefan Hajnoczi wrote:
On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant <coreyb linux vnet ibm com> wrote:
diff --git a/monitor.c b/monitor.c
index 49dccfe..9aa9f7e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -140,6 +140,24 @@ struct mon_fd_t {
      QLIST_ENTRY(mon_fd_t) next;
  };

+/* file descriptor associated with a file descriptor set */
+typedef struct mon_fdset_fd_t mon_fdset_fd_t;
+struct mon_fdset_fd_t {

QEMU coding style is:

typedef struct MonFdsetFd MonFdsetFd;
struct MonFdsetFd {

See ./CODING_STYLE for more info.


Thanks, I'll fix that.

+    int fd;
+    bool removed;
+    QLIST_ENTRY(mon_fdset_fd_t) next;
+};
+
+/* file descriptor set containing fds passed via SCM_RIGHTS */
+typedef struct mon_fdset_t mon_fdset_t;
+struct mon_fdset_t {
+    int64_t id;
+    int refcount;
+    bool in_use;
+    QLIST_HEAD(, mon_fdset_fd_t) fds;
+    QLIST_ENTRY(mon_fdset_t) next;
+};

At this point in the patch series it's not clear to me whether the
removed and refcount/in_use fields are a clean and correct solution.
Exposing these fields via QMP is also something I'm going to carefully
review because they seem like internals.


Yes, please review the v7 patches and let me know what you think. I explained the purpose of these fields in the previous email I just sent you, so I won't go into their details again here. But I will point out that refcount/in-use came about after concern of fd leakage if libvirt's monitor connection disconnects. query-fdsets allows the client to determine the state of the fd sets after reconnecting.

+
  typedef struct MonitorControl {
      QObject *id;
      JSONMessageParser parser;
@@ -176,7 +194,8 @@ struct Monitor {
      int print_calls_nr;
  #endif
      QError *error;
-    QLIST_HEAD(,mon_fd_t) fds;
+    QLIST_HEAD(, mon_fd_t) fds;
+    QLIST_HEAD(, mon_fdset_t) fdsets;
      QLIST_ENTRY(Monitor) entry;
  };

@@ -2389,6 +2408,157 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
      return -1;
  }

+static void monitor_fdset_cleanup(mon_fdset_t *mon_fdset)
+{
+    mon_fdset_fd_t *mon_fdset_fd;
+    mon_fdset_fd_t *mon_fdset_fd_next;
+
+    if (mon_fdset->refcount != 0) {
+        return;
+    }
+
+    QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) {
+        if (!mon_fdset->in_use || mon_fdset_fd->removed) {
+            close(mon_fdset_fd->fd);
+            QLIST_REMOVE(mon_fdset_fd, next);
+            g_free(mon_fdset_fd);
+        }
+    }
+
+    if (QLIST_EMPTY(&mon_fdset->fds)) {
+        QLIST_REMOVE(mon_fdset, next);
+        g_free(mon_fdset);
+    }
+}
+
+AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, Error **errp)
+{
+    int fd;
+    Monitor *mon = cur_mon;
+    mon_fdset_t *mon_fdset;
+    mon_fdset_fd_t *mon_fdset_fd;
+    AddfdInfo *fdinfo;
+
+    fd = qemu_chr_fe_get_msgfd(mon->chr);
+    if (fd == -1) {
+        qerror_report(QERR_FD_NOT_SUPPLIED);
+        return NULL;
+    }
+
+    if (has_fdset_id) {
+        QLIST_FOREACH(mon_fdset, &mon->fdsets, next) {
+            if (mon_fdset->id == fdset_id) {
+                break;
+            }
+        }
+        if (mon_fdset == NULL) {
+            qerror_report(QERR_FDSET_NOT_FOUND, fdset_id);
+            return NULL;

fd is leaked?


Yes, it looks like it is.  I'll fix that.

+        }
+    } else {
+        int64_t fdset_id_prev = -1;
+        mon_fdset_t *mon_fdset_cur = QLIST_FIRST(&mon->fdsets);
+
+        /* Use first available fdset ID */
+        QLIST_FOREACH(mon_fdset, &mon->fdsets, next) {
+            mon_fdset_cur = mon_fdset;
+            if (fdset_id_prev == mon_fdset_cur->id - 1) {
+                fdset_id_prev = mon_fdset_cur->id;
+                continue;
+            }
+            break;
+        }
+
+        mon_fdset = g_malloc0(sizeof(*mon_fdset));
+        mon_fdset->id = fdset_id_prev + 1;
+        mon_fdset->refcount = 0;
+        mon_fdset->in_use = true;
+
+        /* The fdset list is ordered by fdset ID */
+        if (mon_fdset->id == 0) {
+            QLIST_INSERT_HEAD(&mon->fdsets, mon_fdset, next);
+        } else if (mon_fdset->id < mon_fdset_cur->id) {
+            QLIST_INSERT_BEFORE(mon_fdset_cur, mon_fdset, next);
+        } else {
+            QLIST_INSERT_AFTER(mon_fdset_cur, mon_fdset, next);
+        }
+    }
+
+    mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd));
+    mon_fdset_fd->fd = fd;
+    mon_fdset_fd->removed = false;
+    QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next);
+
+    fdinfo = g_malloc0(sizeof(*fdinfo));
+    fdinfo->fdset_id = mon_fdset->id;
+    fdinfo->fd = mon_fdset_fd->fd;
+
+    return fdinfo;
+}
+
+void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
+{
+    Monitor *mon = cur_mon;
+    mon_fdset_t *mon_fdset;
+    mon_fdset_fd_t *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), "%ld", fd);
+    qerror_report(QERR_FD_NOT_FOUND, fd_str);

Why use an fd_str instead of passing an int64_t into the error
message?  This also assumed sizeof(long) == 8, which isn't true on
32-bit hosts, so %ld should be %"PRId64".

Can I pass an int64_t into the message if it takes a string?

I thought int64_t was a long long in 32-bit mode, but perhaps that's not always the case?


There is a new policy on error reporting.  I think this patch series
may be affected/conflict, please qemu-devel to check.  I think Luiz
can also help here.

Ok thanks, I'll take a look at qemu-devel.

--
Regards,
Corey



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