Patchwork [02/11] Add operations to qlist to allow it to be used as a stack

login
register
mail settings
Submitter Anthony Liguori
Date Nov. 11, 2009, 5:28 p.m.
Message ID <1257960543-26373-2-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/38158/
State New
Headers show

Comments

Anthony Liguori - Nov. 11, 2009, 5:28 p.m.
This makes lists no longer invariant. It's a very useful bit of functionality
though.

To deal with the fact that lists are no longer invariant, introduce a deep
copy mechanism for lists.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qlist.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qlist.h |    4 ++++
 2 files changed, 60 insertions(+), 0 deletions(-)
Kevin Wolf - Nov. 12, 2009, 3:33 p.m.
Am 11.11.2009 18:28, schrieb Anthony Liguori:
> This makes lists no longer invariant. It's a very useful bit of functionality
> though.
> 
> To deal with the fact that lists are no longer invariant, introduce a deep
> copy mechanism for lists.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qlist.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qlist.h |    4 ++++
>  2 files changed, 60 insertions(+), 0 deletions(-)

So far all functions in qlist.c have a header comment. Any reason to
change this?

Kevin
Anthony Liguori - Nov. 12, 2009, 4:46 p.m.
Kevin Wolf wrote:
> Am 11.11.2009 18:28, schrieb Anthony Liguori:
>   
>> This makes lists no longer invariant. It's a very useful bit of functionality
>> though.
>>
>> To deal with the fact that lists are no longer invariant, introduce a deep
>> copy mechanism for lists.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  qlist.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qlist.h |    4 ++++
>>  2 files changed, 60 insertions(+), 0 deletions(-)
>>     
>
> So far all functions in qlist.c have a header comment. Any reason to
> change this?
>   

But nothing else in qemu does.

I don't find this commenting style particularly helpful since they 
really don't tell you anything you can't infer from the function name.  
I'm not opposed to people adding these types of comments but I don't 
think it's a good idea to attempt to enforce it in just this one place 
(not that I think it should be enforced everywhere).

Regards,

Anthony Liguori
> Kevin
>
>
>
Kevin Wolf - Nov. 12, 2009, 4:56 p.m.
Am 12.11.2009 17:46, schrieb Anthony Liguori:
> Kevin Wolf wrote:
>> Am 11.11.2009 18:28, schrieb Anthony Liguori:
>>   
>>> This makes lists no longer invariant. It's a very useful bit of functionality
>>> though.
>>>
>>> To deal with the fact that lists are no longer invariant, introduce a deep
>>> copy mechanism for lists.
>>>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>>  qlist.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  qlist.h |    4 ++++
>>>  2 files changed, 60 insertions(+), 0 deletions(-)
>>>     
>>
>> So far all functions in qlist.c have a header comment. Any reason to
>> change this?
>>   
> 
> But nothing else in qemu does.

Unfortunately. There are places where such comments could be a good
specification on what an interface is actually meant to work like
(particularly in error cases). Currently you often can't tell if the
implementation or the caller of a function is buggy.

Not sure if they are really useful for the simple qlist.c functions (but
even there the function name does not tell me what it's doing with NULL
parameters), but it might be helpful to have a general discussion about
it. I think in general qemu is poorly commented.

Kevin
Anthony Liguori - Nov. 12, 2009, 5:13 p.m.
Kevin Wolf wrote:
> Unfortunately. There are places where such comments could be a good
> specification on what an interface is actually meant to work like
> (particularly in error cases). Currently you often can't tell if the
> implementation or the caller of a function is buggy.
>
> Not sure if they are really useful for the simple qlist.c functions (but
> even there the function name does not tell me what it's doing with NULL
> parameters), but it might be helpful to have a general discussion about
> it. I think in general qemu is poorly commented.
>   

I agree, but I don't think the solution is forcing boiler plate 
commenting styles.  I think what we could improve on is asking people to 
comment bits of code during review.

> Kevin
>
Ian Molton - Nov. 12, 2009, 5:17 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kevin Wolf wrote:
> I think in general qemu is poorly commented.

I think thats a bit of an understatement. in particular I'm finding the
options code to be a nightmare. Hopefully I'll be fixing some of that in
my upcomming submissions.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJK/EMMAAoJEFIjE1w7L6YH2BgP/17YUHz92sD001Q8VxETeTJS
RhKONA6tX4vmnKfUTkM9VrqV3Qv7lyhhGN2IdJulUpKEKRmoykMuPKopkaYKuCJ4
9WzxQOimAHQbZ9NrPYulafygCx4opiuzFenuCoiabzdN6qhT7zdBmhqIMH188f/G
W8Bc4c0eiPD0wWwaomr4VMh+BaJ2lZDElPNNz7FxoUAdoedoNs0Iyj8VxlBGzNSz
6IoIqCBLgTzMbSKLIeKZ3eKGK3AZDIPpWbA7qlsB2K2zUQW9nlNVgs8BbOihSTng
NN2+9e0/QQvYGa/xYaGc/gmXf9WZwhMncvhPyuwB6alsMNzNF6X3jfvqC6ZDkzq5
NVNI6ItN1cVvbJqTPLGQ6EHspO9Du8GssbQnPiM3wCTmsB9aAV65LOWyBxigAdz1
7gQvXxt2+pDduqahOdw3vc2DllhtkASSH1JIPM90Hc/H6qHCBdkQO3okqZr/rOm5
OiQwONQ84k5Pp6GRcuWPbl+Dnrs47P4N7wQ876CooqGxhn8z4JIk6jKpIPqsDWVn
rxWVF41Hbxr/92BVrxBqmAJlIaqUMnUhTfpiYPVdaKMcrRIDWXpDXQOlNrztdk3a
INcv4NhJ604ab5UpLO3WbLIJK4CTnRXldly++3DHM60im6fO1kCsq5sKe56V7lNi
WossoFOnMg5Xb9RBxWxo
=iEnN
-----END PGP SIGNATURE-----
Luiz Capitulino - Nov. 12, 2009, 5:20 p.m.
On Thu, 12 Nov 2009 11:13:45 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> Kevin Wolf wrote:
> > Unfortunately. There are places where such comments could be a good
> > specification on what an interface is actually meant to work like
> > (particularly in error cases). Currently you often can't tell if the
> > implementation or the caller of a function is buggy.
> >
> > Not sure if they are really useful for the simple qlist.c functions (but
> > even there the function name does not tell me what it's doing with NULL
> > parameters), but it might be helpful to have a general discussion about
> > it. I think in general qemu is poorly commented.
> >   
> 
> I agree, but I don't think the solution is forcing boiler plate 
> commenting styles.  I think what we could improve on is asking people to 
> comment bits of code during review.

 I've started adding comments like that because this is an API which
