Re: [libvirt] [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets
On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant 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.
> +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.
> +
> 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?
> +}
> +} 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_
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 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_fds
Re: [libvirt] [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets
On Tue, Aug 7, 2012 at 8:59 PM, Corey Bryant wrote: > > > On 08/07/2012 02:16 PM, Stefan Hajnoczi wrote: >> >> On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant >> wrote: >>> +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? The PRId64 format specifier macro from the C standard hides this so you can pass int64_t values to printf()-style functions in a portable way. Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets
On 08/08/2012 04:52 AM, Stefan Hajnoczi wrote: On Tue, Aug 7, 2012 at 8:59 PM, Corey Bryant wrote: On 08/07/2012 02:16 PM, Stefan Hajnoczi wrote: On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant wrote: +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? The PRId64 format specifier macro from the C standard hides this so you can pass int64_t values to printf()-style functions in a portable way. Stefan Thanks, I'll use PRId64. -- Regards, Corey -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
