There were some trivial and some non-trivial issues. Glad the compiled picked up on
this!

> I see loads of warnings during compile, most of which I know from earlier 
> compiles:
> * warning: dereferencing type-punned pointer will break strict-aliasing rules
> * warning: pointer targets in passing argument 3 of 'getsockname' differ in
>            signedness

I ignore these for the moment until I have a gcc 4.2 myself (we use Scientific
Linux 5 which has gcc 4.1 for the moment). As Randolph pointed out correctly you
can make gcc shut up by a proper flag there. The warnings have no influence on the
stability of midas.

> (1)=========================
> src/midas.c:7398: warning: array subscript is above array bounds
> Inspection of midas.c:
> 
>    if (i == MAX_DEFRAG_EVENTS) {
>       /* no buffer available -> no first fragment received */
> 7398: free(defrag_buffer[i].pevent);
>       memset(&defrag_buffer[i].event_id, 0, sizeof(EVENT_DEFRAG_BUFFER));
>       cm_msg(MERROR, "bm_defragement_event",
>              "Received fragment without first fragment (ID %d) Ser#:%d",
>              pevent->event_id & 0x0FFF, pevent->serial_number);
>       return;
>    }

The free() was just wrong at that place, I removed it.

> (2)==========================
> src/midas.c:2958: warning: array subscript is above array bounds
> 
>    for (i = 0; i < 13; i++)
> 2958  if (trans_name[i].transition == transition)
>          break;

Fixed that by
     
     for (i=0 ;; i++)
        if (trans_name[i].name[0] == 0 || trans_name[i].transition == transition)
           break;

Since trans_name[i].name = "" indicates the end of the list.

> (3)=============================
> mfe.c:
> src/mfe.c:412: warning: array subscript is above array bounds
> src/mfe.c:311: warning: array subscript is above array bounds
> src/mfe.c:340: warning: array subscript is above array bounds
> 
> 412: device_drv->dd(CMD_GET_DEMAND, device_drv->dd_info, i, 
>           &device_drv->mt_buffer->channel[i].array[CMD_GET_DEMAND]);

The code at 412 was wrong there, the demand value is queried later by the device
driver directly. For the other two occurences (311 and 340) I had to really
increase the array size by one. This issue can cause segfaults if you have a slow
control front-end which uses multithreading (not many people use it except me).

> (4)=========================
> src/lazylogger.c:1957: warning: array subscript is below array bounds
> 
> if ((channel < 0) && (lazyinfo[channel].hKey != 0))
> 
> That is lazyinfo[something below zero].

This has to be fixed by Pierre. I guess an or instead of an and would do it, but
I'm not 100% sure.

> (5)=============================
> More warnings an expert might want to have a look at:
> 
> * warning: deprecated conversion from string constant to 'char*'
> 
> * src/fal.c:106: warning: non-local variable '<anonymous struct> out_info'
>                  uses anonymous type
> * src/fal.c:3064: warning: non-local variable '<anonymous struct> eb' uses
>                   anonymous type
> 
> I attach the full output of make.
> Could someone knowledgeable please have a look at these warnings and fix them?

Uahhh. Especially the "const char*" vs. "char*" is in principle right, but will
cause a major rework. Probably hundreds of occations have to be fixed. Many strings
must be declared const, others not. It will help the programmer to find some errors
during compile which would later show up only during runtime (like writing into a
fixed string), but I only will go through that when I have gcc 4.2 installed
myself, and have two free days to work on this ;-)

> They make me a bit nervous when thinking about data integrity, and
> there are now so many that they actually start to hide serious stuff
> like the ones I presented.

Except the slow control stuff (which only is an issue for multithreaded frontends)
none of the above things will have an influence on the data integrity. But I agree
that they should be fixed.

- Stefan

Here is the list of known problems I am aware of and of fixes not yet committed
to midas svn:

Here is the list of known problems I am aware of and of fixes not yet committed
to midas svn:

