[Qemu-devel] Re: [PATCH 02/15] Introduce QList

[Anthony Liguori <aliguori@us.ibm.com>]

Luiz Capitulino wrote:
> QList is a high-level data type that can be used to store QObjects
> in a singly-linked list.
>
> The following functions are available:
>
> - qlist_new()    Create a new QList
> - qlist_append() Append a QObject to the list
> - qlist_iter()   Iterate over stored QObjects
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  Makefile  |    2 +-
>  qlist.c   |  100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qlist.h   |   27 ++++++++++++++++
>  qobject.h |    1 +
>  4 files changed, 129 insertions(+), 1 deletions(-)
>  create mode 100644 qlist.c
>  create mode 100644 qlist.h
>   
<snip>
> +/**
> + * qlist_iter(): Iterate over all the list's stored values.
> + *
> + * This function allows the user to provide an iterator, which will be
> + * called for each stored value in the list.
> + */
> +void qlist_iter(const QList *qlist,
> +                void (*iter)(QObject *obj, void *opaque), void *opaque)
> +{
> +    QListEntry *entry;
> +
> +    QTAILQ_FOREACH(entry, &qlist->head, next)
> +        iter(entry->value, opaque);
> +}
> +
> +/**
> + * qobject_to_qlist(): Convert a QObject into a QList
> + */
> +QList *qobject_to_qlist(const QObject *obj)
> +{
> +    if (qobject_type(obj) != QTYPE_QLIST)
> +        return NULL;
> +
> +    return container_of(obj, QList, base);
> +}
>   

Missing {} around ifs.

> +
> +/**
> + * qlist_destroy_obj(): Free all the memory allocated by a QList
> + */
> +static void qlist_destroy_obj(QObject *obj)
> +{
> +    QList *qlist;
> +    QListEntry *entry, *next_entry;
> +
> +    assert(obj != NULL);
>   

Usually accepting NULL in a free function makes for nicer exit paths in 
function.

> +    qlist = qobject_to_qlist(obj);
> +
> +    QTAILQ_FOREACH_SAFE(entry, &qlist->head, next, next_entry) {
> +        QTAILQ_REMOVE(&qlist->head, entry, next);
> +        qobject_decref(entry->value);
> +        qemu_free(entry);
> +    }
> +
> +    qemu_free(qlist);
> +}
> +
> +static const QType qlist_type = {
> +    .code = QTYPE_QLIST,
> +    .destroy = qlist_destroy_obj,
> +};
>   

Forward definitions of statics usually indicate code motion is in 
order.  In this case, I think it's probably a good idea as my first 
reaction to this was, gees, this has to be dead code since it's a static 
at the end of a file.

> diff --git a/qlist.h b/qlist.h
> new file mode 100644
> index 0000000..b38786e
> --- /dev/null
> +++ b/qlist.h
> @@ -0,0 +1,27 @@
>   

Missing copyright/license.

> +#ifndef QLIST
> +#define QLIST
>   

Should probably at least do QLIST_H to avoid namespace polution.

-- 
Regards,

Anthony Liguori