[Freeciv-Dev] Re: (PR#9640) RfP: help_items_iterate isn't reentrant

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9640 >

Jason Short wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9640 >
> 
> help_items_iterate isn't reentrant.  You can't embed one iteration 
> inside another.
> 
> This probably isn't a big deal, except that this iterator is global and 
> it should be very easy to fix it.  We should just make it a speclist...

Here's a patch.

This isn't necessarily much safer but it is simpler and 50 lines shorter.

Note there are two iterators.  help_items_iterate is used by lots of GUI 
code so I didn't want to rename it; it also uses the single global list 
for its iteration.  help_list_iterate is the classical form that's only 
used in one place.

jason

? diff
Index: client/civclient.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/civclient.c,v
retrieving revision 1.193
diff -u -r1.193 civclient.c
--- client/civclient.c  2 Aug 2004 23:19:35 -0000       1.193
+++ client/civclient.c  9 Aug 2004 06:31:32 -0000
@@ -49,7 +49,7 @@
 #include "diplodlg_g.h"
 #include "goto.h"
 #include "gui_main_g.h"
-#include "helpdata.h"          /* boot_help_texts() */
+#include "helpdata.h"          /* load_help_texts() */
 #include "mapctrl_g.h"
 #include "mapview_g.h"
 #include "menu_g.h"
@@ -232,7 +232,8 @@
      have cosmetic effects only (eg city name suggestions).  --dwp */
   mysrand(time(NULL));
 
-  boot_help_texts();
+  init_help_texts();
+  load_help_texts();
   if (!tilespec_read_toplevel(tileset_name)) {
     /* get tile sizes etc */
     exit(EXIT_FAILURE);
@@ -398,7 +399,7 @@
       precalc_tech_data();
       update_research(game.player_ptr);
       role_unit_precalcs();
-      boot_help_texts();       /* reboot */
+      load_help_texts();       /* reload */
       update_unit_focus();
     }
     else if (client_state == CLIENT_PRE_GAME_STATE) {
Index: client/helpdata.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/helpdata.c,v
retrieving revision 1.70
diff -u -r1.70 helpdata.c
--- client/helpdata.c   8 May 2004 00:00:21 -0000       1.70
+++ client/helpdata.c   9 Aug 2004 06:31:32 -0000
@@ -47,49 +47,25 @@
 
 #define MAX_LAST (MAX(MAX(MAX(A_LAST,B_LAST),U_LAST),T_COUNT))
 
-#define SPECLIST_TAG help
-#define SPECLIST_TYPE struct help_item
-#include "speclist.h"
-
-#define help_list_iterate(helplist, phelp) \
-    TYPED_LIST_ITERATE(struct help_item, helplist, phelp)
-#define help_list_iterate_end  LIST_ITERATE_END
+#define help_list_iterate(helplist, pitem)                     \
+  TYPED_LIST_ITERATE(struct help_item, helplist, pitem)
+#define help_list_iterate_end LIST_ITERATE_END
 
-static struct genlist_link *help_nodes_iterator;
-static struct help_list help_nodes;
+struct help_list help_nodes;
 static bool help_nodes_init = FALSE;
-/* helpnodes_init is not quite the same as booted in boot_help_texts();
-   latter can be 0 even after call, eg if couldn't find helpdata.txt.
-*/
 
 char long_buffer[64000];
 
 /****************************************************************
-  Make sure help_nodes is initialised.
-  Should call this just about everywhere which uses help_nodes,
-  to be careful...  or at least where called by external
-  (non-static) functions.
-*****************************************************************/
-static void check_help_nodes_init(void)
-{
-  if (!help_nodes_init) {
-    help_list_init(&help_nodes);
-    help_nodes_init = TRUE;    /* before help_iter_start to avoid recursion! */
-    help_iter_start();
-  }
-}
-
-/****************************************************************
   Free all allocations associated with help_nodes.
 *****************************************************************/
 void free_help_texts(void)
 {
-  check_help_nodes_init();
-  help_list_iterate(help_nodes, ptmp) {
+  help_items_iterate(ptmp) {
     free(ptmp->topic);
     free(ptmp->text);
     free(ptmp);
-  } help_list_iterate_end;
+  } help_items_iterate_end;
   help_list_unlink_all(&help_nodes);
 }
 
@@ -171,12 +147,19 @@
 }
 
 /****************************************************************
-...
+  Initialize the help texts.
 *****************************************************************/
-void boot_help_texts(void)
+void init_help_texts(void)
 {
-  static bool booted = FALSE;
+  help_list_init(&help_nodes);
+  help_nodes_init = TRUE;
+}
 
+/****************************************************************
+...
+*****************************************************************/
+void load_help_texts(void)
+{
   struct section_file file, *sf = &file;
   char *filename;
   struct help_item *pitem;
@@ -184,18 +167,14 @@
   char **sec, **paras;
   int nsec, npara;
 
-  check_help_nodes_init();
-
   /* need to do something like this or bad things happen */
   popdown_help_dialog();
 
-  if(!booted) {
-    freelog(LOG_VERBOSE, "Booting help texts");
-  } else {
-    /* free memory allocated last time booted */
-    free_help_texts();
-    freelog(LOG_VERBOSE, "Rebooting help texts");
-  }    
+  assert(help_nodes_init);
+
+  /* free memory allocated last time loaded */
+  free_help_texts();
+  freelog(LOG_VERBOSE, "Rebooting help texts");
 
   filename = datafilename("helpdata.txt");
   if (!filename) {
@@ -217,9 +196,6 @@
     
     if (gen_str) {
       enum help_page_type current_type = HELP_ANY;
-      if (!booted) {
-       continue; /* on initial boot data tables are empty */
-      }
       for(i=2; help_type_names[i]; i++) {
        if(strcmp(gen_str, help_type_names[i])==0) {
          current_type = i;
@@ -352,7 +328,6 @@
   sec = NULL;
   section_file_check_unused(sf, sf->filename);
   section_file_free(sf);
-  booted = TRUE;
   freelog(LOG_VERBOSE, "Booted help texts ok");
 }
 
@@ -368,7 +343,6 @@
 *****************************************************************/
 int num_help_items(void)
 {
-  check_help_nodes_init();
   return help_list_size(&help_nodes);
 }
 
@@ -381,7 +355,6 @@
 {
   int size;
   
-  check_help_nodes_init();
   size = help_list_size(&help_nodes);
   if (pos < 0 || pos > size) {
     freelog(LOG_ERROR, "Bad index %d to get_help_item (size %d)", pos, size);
@@ -408,7 +381,6 @@
   static char vtopic[128];
   static char vtext[256];
 
-  check_help_nodes_init();
   idx = 0;
   help_list_iterate(help_nodes, ptmp) {
     char *p=ptmp->topic;
@@ -446,34 +418,6 @@
 }
 
 /****************************************************************
-  Start iterating through help items;
-  that is, reset iterator to start position.
-  (Could iterate using get_help_item(), but that would be
-  less efficient due to scanning to find pos.)
-*****************************************************************/
-void help_iter_start(void)
-{
-  check_help_nodes_init();
-  help_nodes_iterator = help_nodes.list.head_link;
-}
-
-/****************************************************************
-  Returns next help item; after help_iter_start(), this is
-  the first item.  At end, returns NULL.
-*****************************************************************/
-const struct help_item *help_iter_next(void)
-{
-  const struct help_item *pitem;
-  
-  check_help_nodes_init();
-  pitem = help_nodes_iterator->dataptr;
-  help_nodes_iterator = help_nodes_iterator->next;
-
-  return pitem;
-}
-
-
-/****************************************************************
   FIXME:
   All these helptext_* functions have a pretty crappy interface:
   we just write to buf and hope that its long enough.
Index: client/helpdata.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/helpdata.h,v
retrieving revision 1.7
diff -u -r1.7 helpdata.h
--- client/helpdata.h   28 Sep 2002 01:36:20 -0000      1.7
+++ client/helpdata.h   9 Aug 2004 06:31:32 -0000
@@ -20,7 +20,17 @@
   enum help_page_type type;
 };
 
-void boot_help_texts(void);
+/* Get 'struct help_item_list' and related functions: */
+#define SPECLIST_TAG help
+#define SPECLIST_TYPE struct help_item
+#include "speclist.h"
+
+#define help_items_iterate(pitem)                      \
+  TYPED_LIST_ITERATE(struct help_item, help_nodes, pitem)
+#define help_items_iterate_end LIST_ITERATE_END
+
+void init_help_texts(void);
+void load_help_texts(void);
 void free_help_texts(void);
 
 int num_help_items(void);
@@ -28,8 +38,6 @@
 const struct help_item *get_help_item_spec(const char *name,
                                           enum help_page_type htype,
                                           int *pos);
-void help_iter_start(void);
-const struct help_item *help_iter_next(void);
 
 void helptext_improvement(char *buf, int which, const char *user_text);
 void helptext_wonder(char *buf, int which, const char *user_text);
@@ -40,12 +48,7 @@
 
 char *helptext_unit_upkeep_str(int i);
 
-#define help_items_iterate(pitem) {       \
-        const struct help_item *pitem;    \
-        help_iter_start();                \
-        while((pitem=help_iter_next())) {   
-#define help_items_iterate_end }}
-
 extern char long_buffer[64000];
+extern struct help_list help_nodes;
 
 #endif  /* FC__HELPDATA_H */
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.393
diff -u -r1.393 packhand.c
--- client/packhand.c   2 Aug 2004 16:59:14 -0000       1.393
+++ client/packhand.c   9 Aug 2004 06:31:35 -0000
@@ -53,7 +53,7 @@
 #include "goto.h"               /* client_goto_init() */
 #include "graphics_g.h"
 #include "gui_main_g.h"
-#include "helpdata.h"          /* boot_help_texts() */
+#include "helpdata.h"          /* load_help_texts() */
 #include "inteldlg_g.h"
 #include "mapctrl_g.h"         /* popup_newcity_dialog() */
 #include "mapview_g.h"
@@ -1346,7 +1346,7 @@
 void handle_game_info(struct packet_game_info *pinfo)
 {
   int i;
-  bool boot_help, need_effect_update = FALSE;
+  bool load_help, need_effect_update = FALSE;
 
   game.gold=pinfo->gold;
   game.tech=pinfo->tech;
@@ -1403,7 +1403,7 @@
   game.unhappysize = pinfo->unhappysize;
   game.cityfactor = pinfo->cityfactor;
 
-  boot_help = (can_client_change_view()
+  load_help = (can_client_change_view()
               && game.spacerace != pinfo->spacerace);
   game.spacerace=pinfo->spacerace;
   if (game.timeout != 0) {
@@ -1411,8 +1411,8 @@
       seconds_to_turndone = pinfo->seconds_to_turndone;
   } else
     seconds_to_turndone = 0;
-  if (boot_help) {
-    boot_help_texts();         /* reboot, after setting game.spacerace */
+  if (load_help) {
+    load_help_texts();         /* reboot, after setting game.spacerace */
   }
 
   update_unit_focus();