[libvirt] [libvirt-glib] Prefer 'for' over 'while'
From: "Zeeshan Ali (Khattak)" <zeeshanak@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
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@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/
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=e1c15dac18fd5fc3d7c2cb... 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
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
On Thu, Jan 26, 2012 at 11:19 AM, Christophe Fergeau <cfergeau@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
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@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
On Thu, Jan 26, 2012 at 5:09 PM, Christophe Fergeau <cfergeau@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@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
participants (3)
-
Christophe Fergeau -
Philipp Hahn -
Zeeshan Ali (Khattak)