1) added variable /equiment/foo/common/PerVariableHistory breaks stuff (mostly
mhttpd). It is not clear how this problem escaped my pre-commit checks. This
per-equipment variable enables the per-variable history for the given equipment.
Local consensus is that this variable should not be in "common" and should not
be in "settings". Probably in "/history"? Or have only one variable to enable
this for all equipments at once (like we do in ALPHA).

2) writing compressed midas files (foo.mid.gz) crashes the mlogger when file
size reaches 2 GBytes. This problem could be new in SL5.1.

3) when a midas client becomes unresponsive, runs cannot be stopped using the
"stop" button in mhttpd. This is because cm_transition() loops over all attached
clients, but never removes clients that are known to be dead. Proposed fix is to
call cm_check_client() for each client before calling their rpc transition handler.

4) the discussed before fix for reading broken history files (skip bad data).

5) mhttpd history "export" button needs to be fixed (by request from ALPHA). At
present it either does not return all exiting data or crashes mhttpd. (no fix)

6) mhttpd ODB editor in "set value" page, the "cancel" button is broken (needs
to be corrected for "relative URL"). (no fix)

7) mhttpd needs AJAX-style methods for reading and writing ODB. (no fix)

K.O.

> Here is the list of known problems I am aware of and of fixes not yet committed
> to midas svn:
> 
> 1) added variable /equiment/foo/common/PerVariableHistory

corrected in svn revision 4203, read
http://savannah.psi.ch/viewcvs/trunk/src/mlogger.c?root=midas&rev=4203&sortby=rev&view=log

> 2) writing compressed midas files (foo.mid.gz) crashes the mlogger when file
> size reaches 2 GBytes. This problem could be new in SL5.1.

(no change)

> 3) when a midas client becomes unresponsive, runs cannot be stopped using the
> "stop" button in mhttpd. This is because cm_transition() loops over all attached
> clients, but never removes clients that are known to be dead. Proposed fix is to
> call cm_check_client() for each client before calling their rpc transition handler.

Fixed in SVN revision 4198, read
http://savannah.psi.ch/viewcvs/trunk/src/midas.c?root=midas&rev=4201&sortby=rev&view=log

> 4) the discussed before fix for reading broken history files (skip bad data).

Fixed in SVN revision 4202, read https://ladd00.triumf.ca/elog/Midas/482

> 5) mhttpd history "export" button needs to be fixed (by request from ALPHA). At
> present it either does not return all exiting data or crashes mhttpd. (no fix)

(no change)

> 6) mhttpd ODB editor in "set value" page, the "cancel" button is broken (needs
> to be corrected for "relative URL").

Apply this patch to src/mhttpd.c

@@ -11156,10 +11190,7 @@
          sprintf(str, "SC/%s/%s", eq_name, group);
          redirect(str);
       } else {
-         strlcpy(str, path, sizeof(str));
-         if (strrchr(str, '/'))
-            strlcpy(str, strrchr(str, '/')+1, sizeof(str));
-         redirect(str);
+         redirect("./");
       }

> 7) mhttpd needs AJAX-style methods for reading and writing ODB. (no fix)

(no change)

K.O.

> > Here is the list of known problems I am aware of and of fixes not yet committed to midas svn:
> > 1) added variable /equiment/foo/common/PerVariableHistory
> 
> corrected in svn revision 4203, read
> http://savannah.psi.ch/viewcvs/trunk/src/mlogger.c?root=midas&rev=4203&sortby=rev&view=log

Was still broken - all should work in revision 4207.

> > 2) writing compressed midas files (foo.mid.gz) crashes the mlogger when file
> > size reaches 2 GBytes. This problem could be new in SL5.1.

It turns out that on SL5 and SL5.1 (and others?) the 32-bit version of ZLIB opens the
compressed output file without the O_LARGEFILE flag, this limits the file size to 2 GB.

Fixed by opening the file ourselves, then attach compression stream using gzdopen().

