9:10 a.m.
From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org>
In this particular case 'for' seems like a more natural choice as then
we don't need to update the iterator (which we were forgetting to do and
causing a hang in Boxes).
---
libvirt-gconfig/libvirt-gconfig-helpers.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-helpers.c
b/libvirt-gconfig/libvirt-gconfig-helpers.c
index c406a49..cc2e5cc 100644
--- a/libvirt-gconfig/libvirt-gconfig-helpers.c
+++ b/libvirt-gconfig/libvirt-gconfig-helpers.c
@@ -169,17 +169,14 @@ void gvir_config_xml_foreach_child(xmlNodePtr node,
g_return_if_fail(iter_func != NULL);
- it = node->children;
- while (it != NULL) {
+ for (it = node->children; it != NULL; it = it->next) {
gboolean cont;
- xmlNodePtr next = it->next;
if (xmlIsBlankNode(it))
continue;
cont = iter_func(it, opaque);
if (!cont)
break;
- it = next;
}
}
--
1.7.7.5
11:18 a.m.
Hello,
On Thursday 26 January 2012 06:10:28 Zeeshan Ali (Khattak) wrote:
- it = node->children;
- while (it != NULL) {
+ for (it = node->children; it != NULL; it = it->next) {
...
- xmlNodePtr next = it->next;
...
cont = iter_func(it, opaque);
...
- it = next;
Your changed version only has the same
behaviour, if the user-passed-in
function iter_func() never changes it->next, which you can't guarentee here.
You need to keep the "next" copy.
BYtE
Philipp
--
Philipp Hahn Open Source Software Engineer hahn(a)univention.de
Univention GmbH Linux for Your Business fon: +49 421 22 232- 0
Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99
http://www.univention.de/
1:19 p.m.
On Thu, Jan 26, 2012 at 08:18:38AM +0100, Philipp Hahn wrote:
Your changed version only has the same behaviour, if the
user-passed-in
function iter_func() never changes it->next, which you can't guarentee here.
You need to keep the "next" copy.
Yes, the for loop was changed to a while loop recently exactly for that
purpose:
commit e1c15dac18fd5fc3d7c2cbfebb0402c05cc3cee3
http://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=e1c15dac18fd5fc3d7c...
Refactor gvir_config_object_delete_child
By changing gvir_config_xml_foreach_child to make it robust against
current node deletion in the "foreach" callback, we can use
gvir_config_object_foreach_child to implement
gvir_config_object_delete_child
Christophe
1:25 p.m.
New subject: [libvirt] [libvirt-glib] Correctly handle blank nodes in _foreach_child
When encountering a blank node, the child iterator wasn't set to
point to the next node leading to an infinite loop.
---
libvirt-gconfig/libvirt-gconfig-helpers.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-helpers.c
b/libvirt-gconfig/libvirt-gconfig-helpers.c
index 6500439..071a1e7 100644
--- a/libvirt-gconfig/libvirt-gconfig-helpers.c
+++ b/libvirt-gconfig/libvirt-gconfig-helpers.c
@@ -172,14 +172,14 @@ void gvir_config_xml_foreach_child(xmlNodePtr node,
it = node->children;
while (it != NULL) {
- gboolean cont;
xmlNodePtr next = it->next;
- if (xmlIsBlankNode(it))
- continue;
- cont = iter_func(it, opaque);
- if (!cont)
- break;
+ if (!xmlIsBlankNode(it)) {
+ gboolean cont;
+ cont = iter_func(it, opaque);
+ if (!cont)
+ break;
+ }
it = next;
}
}
--
1.7.7.6
7:05 p.m.
On Thu, Jan 26, 2012 at 11:19 AM, Christophe Fergeau
<cfergeau(a)redhat.com> wrote:
On Thu, Jan 26, 2012 at 08:18:38AM +0100, Philipp Hahn wrote:
> Your changed version only has the same behaviour, if the user-passed-in
> function iter_func() never changes it->next, which you can't guarentee here.
> You need to keep the "next" copy.
Yes, the for loop was changed to a while loop recently exactly for that
purpose:
I don't get it, your commit clearly introduced a hang in Boxes and my
change fixes it back while also simplifying the code slightly. So I
don't at all buy the 'while' being more 'reliable'.
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124
7:09 p.m.
On Thu, Jan 26, 2012 at 05:05:15PM +0200, Zeeshan Ali (Khattak) wrote:
On Thu, Jan 26, 2012 at 11:19 AM, Christophe Fergeau
<cfergeau(a)redhat.com> wrote:
> On Thu, Jan 26, 2012 at 08:18:38AM +0100, Philipp Hahn wrote:
>> Your changed version only has the same behaviour, if the user-passed-in
>> function iter_func() never changes it->next, which you can't guarentee
here.
>> You need to keep the "next" copy.
>
> Yes, the for loop was changed to a while loop recently exactly for that
> purpose:
I don't get it, your commit clearly introduced a hang in Boxes and my
change fixes it back while also simplifying the code slightly. So I
don't at all buy the 'while' being more 'reliable'.
The callback gets passed 'it', and the callback used in
gvir_config_object_delete_child can unlink/free 'it', so things won't work
as expected if you try to get it->next after calling the callback. Using a
while loop and getting it->next before calling the callback avoids this
problem.
Christophe
9:03 p.m.
On Thu, Jan 26, 2012 at 5:09 PM, Christophe Fergeau <cfergeau(a)redhat.com> wrote:
On Thu, Jan 26, 2012 at 05:05:15PM +0200, Zeeshan Ali (Khattak)
wrote:
> On Thu, Jan 26, 2012 at 11:19 AM, Christophe Fergeau
> <cfergeau(a)redhat.com> wrote:
> > On Thu, Jan 26, 2012 at 08:18:38AM +0100, Philipp Hahn wrote:
> >> Your changed version only has the same behaviour, if the user-passed-in
> >> function iter_func() never changes it->next, which you can't
guarentee here.
> >> You need to keep the "next" copy.
> >
> > Yes, the for loop was changed to a while loop recently exactly for that
> > purpose:
>
> I don't get it, your commit clearly introduced a hang in Boxes and my
> change fixes it back while also simplifying the code slightly. So I
> don't at all buy the 'while' being more 'reliable'.
The callback gets passed 'it', and the callback used in
gvir_config_object_delete_child can unlink/free 'it', so things won't work
as expected if you try to get it->next after calling the callback. Using a
while loop and getting it->next before calling the callback avoids this
problem.
Ah, missed that somehow. :) Sent another fix for the infinite loop,
please check it out.
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124
4833
days inactive
4833
days old
6 comments
3 participants
participants (3)
-
Christophe Fergeau -
Philipp Hahn -
Zeeshan Ali (Khattak)