Re: store files - store_load

attempted dif -u  for some store changes...........

bb_store.c

@@ -484,33 +435,32 @@
     long end, pos;

     if (filename == NULL)
-               return 0;
-
+       return 0;

     mutex_lock(file_mutex);
     if (file != NULL) {
-               fclose(file);
-               file = NULL;
+       fclose(file);
+       file = NULL;
     }

     store_file = octstr_read_file(octstr_get_cstr(filename));
-    if (store_file != NULL && octstr_len(store_file)!=0)
-               info(0, "Loading store file `%s'",
octstr_get_cstr(filename));
+    if (store_file != NULL)
+       info(0, "Loading store file `%s'", octstr_get_cstr(filename));
     else {
-               store_file = octstr_read_file(octstr_get_cstr(newfile));
-               if (store_file != NULL && octstr_len(store_file)!=0)
-                       info(0, "Loading store file `%s'",
octstr_get_cstr(newfile));
-               else {
-                       store_file =
octstr_read_file(octstr_get_cstr(bakfile));
-                       if (store_file != NULL)
-                               info(0, "Loading store file `%s'",
octstr_get_cstr(bakfile));
-                       else {
-                               info(0, "Cannot open any store file,
starting a new one");
-                               retval = open_file(filename);
-                               mutex_unlock(file_mutex);
-                               return retval;
-                       }
-               }
+       store_file = octstr_read_file(octstr_get_cstr(newfile));
+       if (store_file != NULL)
+           info(0, "Loading store file `%s'", octstr_get_cstr(newfile));
+       else {
+           store_file = octstr_read_file(octstr_get_cstr(bakfile));
+           if (store_file != NULL)
+               info(0, "Loading store file `%s'",
octstr_get_cstr(bakfile));
+           else {
+               info(0, "Cannot open any store file, starting a new one");
+               retval = open_file(filename);
+               mutex_unlock(file_mutex);
+               return retval;
+           }
+       }
     }

     info(0, "Store-file size %ld, starting to unpack%s",
octstr_len(store_file),
@@ -522,33 +472,33 @@

     while ((end = octstr_search_char(store_file, '\n', pos)) != -1) {

-               pack = octstr_copy(store_file, pos, end-pos);
-               pos = end+1;
+       pack = octstr_copy(store_file, pos, end-pos);
+       pos = end+1;
+
+       if (octstr_url_decode(pack) == -1) {
+           debug("bb.store", 0, "Garbage at store-file, skipped");
+            octstr_destroy(pack);
+           continue;
+       }
+
+       msg = msg_unpack(pack);
+       octstr_destroy(pack);
+       if (msg == NULL) {
+           continue;
+        }

-               if (octstr_url_decode(pack) == -1) {
-                       debug("bb.store", 0, "Garbage at store-file,
skipped");
-                               octstr_destroy(pack);
-                       continue;
-               }
-
-               msg = msg_unpack(pack);
-               octstr_destroy(pack);
-               if (msg == NULL) {
-                       continue;
-                       }
-
-               if (msg_type(msg) == sms) {
-                       store_to_dict(msg);
-                       msgs++;
-               }
-               else if (msg_type(msg) == ack) {
-                       store_to_dict(msg);
-               } else {
-                       warning(0, "Strange message in store-file,
discarded, "
-                               "dump follows:");
-                       msg_dump(msg, 0);
-                       msg_destroy(msg);
-               }
+       if (msg_type(msg) == sms) {
+           store_to_dict(msg);
+           msgs++;
+       }
+       else if (msg_type(msg) == ack) {
+           store_to_dict(msg);
+       } else {
+           warning(0, "Strange message in store-file, discarded, "
+                   "dump follows:");
+           msg_dump(msg, 0);
+           msg_destroy(msg);
+       }
     }
     octstr_destroy(store_file);

@@ -632,25 +582,20 @@
     sms_dict = dict_create(201, msg_destroy_item);

     if (dump_freq > 0)
-               dump_frequency = dump_freq;
+       dump_frequency = dump_freq;
     else
-               dump_frequency = BB_STORE_DEFAULT_DUMP_FREQ;
+       dump_frequency = BB_STORE_DEFAULT_DUMP_FREQ;

     file_mutex = mutex_create();
-    return 0;
-}
-
-
-void store_start(void)
-{
     active = 1;

     if ((cleanup_thread = gwthread_create(store_dumper, NULL))==-1)
-               panic(0, "Failed to create a cleanup thread!");
-
+       panic(0, "Failed to create a cleanup thread!");

+    return 0;
 }


bearerbox.c


@@ -606,23 +579,18 @@
     gwthread_sleep(5.0); /* give time to threads to register themselves */

     if (store_load()== -1)
-               panic(0, "Cannot start with store-file failing");
-
+       panic(0, "Cannot start with store-file failing");

-    /* start the store file thread*/
-       store_start();
-


sorry, looks a mess, essentially the store dumper thread is not started in
the store_init,
but later in store_start()



----- Original Message ----- 
From: "fred" <[email protected]>
To: "devel" <[email protected]>; "Alexander Malysh" <[email protected]>
Sent: Friday, October 07, 2005 11:50 AM
Subject: Re: store files - store_load -#- MailID:PAAA


>
> ----- Original Message ----- 
> From: "Alexander Malysh" <[email protected]>
> To: <[email protected]>
> Sent: Thursday, October 06, 2005 11:50 PM
> Subject: Re: store files - store_load -#- MailID:PAAA
>
>
> > Hi,
> >
> > fred wrote:
> > > Hi devel groupers
> > >
> > >
> > > I have been testing with a few fail scenarios lately, and the way the
> > > code is currently doesn;t
> > > quite do it for me, strange because much of the way its coded has been
> > > like that for quite some time......
> > >
> > > 1.) Firstly during init_bearerbox, the store_init - which starts up
the
> > > store_dumper thread,
> > > which has a tendency to run the store_dump() and thus do_dump and
rename
> > > the say
> > > "kannel.store" to "kannel.store.bak"
> > > and the new store is renamed to "kannel.store"
> >
> > you are right, it's a race condition here (but unfortunately not only
> > one in store-file support)...
> >
> > >
> > > 2.) now the store_load() is run, the "kannel.store" now read is
> > > typically 0 length
> > >   I have actually changed the code to like
> > >    if (store_file != NULL && octstr_len(store_file)!=0)
> > >   info(0, "Loading store file `%s'", octstr_get_cstr(filename));
> > >     else {
> > >   store_file = octstr_read_file(octstr_get_cstr(newfile));
> > > so it gets to "kannel.store.bak" and loads these messages,
> >
> > it's workaround but not properly fix.
> agree, which is why the following is best
> >
> >
> > >
> > > 3.) however, rationally I think the order of how its happening is all
> > > wrong, and would rather have
> > > store_load as deterministically being the first thing to run which is
> > > how i am changing it to.
> > >
> >
> > the order is not wrong. Only thing that I would change, is: start dumper
> > thread after store_load completed, means at the and of store_load
> function.
>
> Yes, thats what I meant and did and works much better.
>
>
> >
> > > 4.) theres a few other things i'm going to say about message
> > > acknowledgement...but I'll make that another subject
> > > ITNTDF
> >
> > hmm, interesting to hear about...
> >
> > >
> > > I would like to know what current scenario of recovery the existing
code
> > > has  been tested for..??? anyone?
> > >
> >
> > it was tested for most cases but it's not bug free! I personally know
> > some strange race conditions within store-file support that not too hard
> > to fix and will be fixed as soon I have time for it hopefully by the end
> > of this week :-) In the meantime you could test my new store spool
> > replacement that was posted to this list.
> >
> > > Anyway.......I written this stuff here so you can comment on what I
> > > said......I changed stuff anyway for our version........
> >
> > hmm, it's open source if you still didn't get it... so patch is
welcome...
>
> hmm,yeh but making a diff -u that will make sense to you is a bit of a
> problem...after
> doing custom stuff, and I tend to reformat the code in places I need to be
> able to read and follow it.
>
>
> >
> > Thanks,
> > Alex
> >
> > >
> > >
> > > Cheers
> > >
> > >
> >
> >
>
>