Revision 4207. (Not tested on Windows - may be broken!)

> > 5) mhttpd history "export" button needs to be fixed (by request from ALPHA). At
> > present it either does not return all exiting data or crashes mhttpd. (no fix)
> 
> (no change)
> 
> > 6) mhttpd ODB editor in "set value" page, the "cancel" button is broken (needs
> > to be corrected for "relative URL").
> 
> Apply this patch to src/mhttpd.c
> 
> @@ -11156,10 +11190,7 @@
>           sprintf(str, "SC/%s/%s", eq_name, group);
>           redirect(str);
>        } else {
> -         strlcpy(str, path, sizeof(str));
> -         if (strrchr(str, '/'))
> -            strlcpy(str, strrchr(str, '/')+1, sizeof(str));
> -         redirect(str);
> +         redirect("./");
>        }
> 
> > 7) mhttpd needs AJAX-style methods for reading and writing ODB. (no fix)
> 
> (no change)
> 
> K.O.

mhttpd "messages" page stopped working after svn revision 4327 because of uninitialized variable 
"filename2" in midas.c:cm_message_retrieve(). Attached patch fixes the problem for me.
K.O.


--- src/midas.c (revision 4342)
+++ src/midas.c (working copy)
@@ -978,6 +978,8 @@
       size = sizeof(filename);
       db_get_value(hDB, 0, "/Logger/Message file", filename, &size, TID_STRING, TRUE);
 