is probably going to be part of a library, as such it has to be properly
documented and very likely to be generated automatically by tools like
doxygen.

 If we do this we should be consistent and document everything which
is public, even simple cases.
Kevin Wolf - Nov. 13, 2009, 8:51 a.m.
Am 12.11.2009 18:13, schrieb Anthony Liguori:
> Kevin Wolf wrote:
>> Unfortunately. There are places where such comments could be a good
>> specification on what an interface is actually meant to work like
>> (particularly in error cases). Currently you often can't tell if the
>> implementation or the caller of a function is buggy.
>>
>> Not sure if they are really useful for the simple qlist.c functions (but
>> even there the function name does not tell me what it's doing with NULL
>> parameters), but it might be helpful to have a general discussion about
>> it. I think in general qemu is poorly commented.
>>   
> 
> I agree, but I don't think the solution is forcing boiler plate 
> commenting styles.  I think what we could improve on is asking people to 
> comment bits of code during review.

Be sure that I'll be asking for comments in qcow2 patches. I did in the
past and I'll continue to do so. ;-)

I agree that boiler plates are not going to help per se. But I think
what actually does play a role in commenting is what surrounding code
looks like and it's also habits. And it's probably not wrong to say that
we're currently in a habit of not documenting anything. If boiler plates
in some places are the price to get into a habit of documenting things,
I think considering to accept that price wouldn't be completely
unreasonable.

Another thing is that we could ask more often to move explanations from
mails into the code. The explanation often exists and sometimes we're
lucky enough that it ends up at least in the commit log, but there are
cases where it's in PATCH 0/n or buried in the corresponding mail thread.

Kevin

Patch

diff --git a/qlist.c b/qlist.c
index ba2c66c..5fccb7d 100644
--- a/qlist.c
+++ b/qlist.c
@@ -37,6 +37,23 @@  QList *qlist_new(void)
     return qlist;
 }
 
+static void qlist_copy_elem(QObject *obj, void *opaque)
+{
+    QList *dst = opaque;
+
+    qobject_incref(obj);
+    qlist_append_obj(dst, obj);
+}
+
+QList *qlist_copy(QList *src)
+{
+    QList *dst = qlist_new();
+
+    qlist_iter(src, qlist_copy_elem, dst);
+
+    return dst;
+}
+
 /**
  * qlist_append_obj(): Append an QObject into QList
  *
@@ -67,6 +84,45 @@  void qlist_iter(const QList *qlist,
         iter(entry->value, opaque);
 }
 
+QObject *qlist_pop(QList *qlist)
+{
+    QListEntry *entry;
+    QObject *ret;
+
+    if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) {
+        return NULL;
+    }
+
+    entry = QTAILQ_FIRST(&qlist->head);
+    QTAILQ_REMOVE(&qlist->head, entry, next);
+
+    ret = entry->value;
+    qemu_free(entry);
+
+    return ret;
+}
+
+QObject *qlist_peek(QList *qlist)
+{
+    QListEntry *entry;
+    QObject *ret;
+
+    if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) {
+        return NULL;
+    }
+
+    entry = QTAILQ_FIRST(&qlist->head);
+
+    ret = entry->value;
+
+    return ret;
+}
+
+int qlist_empty(const QList *qlist)
+{
+    return QTAILQ_EMPTY(&qlist->head);
+}
+
 /**
  * qobject_to_qlist(): Convert a QObject into a QList
  */
diff --git a/qlist.h b/qlist.h
index 3eb1eb8..afdc446 100644
--- a/qlist.h
+++ b/qlist.h
@@ -30,9 +30,13 @@  typedef struct QList {
         qlist_append_obj(qlist, QOBJECT(obj))
 
 QList *qlist_new(void);
+QList *qlist_copy(QList *src);
 void qlist_append_obj(QList *qlist, QObject *obj);
 void qlist_iter(const QList *qlist,
                 void (*iter)(QObject *obj, void *opaque), void *opaque);
+QObject *qlist_pop(QList *qlist);
+QObject *qlist_peek(QList *qlist);
+int qlist_empty(const QList *qlist);
 QList *qobject_to_qlist(const QObject *obj);
 
 #endif /* QLIST_H */