Re: [Qemu-devel] [PATCH 27/40] char: move QIOChannel-related in char-io.h

[Marc-André Lureau]

Hi

- Original Message -
> On 01/11/2017 11:29 AM, Marc-André Lureau wrote:
> 
> Grammar in subject is a bit terse; maybe:
> 
> char: move QIOChannel-related stuff to char-io.h
> 
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  chardev/char-io.h |  24 +++
> >  chardev/char-io.c | 168
> >  
> >  chardev/char.c| 174
> >  +-
> >  chardev/Makefile.objs |   1 +
> >  4 files changed, 194 insertions(+), 173 deletions(-)
> >  create mode 100644 chardev/char-io.h
> >  create mode 100644 chardev/char-io.c
> > 
> > diff --git a/chardev/char-io.h b/chardev/char-io.h
> > new file mode 100644
> > index 00..ea559fd124
> > --- /dev/null
> > +++ b/chardev/char-io.h
> > @@ -0,0 +1,24 @@
> > +#ifndef CHAR_IO_H
> > +#define CHAR_IO_H
> 
> Must... resist... the temptation to repeat myself!

Ok, I'll just copy the original copyright from qemu-char.c in all the files I 
created in the series.

> 
> > +
> > +#include "qemu/osdep.h"
> 
> .h files should NOT include osdep.h; since all .c files included it
> before any other .h file, then all things in osdep.h are already in
> scope at the point the .h file starts.

ok