+      strlcpy(filename2, filename, sizeof(filename2));
+
       if (strchr(filename, '%')) {
          /* replace strings such as midas_%y%m%d.mid with current date */
          tzset();

> mhttpd "messages" page stopped working after svn revision 4327 because of uninitialized variable 
> "filename2" in midas.c:cm_message_retrieve(). Attached patch fixes the problem for me.
> K.O.
> 
> 
> --- src/midas.c (revision 4342)
> +++ src/midas.c (working copy)
> @@ -978,6 +978,8 @@
>        size = sizeof(filename);
>        db_get_value(hDB, 0, "/Logger/Message file", filename, &size, TID_STRING, TRUE);
>  
> +      strlcpy(filename2, filename, sizeof(filename2));
> +
>        if (strchr(filename, '%')) {
>           /* replace strings such as midas_%y%m%d.mid with current date */
>           tzset();

Ups, was my fault, sorry. I committed your change.

I am using an external Custom web page via a link in the ODB in /Custom, and
Javascript to add customscript button(s) and run start/stop buttons.

After executing these buttons, instead of returning to the custom page, or
to the Midas main status page, there is an error page generated:

Invalid custom page: NULL path
and the URL is 

http://lxfred:8082/CS/

The behaviour is the same whether the custom page replaces the main status page
or not.

I am using
MIDAS version 2.0.0
mhttpd.c SVN Rev 4282

In an older version of mhttpd.c, buttons of this type used to return to the
Midas main status page regardless of whether the custom page replaced the status
page. I found this behaviour annoying, and I made a custom mhttpd.c that
returned to the custom page. 
Would it be possible to fix this problem, and to return to the custom page after
pressing the buttons?


Here is the Javascript to add the buttons:

<script type="text/javascript">
var rstate = '<odb src="/runinfo/run state">'

 if (rstate == 1) // stopped
    document.write('<input name="cmd" value="Start" type="submit">')
 else if (rstate == 2 // paused
    document.write('<input name="cmd" value="Resume" type="submit">')
 else  // running
 {
    document.write('<input name="cmd" value="Stop" type="submit">')
    document.write('<input name="cmd" value="Pause" type="submit">')
 }

 if (rstate == 1) // stopped
    document.write('<input name="customscript" value="tri_config" type="submit">');
</script>

In midas.c there are several places where client last activity time stamps are checked against the 
watchdog timeout and the clients are declared dead if they fail to update their activity time stamps. 
ODB time stamps and data buffer time stamps appear to be handled in a similar manner.

Most checks are done like this:

now = ss_millitime();
if (client->watchdog > 0      <----- check that the watchdog is enabled
    && now > client->last_activity    <---- check for crazy time stamps from the future
    && now - client->last_activity > client->watchdog_timeout)   <--- normal timeout
        remove_client(client);

But in a few places, the extra checks are missing:

now = ss_millitime();
if (now - client->last_activity > client->watchdog_timeout)
        remove_client(client);

Is this an oversight from when additional checks were added?
Should I make all checks read like the first one?

K.O.

When I run odbedit remotely (odbedit -h ladd09), the "cleanup" command unexpectedly produces the 
output of the "sor" command (sure enough, there is a call to db_get_open_records() there), but when I run 
it locally, I do not get this output (but db_get_open_records() is still called). Strange. K.O.

When mserver receives events from remote client, writes them into a data buffer and this data buffer 
becomes 100% full, we see mserver go into 100% consumption.

It turns out this happens because bm_wait_for_free_space() never sleeps, instead, it busy-loops waiting 
for free space. bm_wait_for_free_space() does call ss_suspend(), but ss_suspend() does not sleep 
because there is pending data in the event network connection and it want to process it.

Best solution I have is to use silly "if (ss_suspend()!=SS_TIMEOUT) sleep(1);"

Also read this explanation: (bm_cleanup is needed to detect that the client holding the buffer at 100% 
full (a stuck or dead GET_ALL reader, mevb in our case), has been killed off and we can continue as 
usual)

       /* signal other clients wait mode */
       pheader->client[bm_validate_client_index(pbuf)].write_wait = requested_space;
 
+      bm_cleanup("bm_wait_for_free_space", ss_millitime(), FALSE);
+
       status = ss_suspend(1000, MSG_BM);
 
+      /* make sure we do sleep in this loop:
+       * if we are the mserver receiving data on the event
+       * socket and the data buffer is full, ss_suspend() will
+       * never sleep: it will detect data on the event channel,
+       * call rpc_server_receive() (recursively, we already *are* in
+       * rpc_server_receive()) and return without sleeping. Result
+       * is a busy loop waiting for free space in data buffer */
+      if (status != SS_TIMEOUT)
+         sleep(1);
+
       /* validate client index: we could have been removed from the buffer */
       pheader->client[bm_validate_client_index(pbuf)].write_wait = 0;

K.O.

> In midas.c there are several places where client last activity time stamps are checked against the 
> watchdog timeout and the clients are declared dead if they fail to update their activity time stamps. 
> ODB time stamps and data buffer time stamps appear to be handled in a similar manner.
> 
> Most checks are done like this:
> 
> now = ss_millitime();
> if (client->watchdog > 0      <----- check that the watchdog is enabled
>     && now > client->last_activity    <---- check for crazy time stamps from the future
>     && now - client->last_activity > client->watchdog_timeout)   <--- normal timeout
>         remove_client(client);
> 
> But in a few places, the extra checks are missing:
> 
> now = ss_millitime();
> if (now - client->last_activity > client->watchdog_timeout)
>         remove_client(client);
> 
> Is this an oversight from when additional checks were added?
> Should I make all checks read like the first one?
> 
> K.O.

This is on purpose. Inside cm_watchdog(), the system check for client->watchdog > 0. If the watchdog 
timeout is zero, the client is not removed. This feature is used if you debug a program. If you come to a 
breakpoint and sit there for a while, you might be declared dead and the application is removed from the 
ODB, meaning that you cannot continue debugging (on the next ODB access the application asserts). This 
can be avoided by setting the watchdog to zero, which is implemented in most applications by supplying 
"-d" on the command line. Now assume you debug a program, so you set the watchdog timeout to zero, but in 
the debugging session you decide to quit. Since the watchdog timeout is zero, you will never be removed 
from the ODB. Therefore, the code inside cm_cleanup() doe NOT check client->watchdog > 0. Therefore, a 
"cleanup" inside odbedit will even remove clients having the timeout set to zero. 

Now there might be more clever ways to accomplish that, but that's how it is implemented right now.

> When I run odbedit remotely (odbedit -h ladd09), the "cleanup" command unexpectedly produces the 
> output of the "sor" command (sure enough, there is a call to db_get_open_records() there), but when I run 
> it locally, I do not get this output (but db_get_open_records() is still called). Strange. K.O.

The db_get_open_records() call was by mistake there, I removed it. What remains is that the notification 
message if a client is removed from the ODB goes through the system messages. When running locally, odbedit 
echoes it's own messages, but when running remotely, this is not the case. So the messages can be seen by 
everybody else (plus it ends up in the message file), but not by the remote odbedit where the cleanup is 
started. The quick fix for that is to say "old" in odbedit which shows the last few lines of the message 
file, so one can see any successful cleanup. 

To fix this problem, do the following:

- Update to the current SVN revision 4368 of mhttpd.c
- Add following tag into your custom page:

  <input type=hidden name="redir" value="name">

  where "name" is the name of your custom page which follows the CS/ in the URL. Like 
if you have a custom page which you access through httpd://localhost/CS/junk then the 
tag would be 

  <input type=hidden name="redir" value="junk">

The "redir" parameter is now evaluated inside mhttpd and brings you back to the proper 
custom page. You can also define another custom page as the target, if that makes 
sense in your application.

Pierre: Would be nice to document this somewhere more officially.

Thanks Stefan. 
Your fix works nicely with the start/stop buttons not returning to the same or to a
different web page.

However, it does not seem to have fixed the problem with the Customscript button. It does
not seem to pick up the redirect, nor do the Pause/Resume buttons (which are programmed to
appear when the run starts).


> To fix this problem, do the following:
> 
> - Update to the current SVN revision 4368 of mhttpd.c
> - Add following tag into your custom page:
> 
>   <input type=hidden name="redir" value="name">
> 
>   where "name" is the name of your custom page which follows the CS/ in the URL. Like 
> if you have a custom page which you access through httpd://localhost/CS/junk then the 
> tag would be 
> 
>   <input type=hidden name="redir" value="junk">
> 
> The "redir" parameter is now evaluated inside mhttpd and brings you back to the proper 
> custom page. You can also define another custom page as the target, if that makes 
> sense in your application.
> 
> Pierre: Would be nice to document this somewhere more officially.

Not sure if this is a bug or a feature:

Writing a boolean label on an image seems to produce rather strange behaviour.

For example,
odb>ls /Equipment/gas/settings/my_bool -lt
Key name                        Type    #Val  Size  Last Opn Mode Value
---------------------------------------------------------------------------
my_bool                         BOOL    1     4     14m  0   RWD  y


odb>cd /custom/images/my_image.gif/labels
odb>ls
Src                             /Equipment/gas/settings/my_bool
Format                          val: %d (bool)
Font                            Medium
X                               10
Y                               10
Align                           0
FGColor                         FFFFFF
BGColor                         FF8800

Instead of the expected string "val: y (bool)", only the value of the key
appears, i.e. "y". 
The behaviour is the same whether I use %d, %u, %s, %c etc as the format character. 

> Not sure if this is a bug or a feature:
> 
> Writing a boolean label on an image seems to produce rather strange behaviour.
> 
> For example,
> odb>ls /Equipment/gas/settings/my_bool -lt
> Key name                        Type    #Val  Size  Last Opn Mode Value
> ---------------------------------------------------------------------------
> my_bool                         BOOL    1     4     14m  0   RWD  y
> 
> 
> odb>cd /custom/images/my_image.gif/labels
> odb>ls
> Src                             /Equipment/gas/settings/my_bool
> Format                          val: %d (bool)
> Font                            Medium
> X                               10
> Y                               10
> Align                           0
> FGColor                         FFFFFF
> BGColor                         FF8800
> 
> Instead of the expected string "val: y (bool)", only the value of the key
> appears, i.e. "y". 
> The behaviour is the same whether I use %d, %u, %s, %c etc as the format character. 

That has been fixed in rev. 4379

> Thanks Stefan. 
> Your fix works nicely with the start/stop buttons not returning to the same or to a
> different web page.
> 
> However, it does not seem to have fixed the problem with the Customscript button. It does
> not seem to pick up the redirect, nor do the Pause/Resume buttons (which are programmed to
> appear when the run starts).

That has been fixed in rev. 4377

I now have a better understanding of this: lazylogger uses ss_file_size() to find
out if a file exists or not. This function used to return 0 (probably) for
non-existant files (there was no check for error status from stat() system call,
so the return value for non-existant files was never well defined).

With ss_file_size() returning 0 for nonexistant files, 0-size files clearly cause
problems to lazylogger.

Now, since svn revision 4397, ss_file_size() returns -1 for non-existant files,
but lazylogger still needs to be tought about this.

The problem "lazylogger does not like 0-size files" remains for now.

K.O.


> With latest midas, I see this:
> 
> Thu Oct 14 19:31:17 2004 [Lazy_Tape] [lazylogger.c:1717:Lazy] lazy_file_exists
> file run17567.ybs doesn't exists
> Thu Oct 14 19:31:27 2004 [Lazy_Tape] [lazylogger.c:1717:Lazy] lazy_file_exists
> file run17567.ybs doesn't exists
> 
> The file run17567.ybs has size zero:
> 
> -rw-r--r--    1 twistonl users      950272 Oct 13 19:29
> /twist/data_onl/current/run17565.ybs
> -rw-r--r--    1 twistonl users      950272 Oct 13 19:45
> /twist/data_onl/current/run17566.ybs
> -rw-r--r--    1 twistonl users           0 Oct 13 20:00
> /twist/data_onl/current/run17567.ybs
> -rw-r--r--    1 twistonl users      983040 Oct 13 20:03
> /twist/data_onl/current/run17568.ybs
> -rw-r--r--    1 twistonl users      950272 Oct 13 20:26
> /twist/data_onl/current/run17569.ybs
> 
> I am not sure how to fix this lazylogger logic. Please help.
> 
> K.O.

Hi all,

I have just compiled midas 4401 using SuSE 11.0.
gcc is some odd SuSE version:
gcc version 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision 135036] (SUSE
Linux) 

Anyway, gcc stumbled over melog.c. I don't see the reason myself, but my
experience is that gcc is usually right when complaining about "array subscript
is above array bounds". So, just in case somebody knowlegeable wants to have a
look at this....

Cheers,

Randolf

The gcc output:

[...]
cc -g -O3 -Wall -Wuninitialized -Iinclude -Idrivers -I../mxml -Llinux/lib
-DINCLUDE_FTPLIB   -D_LARGEFILE64_SOURCE -DHAVE_MYSQL -I/usr/include/mysql
-DHAVE_ROOT -pthread -m64 -I/usr/local/root/root_v5.20.00/include/root
-DHAVE_ZLIB -DOS_LINUX -fPIC -Wno-unused-function -o linux/bin/melog
utils/melog.c linux/lib/libmidas.a -lutil -lpthread -lz
utils/melog.c: In function 'submit_elog':
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
utils/melog.c:224: warning: array subscript is above array bounds
cc -g -O3 -Wall -Wuninitialized -Iinclude -Idrivers -I../mxml -Llinux/lib
-DINCLUDE_FTPLIB   -D_LARGEFILE64_SOURCE -DHAVE_MYSQL -I/usr/include/mysql
-DHAVE_ROOT -pthread -m64 -I/usr/local/root/root_v5.20.00/include/root
-DHAVE_ZLIB -DOS_LINUX -fPIC -Wno-unused-function -o linux/bin/mlxspeaker
utils/mlxspeaker.c linux/lib/libmidas.a -lutil -lpthread -lz