| Message ID | 1429280557-8887-26-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 17/04/2015 16:22, Daniel P. Berrange wrote: > A number of I/O operations need to be performed asynchronously > to avoid blocking the main loop. The caller of such APIs need > to provide a callback to be invoked on completion/error and > need access to the error, if any. The small QIOTask provides > a simple framework for dealing with such probes. The API > docs inline provide an outline of how this is to be used. > > In this series, the QIOTask class will be used for things like > the TLS handshake, the websockets handshake and TCP connect() > progress. Is this actually worth making it a heavyweight QOM object? In general if you don't do object_property_add_child (and I don't think here you do), it's simpler to just make it a C struct. In this case I even think you're leaking the task. You do object_ref twice (once at creation time, once before qio_channel_add_watch_full) and only have one object_unref. I would just do without reference counting, and add a qio_task_free() function that calls qio_task_finalize() and frees the QIOTask. BTW, do you have plans to use the GDestroyNotify argument to qio_task_new, or is it just for consistency? Paolo
On Fri, Apr 17, 2015 at 05:16:26PM +0200, Paolo Bonzini wrote: > > > On 17/04/2015 16:22, Daniel P. Berrange wrote: > > A number of I/O operations need to be performed asynchronously > > to avoid blocking the main loop. The caller of such APIs need > > to provide a callback to be invoked on completion/error and > > need access to the error, if any. The small QIOTask provides > > a simple framework for dealing with such probes. The API > > docs inline provide an outline of how this is to be used. > > > > In this series, the QIOTask class will be used for things like > > the TLS handshake, the websockets handshake and TCP connect() > > progress. > > Is this actually worth making it a heavyweight QOM object? In general > if you don't do object_property_add_child (and I don't think here you > do), it's simpler to just make it a C struct. Essentially I just used QOM anywhere that I wanted to have ref counting, even if I didn't need the property feature. I figure this particular usage isn't performance critical so using the more heavyweight QOM framework wasn't the end of the world. > In this case I even think you're leaking the task. You do object_ref > twice (once at creation time, once before qio_channel_add_watch_full) > and only have one object_unref. I would just do without reference > counting, and add a qio_task_free() function that calls > qio_task_finalize() and frees the QIOTask. Are you referring to the qio_channel_tls_handshake() method in the next patch ? If so it does actually have two object_unref calls so shouldn't be leaking. In more complex scenarios I thnk the ref counting ability will come in useful. Of course I could add ref counting to a plain struct without using QOM, but it felt better to just use QOM and be done with it, so people don't have to remember which particular unref method they must use. > BTW, do you have plans to use the GDestroyNotify argument to > qio_task_new, or is it just for consistency? I'm not 100% sure if I'll need it or not yet. One of my todo items is to double check the sequence of operations when a VNC/chardev client disconnects while a background task is in progress. It is possible I might need to hold a reference on the VNC/chardev state in which case the GDestroyNotify arg will come in useful. So I just added it for consistency initially. Regards, Daniel
On 17/04/2015 17:49, Daniel P. Berrange wrote: > > In this case I even think you're leaking the task. You do object_ref > > twice (once at creation time, once before qio_channel_add_watch_full) > > and only have one object_unref. I would just do without reference > > counting, and add a qio_task_free() function that calls > > qio_task_finalize() and frees the QIOTask. > > Are you referring to the qio_channel_tls_handshake() method in the > next patch ? If so it does actually have two object_unref calls > so shouldn't be leaking. In more complex scenarios I thnk the > ref counting ability will come in useful. Of course I could add > ref counting to a plain struct without using QOM, but it felt > better to just use QOM and be done with it, so people don't have > to remember which particular unref method they must use. I cannot find the second... O:-) > > BTW, do you have plans to use the GDestroyNotify argument to > > qio_task_new, or is it just for consistency? > > I'm not 100% sure if I'll need it or not yet. One of my todo items > is to double check the sequence of operations when a VNC/chardev > client disconnects while a background task is in progress. It is > possible I might need to hold a reference on the VNC/chardev state > in which case the GDestroyNotify arg will come in useful. So I just > added it for consistency initially. Yup, no problem there. Paolo
On Fri, Apr 17, 2015 at 05:57:24PM +0200, Paolo Bonzini wrote: > > > On 17/04/2015 17:49, Daniel P. Berrange wrote: > > > In this case I even think you're leaking the task. You do object_ref > > > twice (once at creation time, once before qio_channel_add_watch_full) > > > and only have one object_unref. I would just do without reference > > > counting, and add a qio_task_free() function that calls > > > qio_task_finalize() and frees the QIOTask. > > > > Are you referring to the qio_channel_tls_handshake() method in the > > next patch ? If so it does actually have two object_unref calls > > so shouldn't be leaking. In more complex scenarios I thnk the > > ref counting ability will come in useful. Of course I could add > > ref counting to a plain struct without using QOM, but it felt > > better to just use QOM and be done with it, so people don't have > > to remember which particular unref method they must use. > > I cannot find the second... O:-) I create & unref it in the same place: + task = qio_task_new(OBJECT(ioc), + func, opaque, destroy); + + qio_channel_tls_handshake_task(ioc, task); + + object_unref(OBJECT(task)); The second ref taken at time of qio_channel_add_watch_full() gets released by the GDestroyNotify that is passed to that method. Regards, Daniel
On 17/04/2015 18:11, Daniel P. Berrange wrote: > > + task = qio_task_new(OBJECT(ioc), > + func, opaque, destroy); > + > + qio_channel_tls_handshake_task(ioc, task); > + > + object_unref(OBJECT(task)); > > The second ref taken at time of qio_channel_add_watch_full() gets released > by the GDestroyNotify that is passed to that method. /me is blind. So this means you really do not need the reference counting; qio_channel_tls_handshake_task (which is static anyway) is what manages the lifetime of the QIOTask, it can take ownership of the task right after it is created. In effect QIOTask is just wrapping the (func, opaque, destroy) continuation. It makes sense that it doesn't need reference counting, because it has a well-defined point of death (just before the continuation is called, or just before you find out it will never be called). So I think it's okay to remove the reference counting and make it a simple heap-allocated struct. gio_channel_tls_handshake_task can free it after calling gio_task_{abort,complete}. Without the reference counting the GDestroyNotify also becomes unnecessary (and the fact that you're using QIOTask as a simple continuation explains why you didn't need GDestroyNotify), but it's okay if you want to leave it in. I would also have QIOTaskFunc take the "exploded" struct, i.e. typedef void QIOTaskFunc(QMyObject *, gpointer, Error *), and hide QIOTask entirely from the user. Paolo
On Fri, Apr 17, 2015 at 07:06:04PM +0200, Paolo Bonzini wrote: > > > On 17/04/2015 18:11, Daniel P. Berrange wrote: > > > > + task = qio_task_new(OBJECT(ioc), > > + func, opaque, destroy); > > + > > + qio_channel_tls_handshake_task(ioc, task); > > + > > + object_unref(OBJECT(task)); > > > > The second ref taken at time of qio_channel_add_watch_full() gets released > > by the GDestroyNotify that is passed to that method. > > /me is blind. > > So this means you really do not need the reference counting; > qio_channel_tls_handshake_task (which is static anyway) is what manages > the lifetime of the QIOTask, it can take ownership of the task right > after it is created. > > In effect QIOTask is just wrapping the (func, opaque, destroy) > continuation. It makes sense that it doesn't need reference counting, > because it has a well-defined point of death (just before the > continuation is called, or just before you find out it will never be > called). So I think it's okay to remove the reference counting and make > it a simple heap-allocated struct. gio_channel_tls_handshake_task can > free it after calling gio_task_{abort,complete}. > > Without the reference counting the GDestroyNotify also becomes > unnecessary (and the fact that you're using QIOTask as a simple > continuation explains why you didn't need GDestroyNotify), but it's okay > if you want to leave it in. > > I would also have QIOTaskFunc take the "exploded" struct, i.e. typedef > void QIOTaskFunc(QMyObject *, gpointer, Error *), and hide QIOTask > entirely from the user. Yeah, I can try that approach and see how it works out. Regards, Daniel
diff --git a/include/io/task.h b/include/io/task.h new file mode 100644 index 0000000..8f86902 --- /dev/null +++ b/include/io/task.h @@ -0,0 +1,168 @@ +/* + * QEMU I/O task + * + * Copyright (c) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#ifndef QIO_TASK_H__ +#define QIO_TASK_H__ + +#include "qemu-common.h" +#include "qapi/error.h" +#include "qom/object.h" + +#define TYPE_QIO_TASK "qemu:io-task" +#define QIO_TASK(obj) \ + OBJECT_CHECK(QIOTask, (obj), TYPE_QIO_TASK) +#define QIO_TASK_CLASS(klass) \ + OBJECT_CLASS_CHECK(QIOTaskClass, klass, TYPE_QIO_TASK) +#define QIO_TASK_GET_CLASS(obj) \ + OBJECT_GET_CLASS(QIOTaskClass, obj, TYPE_QIO_TASK) + +typedef struct QIOTask QIOTask; +typedef struct QIOTaskClass QIOTaskClass; + +typedef void (*QIOTaskFunc)(QIOTask *task, + gpointer opaque); + +/** + * QIOTask: + * + * The QIOTask object provides a simple mechanism for reporting + * success / failure of long running background operations. + * + * A object on which the operation is to be performed could have + * a public API which accepts a task callback: + * + * void myobject_operation(QMyObject *obj, + * QIOTaskFunc *func, + * gpointer opaque, + * GDestroyNotify *notify); + * + * The 'func' parameter is the callback to be invoked, and 'opaque' + * is data to pass to it. The optional 'notify' function is used + * to free 'opaque' when no longer needed. + * + * Now, lets say the implementation of this method wants to set + * a timer to run once a second checking for completion of some + * activity. It would do something like + * + * void myobject_operation(QMyObject *obj, + * QIOTaskFunc *func, + * gpointer opaque, + * GDestroyNotify *notify) + * { + * QIOTask *task; + * + * task = qio_task_new(OBJECT(obj), func, opaque, notify); + * + * g_timeout_add_full(G_PRIORITY_DEFAULT, + * 1000, + * myobject_operation_timer, + * task, + * (GDestroyNotify)object_unref); + * } + * + * It could equally have setup a watch on a file descriptor or + * created a background thread, or something else entirely. + * Notice that the source object is passed to the task, and + * QIOTask will hold a reference on that. This ensure that + * the QMyObject instance cannot be garbage collected while + * the async task is still in progress. + * + * In this case, myobject_operation_timer will fire after + * 3 secs and do + * + * gboolean myobject_operation_timer(gpointer opaque) + * { + * QIOTask *task = QIO_TASK(opaque); + * Error *err;* + * + * ...check something important... + * if (err) { + * qio_task_abort(task, err); + * error_free(task); + * return FALSE; + * } else if (...work is completed ...) { + * qio_task_complete(task); + * return FALSE; + * } + * ...carry on polling ... + * return TRUE; + * } + * + * Once this function returns false, object_unref will be called + * automatically on the task causing it to be released and the + * ref on QMyObject dropped too. + */ + +struct QIOTask { + Object parent; + Object *source; + QIOTaskFunc func; + gpointer opaque; + GDestroyNotify destroy; + Error *err; +}; + +struct QIOTaskClass { + ObjectClass parent; +}; + +/** + * qio_task_new: + * @source: the object on which the operation is invoked + * @func: the callback to invoke when the task completes + * @opaque: opaque data to pass to @func when invoked + * @destroy: optional callback to free @opaque + * + * Creates a new task object to track completion of a + * background operation running on the object @source. + * When the operation completes or fails, the callback + * @func will be invoked. The callback can access the + * 'err' attribute in the task object to determine if + * the operation was successful or not. + * + * Returns: the task object + */ +QIOTask *qio_task_new(Object *source, + QIOTaskFunc func, + gpointer opaque, + GDestroyNotify destroy); + +/** + * qio_task_complete: + * @task: the task object + * + * Mark the operation as succesfully completed + */ +void qio_task_complete(QIOTask *task); + +/** + * qio_task_abort: + * @task: the task object + * @err: the error to record for the operation + * + * Mark the operation as failed, with @err providing + * details about the failure. The @err may be freed + * afer the function returns, as the notification + * callback is invoked synchronously. + */ +void qio_task_abort(QIOTask *task, + Error *err); + +#endif /* QIO_TASK_H__ */ diff --git a/io/Makefile.objs b/io/Makefile.objs index eec1b43..b9973ac 100644 --- a/io/Makefile.objs +++ b/io/Makefile.objs @@ -1,3 +1,4 @@ +util-obj-y += task.o util-obj-y += channel.o util-obj-y += channel-unix.o util-obj-y += channel-socket.o diff --git a/io/task.c b/io/task.c new file mode 100644 index 0000000..453f474 --- /dev/null +++ b/io/task.c @@ -0,0 +1,84 @@ +/* + * QEMU I/O task + * + * Copyright (c) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#include "io/task.h" + +QIOTask *qio_task_new(Object *source, + QIOTaskFunc func, + gpointer opaque, + GDestroyNotify destroy) +{ + QIOTask *task; + + task = QIO_TASK(object_new(TYPE_QIO_TASK)); + + task->source = source; + object_ref(source); + task->func = func; + task->opaque = opaque; + task->destroy = destroy; + + return task; +} + +void qio_task_complete(QIOTask *task) +{ + task->func(task, task->opaque); +} + +void qio_task_abort(QIOTask *task, + Error *err) +{ + task->err = err; + task->func(task, task->opaque); + task->err = NULL; +} + +static void qio_task_initfn(Object *obj G_GNUC_UNUSED) +{ + /* nada */ +} + +static void qio_task_finalize(Object *obj) +{ + QIOTask *task = QIO_TASK(obj); + + object_unref(task->source); + if (task->destroy) { + task->destroy(task->opaque); + } +} + + +static const TypeInfo qio_task_info = { + .parent = TYPE_OBJECT, + .name = TYPE_QIO_TASK, + .instance_size = sizeof(QIOTask), + .instance_init = qio_task_initfn, + .instance_finalize = qio_task_finalize, + .class_size = sizeof(QIOTaskClass), +}; + +static void qio_task_register_types(void) +{ + type_register_static(&qio_task_info); +} + +type_init(qio_task_register_types);
A number of I/O operations need to be performed asynchronously to avoid blocking the main loop. The caller of such APIs need to provide a callback to be invoked on completion/error and need access to the error, if any. The small QIOTask provides a simple framework for dealing with such probes. The API docs inline provide an outline of how this is to be used. In this series, the QIOTask class will be used for things like the TLS handshake, the websockets handshake and TCP connect() progress. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/io/task.h | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ io/Makefile.objs | 1 + io/task.c | 84 +++++++++++++++++++++++++++ 3 files changed, 253 insertions(+) create mode 100644 include/io/task.h create mode 100644 io/task.c