> 
> 
> > +static gboolean io_watch_poll_prepare(GSource *source,
> > +  gint *timeout_)
> > +{
> 
> Why the weird spelling of timeout_ ?  Maybe timeout_unused is better?
> 
> 
> > +++ b/chardev/char.c
> 
> > -static gboolean io_watch_poll_prepare(GSource *source,
> > -  gint *timeout_)
> 
> Then again, it's code motion.  Again, up to you if you want to tweak
> style things during code motion, or split them into a general cleanup
> patch separately.

yeah, weird style, any idea why Anthony wrote it that way in commit 96c638477?

I guess I may just drop the _ during the move

Re: [Qemu-devel] [PATCH 27/40] char: move QIOChannel-related in char-io.h

[Eric Blake]

On 01/11/2017 11:29 AM, Marc-André Lureau wrote:

Grammar in subject is a bit terse; maybe:

char: move QIOChannel-related stuff to char-io.h

> Signed-off-by: Marc-André Lureau 
> ---
>  chardev/char-io.h |  24 +++
>  chardev/char-io.c | 168 
>  chardev/char.c| 174 
> +-
>  chardev/Makefile.objs |   1 +
>  4 files changed, 194 insertions(+), 173 deletions(-)
>  create mode 100644 chardev/char-io.h
>  create mode 100644 chardev/char-io.c
> 
> diff --git a/chardev/char-io.h b/chardev/char-io.h
> new file mode 100644
> index 00..ea559fd124
> --- /dev/null
> +++ b/chardev/char-io.h
> @@ -0,0 +1,24 @@
> +#ifndef CHAR_IO_H
> +#define CHAR_IO_H

Must... resist... the temptation to repeat myself!

> +
> +#include "qemu/osdep.h"

.h files should NOT include osdep.h; since all .c files included it
before any other .h file, then all things in osdep.h are already in
scope at the point the .h file starts.


> +static gboolean io_watch_poll_prepare(GSource *source,
> +  gint *timeout_)
> +{

Why the weird spelling of timeout_ ?  Maybe timeout_unused is better?


> +++ b/chardev/char.c

> -static gboolean io_watch_poll_prepare(GSource *source,
> -  gint *timeout_)

Then again, it's code motion.  Again, up to you if you want to tweak
style things during code motion, or split them into a general cleanup
patch separately.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 27/40] char: move QIOChannel-related in char-io.h

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 chardev/char-io.h |  24 +++
 chardev/char-io.c | 168 
 chardev/char.c| 174 +-
 chardev/Makefile.objs |   1 +
 4 files changed, 194 insertions(+), 173 deletions(-)
 create mode 100644 chardev/char-io.h
 create mode 100644 chardev/char-io.c

diff --git a/chardev/char-io.h b/chardev/char-io.h
new file mode 100644
index 00..ea559fd124
--- /dev/null
+++ b/chardev/char-io.h
@@ -0,0 +1,24 @@
+#ifndef CHAR_IO_H
+#define CHAR_IO_H
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "io/channel.h"
+#include "sysemu/char.h"
+
+/* Can only be used for read */
+guint io_add_watch_poll(Chardev *chr,
+QIOChannel *ioc,
+IOCanReadHandler *fd_can_read,
+QIOChannelFunc fd_read,
+gpointer user_data,
+GMainContext *context);
+
+void remove_fd_in_watch(Chardev *chr);
+
+int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
+
+int io_channel_send_full(QIOChannel *ioc, const void *buf, size_t len,
+ int *fds, size_t nfds);
+
+#endif /* CHAR_IO_H */
diff --git a/chardev/char-io.c b/chardev/char-io.c
new file mode 100644
index 00..82bc721d1a
--- /dev/null
+++ b/chardev/char-io.c
@@ -0,0 +1,168 @@
+#include "char-io.h"
+
+typedef struct IOWatchPoll {
+GSource parent;
+
+QIOChannel *ioc;
+GSource *src;
+
+IOCanReadHandler *fd_can_read;
+GSourceFunc fd_read;
+void *opaque;
+GMainContext *context;
+} IOWatchPoll;
+
+static IOWatchPoll *io_watch_poll_from_source(GSource *source)
+{
+return container_of(source, IOWatchPoll, parent);
+}
+
+static gboolean io_watch_poll_prepare(GSource *source,
+  gint *timeout_)
+{
+IOWatchPoll *iwp = io_watch_poll_from_source(source);
+bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
+bool was_active = iwp->src != NULL;
+if (was_active == now_active) {
+return FALSE;
+}
+
+if (now_active) {
+iwp->src = qio_channel_create_watch(
+iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
+g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
+g_source_attach(iwp->src, iwp->context);
+} else {
+g_source_destroy(iwp->src);
+g_source_unref(iwp->src);
+iwp->src = NULL;
+}
+return FALSE;
+}
+
+static gboolean io_watch_poll_check(GSource *source)
+{
+return FALSE;
+}
+
+static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
+   gpointer user_data)
+{
+abort();
+}
+
+static void io_watch_poll_finalize(GSource *source)
+{
+/* Due to a glib bug, removing the last reference to a source
+ * inside a finalize callback causes recursive locking (and a
+ * deadlock).  This is not a problem inside other callbacks,
+ * including dispatch callbacks, so we call io_remove_watch_poll
+ * to remove this source.  At this point, iwp->src must
+ * be NULL, or we would leak it.
+ *
+ * This would be solved much more elegantly by child sources,
+ * but we support older glib versions that do not have them.
+ */
+IOWatchPoll *iwp = io_watch_poll_from_source(source);
+assert(iwp->src == NULL);
+}
+
+static GSourceFuncs io_watch_poll_funcs = {
+.prepare = io_watch_poll_prepare,
+.check = io_watch_poll_check,
+.dispatch = io_watch_poll_dispatch,
+.finalize = io_watch_poll_finalize,
+};
+
+guint io_add_watch_poll(Chardev *chr,
+QIOChannel *ioc,
+IOCanReadHandler *fd_can_read,
+QIOChannelFunc fd_read,
+gpointer user_data,
+GMainContext *context)
+{
+IOWatchPoll *iwp;
+int tag;
+char *name;
+
+iwp = (IOWatchPoll *) g_source_new(_watch_poll_funcs,
+   sizeof(IOWatchPoll));
+iwp->fd_can_read = fd_can_read;
+iwp->opaque = user_data;
+iwp->ioc = ioc;
+iwp->fd_read = (GSourceFunc) fd_read;
+iwp->src = NULL;
+iwp->context = context;
+
+name = g_strdup_printf("chardev-iowatch-%s", chr->label);
+g_source_set_name((GSource *)iwp, name);
+g_free(name);
+
+tag = g_source_attach(>parent, context);
+g_source_unref(>parent);
+return tag;
+}
+
+static void io_remove_watch_poll(guint tag)
+{
+GSource *source;
+IOWatchPoll *iwp;
+
+g_return_if_fail(tag > 0);
+
+source = g_main_context_find_source_by_id(NULL, tag);
+g_return_if_fail(source != NULL);
+
+iwp = io_watch_poll_from_source(source);
+if (iwp->src) {
+g_source_destroy(iwp->src);
+g_source_unref(iwp->src);
+iwp->src = NULL;
+}
+g_source_destroy(>parent);