Ticket #654 (closed defect: fixed)

Opened 3 months ago

Last modified 3 months ago

Updates to lib330

Reported by: baker Owned by: somebody
Priority: minor Milestone: All Platforms
Component: ALL modules Version: 7.10
Keywords: lib330 Cc:

Description

This is a very long post. Please bear with me. It is definitely easier to read on the Earthworm Trac web page, http://earthworm.isti.com/trac/earthworm/. Find the Ticket URL at the bottom of the email version and click that.

I have been methodically going through the myriad of warnings generated for lib330 in advance of the Earthworm 7.10 release. I have categorized the issues that I see below. For each category, I discuss my findings and the fix, when that is possible. Mostly, they are in the order I found them.

After applying the updates, lib330 compiles successfully for both 32-bit and 64-bit versions of Earthworm on a Mac, on Linux, and on Windows. All the systems are 64-bit.

It is important to note that lib330 is not IPv6 compatible. One if the big changes coming in the Earthworm 7.10 release is IPv6 support. As far as I can tell, that will not include lib330. Sites with Q330s will have to keep that in mind.

warning: implicit declaration of function

libnetserv.c:153:9: warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
        close (nsstr->npath) ;
        ^
libpoc.c:110:9: warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
        close (pocstr->cpath) ;
        ^
libsupport.c:555:7: warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
      close (desc) ;
      ^
libsupport.c:567:16: warning: implicit declaration of function 'lseek' is invalid in C99 [-Wimplicit-function-declaration]
      result = lseek(desc, long_offset, SEEK_SET) ;
               ^
libsupport.c:583:17: warning: implicit declaration of function 'read' is invalid in C99 [-Wimplicit-function-declaration]
      numread = read (desc, buf, size) ;
                ^
libsupport.c:595:18: warning: implicit declaration of function 'write' is invalid in C99 [-Wimplicit-function-declaration]
      numwrite = write(desc, buf, size) ;
                 ^
libdss.c:545:7: warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
      close (dssstr->q330->dsspath) ;
      ^
q330io.c:115:15: warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
              close (q330->cpath) ;
              ^
q330io.c:993:13: warning: implicit declaration of function 'read' is invalid in C99 [-Wimplicit-function-declaration]
  numread = read(q330->comid, addr(inbuf), IBSIZE) ;
            ^
q330io.c:1159:14: warning: implicit declaration of function 'write' is invalid in C99 [-Wimplicit-function-declaration]
  numwrite = write(q330->comid, buf, *cnt) ;
             ^

lib330 is very careful to include the header files containing the function prototypes it uses. These warnings occur because the header file containing the POSIX standard I/O calls, unistd.h, was inadvertently left out. The fix is to #include <unistd.h> and, in libsupport.c, #include <sys/types.h> on non-Windows (#ifndef X86_WIN32) platforms, and #include <io.h> on Windows.

Fixed in r7043 and r7423.

warning: returning 'const *' discards qualifiers

libclient.c:699:10: warning: returning 'const tmodules *' (aka 'tmodule const (*)[31]') from a function with result type 'pmodules' (aka 'tmodule (*)[31]') discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
  return addr(modules) ;
         ^~~~~~~~~~~~~

lib_get_modules() in libclient.c returns a pointer to the table of module names and version numbers contained in the library. The table was clearly intended to be read-only (const), yet lib_get_modules() discards the const attribute when it returns the address of the table to the caller.

The entries in the table of modules should definitely not be writeable. I examined the chain of typedef's used to define the table and that are used in lib_get_modules(). The typedef's are not used anywhere else.

lib330 is very careful to declare private (static) and public (extern) attributes. Only public objects and functions appear in the lib330 header files. Since there is an accessor function for the table, and the table is not declared extern in the libclient.h header file, the table should be declared static.

I fixed the typedef for the table to be read-only (const). This preserves the const attribute of the address returned by lib_get_modules(). I declared the table static to make it private.

Fixed in r7110 and r7413.

The only place lib_get_modules() is called is from lib330Interface_initialize() in src/data_sources/q3302ew/lib330interface.c. I changed the variable it uses to iterate through the table to be a pointer to a const module entry:

  const tmodule *module;

Fixed in r7414.

warning: using integer absolute value function 'abs' when argument is of floating point type

libslider.c:634:33: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
                            if (abs(t - q330->lasttime - 1.0) > 0.5)
                                ^
libslider.c:634:33: note: use function 'fabs' instead
                            if (abs(t - q330->lasttime - 1.0) > 0.5)
                                ^~~
                                fabs

This code truncates the argument to abs() towards zero to an integer, and then takes the absolute value. The result will always be an integer. Yet, the comparison is to a double. The code is supposed to detect a jump in the clock when it exceeds 0.5 seconds. It actually won't detect a jump smaller than 1.0 second.

Joe Steim concured: There appears to be one place where the use of abs() instead of the correct fabs() results in a change in operation, and where abs() is not correct. That is line libslider.c:634 that Larry flagged.

The fix is to use fabs(), as the compiler recommends.

Fixed in r7341.

libfilters.c:449:7: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
  if (abs(samp) > pavg->peak_abs)
      ^
libfilters.c:449:7: note: use function 'fabs' instead
  if (abs(samp) > pavg->peak_abs)
      ^~~
      fabs
libfilters.c:451:24: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
      pavg->peak_abs = abs(samp) ;
                       ^
libfilters.c:451:24: note: use function 'fabs' instead
      pavg->peak_abs = abs(samp) ;
                       ^~~
                       fabs

In this code, both samp and pavg->peak_abs are doubles. This code is a peak detector. It makes no sense to use doubles for the values and then throw away the fractional part.

Joe Steim concured: We agree with you on this one.

The fix is to use fabs(), as the compiler recommends.

Fixed in r7341.

warning: taking the absolute value of unsigned type 'uint16_t' (aka 'unsigned short') has no effect

q330io.c:1246:62: warning: taking the absolute value of unsigned type 'uint16_t' (aka 'unsigned short') has no effect [-Wabsolute-value]
        build_ip (q330, payload, srcaddr, destaddr, IPT_TCP, abs(datalength) + TCP_HDR_LTH) ;
                                                             ^

In this code, datalength is an unsigned variable; there is no need to take the absolute value. Six lines earlier, a call to the same function passes datalength, not abs(datalength).

Joe Steim concured: Feel free to make that change, and to delete the “abs()” in q330io.c, in the official release.

The fix is to pass datalength as-is to build_ip(), as is done in the same call six lines above this one.

Fixed in r7338.

warning: using floating point absolute value function 'fabs' when argument is of integer type

libsampcfg.c:226:42: warning: using floating point absolute value function 'fabs' when argument is of integer type [-Wabsolute-value]
        q->gap_secs = q->gap_threshold * fabs(q->rate) ;
                                         ^
libsampcfg.c:226:42: note: use function 'abs' instead
        q->gap_secs = q->gap_threshold * fabs(q->rate) ;
                                         ^~~~
                                         abs
libsampcfg.c:227:25: warning: using floating point absolute value function 'fabs' when argument is of integer type [-Wabsolute-value]
        q->gap_offset = fabs(q->rate) ; /* default is one data point per "rate" seconds */
                        ^
libsampcfg.c:227:25: note: use function 'abs' instead
        q->gap_offset = fabs(q->rate) ; /* default is one data point per "rate" seconds */
                        ^~~~
                        abs

In this code, q->rate is an integer, thus |q->rate| is an integer. Using fabs() instead of abs() gives the mistaken impression that q->rate is a double. abs(q->rate) will be converted to a double when it is multiplied by q->gap_threshold and when it is assigned to q->gap_offset. Until then, the arithmetic can stay in the integer domain and use abs().

N.b. There is actually no need for |rate|. In every case where |rate| is used, rate is already known to be negative (or, at least not positive, when the zero case is not handled). Thus, -rate is always safe to use and, to my eyes, more precisely conveys to the reader the encoding of true_rate = 1 / -rate iff rate < 0. Not to mention that a function call is more expensive than a negation, meaning smaller and faster code. In a real-time system, that is desirable.

Joe Steim remarked: Since you are making changes in the repo at this time, you’re welcome to make the change, but we don’t feel it’s necessary.

The fix is to use abs(), as the compiler recommends.

Fixed in r7339.

warning: incompatible pointer types

q330cvrt.c:604:13: warning: incompatible pointer types assigning to 'pbyte' (aka 'unsigned char *') from 'pbyte *' (aka 'unsigned char **'); dereference with * [-Wincompatible-pointer-types]
      pnext = p ;
            ^ ~
              *
q330cvrt.c:633:15: warning: incompatible pointer types assigning to 'pbyte *' (aka 'unsigned char **') from 'pbyte' (aka 'unsigned char *'); take the address with & [-Wincompatible-pointer-types]
            p = pnext ;
              ^ ~~~~~
                &

These errors occur in void loadssstat (pbyte *p, tstat_sersens *ssstat). p is a pointer to a pointer. However, pstart and pnext are simple pointers, pbyte pstart, pnext. pstart properly points to the start of the Serial sensor status block by dereferencing p first (pstart = *p). The dereferenced p is later updated to advance to the next block (*p = pstart). In contrast, pnext does not properly point to the start of each sensor sub-block in a similar way, as the code intends. As a result, pnext does not point to the start of the next sensor sub-block, and the second time through the loop over sensor sub-blocks will likely access garbage, if it does not segment fault with an illegal memory access. Most likely, when loadssstat() is called, only the first sensor sub-block information will be updated.

The fix is to dereference p when saving and restoring the value to and from pnext, as is done for pstart.

Fixed in r7379.

libcmds.c:1190:50: warning: incompatible pointer types passing 'pbyte' (aka 'unsigned char *') to parameter of type 'pbyte *' (aka 'unsigned char **'); take the address with & [-Wincompatible-pointer-types]
                                    loadfestats (p, addr(q330->share.stat_fes)) ;
                                                 ^
                                                 &

This error occurs in void lib_command_response (pq330 q330, pbyte pb). p is a pointer, pbyte p. void loadfestats (pbyte *p, tstat_fes *fes) in q330cvrt.c expects its p to be a pointer to a pointer, so it can update the dereferenced p to advance through the data. Nearby in lib_command_response() are calls to functions that perform similar functions, such as loadepstat (addr(p), addr(q330->share.stat_ep)). void loadepstat (pbyte *p, tstat_ep *epstat), also in q330cvrt.c, similarly expects its p to be a pointer to a pointer. The pointer p being passed to loadfestats() is indeed an incompatible pointer type, as the compiler says. Most likely, loadfestats() will load garbage into the Environmental Processor status, if it does not segment fault with an illegal memory access.

The fix is to pass addr(p) to loadfestats(), as is done for the call to loadepstat().

Fixed in r7415.

q330io.c:862:5: warning: incompatible pointer types assigning to 'pbyte *' (aka 'unsigned char **') from 'pbyte' (aka 'unsigned char *'); take the address with & [-Wincompatible-pointer-types]
  p = psave ;
    ^ ~~~~~
      &
q330io.c:863:15: warning: incompatible pointer types passing 'pbyte **' (aka 'unsigned char ***') to parameter of type 'pbyte *' (aka 'unsigned char **'); remove & [-Wincompatible-pointer-types]
  loadqdphdr (addr(p), addr(q330->recvhdr)) ;
              ^~~~~~~
q330io.c:874:31: warning: incompatible pointer types passing 'pbyte *' (aka 'unsigned char **') to parameter of type 'pbyte' (aka 'unsigned char *'); dereference with * [-Wincompatible-pointer-types]
  lib_command_response (q330, p) ;
                              ^
                              *

These errors occur in static void proc_udp (pq330 q330, pbyte *p, boolean foreign). p is a pointer to a pointer. Similar to the case above in the call to loadssstat(), psave is a simple pointer, pbyte psave. psave is used to save the start of the QDP header or foreign data by dereferencing p first (psave = *p). The code immediately before the first error, p = psave, looks like this:

  psave = *p ; /* start of QDP header or foreign data */
  if (foreign)
    <always returns>
  if (q330->recvudp.u_dst == q330->dataport)
    <always returns>
  if (q330->recvudp.u_dst != q330->ctrlport)
    <always returns>

The code block where the errors occur is next:

  p = psave ;
  loadqdphdr (addr(p), addr(q330->recvhdr)) ;
  if (q330->recvhdr.datalength > MAXDATA)
    <always returns>
  if (q330->recvhdr.crc != gcrccalc (addr(q330->crc_table), (pointer)((pntrint)psave + 4),
                       q330->recvhdr.datalength + QDP_HDR_LTH - 4))
    <always returns>
  lib_command_response (q330, p) ;

The non-error path calls loadqdphdr(), then lib_command_response(). void loadqdphdr (pbyte *p, tqdp *hdr) is in q330cvrt.c. The comment for loadqdphdr() is:

/* Note - this must be called before any other loadxxxx routines since you
  first need to know what the command received was, and the other routines
  expect a pointer past the header which will be the updated pointer after
  this routine is called. */

loadqdphdr() is supposed to be called with a pointer to a pointer p. It is being incorrectly called with another level of indirection because of the addr(p). void lib_command_response (pq330 q330, pbyte pb) is in libcmds.c. lib_command_response() is supposed to be called with a pointer p. It is being incorrectly called with another level of indirection because p is a pointer to a pointer.

Also, there seems to be no need for p = psave, even if it were written correctly. *p is already pointing to the start of the QDP header and it will be advanced by loadqdphdr() to the correct position for the call to lib_command_response(). psave also points to the start of the QDP header. That is needed for the CRC calculation in gcrccalc().

The proper code for this section is:

  loadqdphdr (p, addr(q330->recvhdr)) ;
  if (q330->recvhdr.datalength > MAXDATA)
    then
      return ;
  if (q330->recvhdr.crc != gcrccalc (addr(q330->crc_table), (pointer)((pntrint)psave + 4),
                       q330->recvhdr.datalength + QDP_HDR_LTH - 4))
    then
      begin
        add_status (q330, AC_CHECK, 1) ;
        return ;
      end
  lib_command_response (q330, *p) ;

I applied the fix. If anyone feels my solution is incorrect, please back out the fix.

Fixed in r7412.

warning: variable 'j' used in loop condition not modified in loop body

q330cvrt.c:980:20: warning: variable 'j' used in loop condition not modified in loop body [-Wfor-loop-analysis]
     for (j = 0 ; j <= 3 ; i++)
                  ^

This error will cause an infinite loop because of the typographical error incrementing i instead of the loop index variable, j.

The fix is to change i++ to j++.

Fixed in r7379.

warning: passing 'int32_t *' to parameter of type 'socklen_t *'

libnetserv.c:229:69: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  getsockopt (nsstr->npath, SOL_SOCKET, SO_LINGER, addr(lingeropt), addr(flag)) ;
                                                                    ^~~~~~~~~~
libnetserv.c:291:41: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  getsockname (nsstr->npath, addr(xyz), addr(lth)) ;
                                        ^~~~~~~~~
libnetserv.c:319:64: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  nsstr->sockpath = accept (nsstr->npath, addr(nsstr->client), addr(lth)) ;
                                                               ^~~~~~~~~
libnetserv.c:391:82: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
        err = getsockopt (nsstr->sockpath, SOL_SOCKET, SO_SNDBUF, addr(bufsize), addr(lth)) ;
                                                                                 ^~~~~~~~~
libpoc.c:163:106: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  err = recvfrom (pocstr->cpath, addr(pocstr->pkt.qdp), QDP_HDR_LTH + MAXDATA, 0, addr(pocstr->csockin), addr(lth)) ;
                                                                                                         ^~~~~~~~~
q330io.c:348:40: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  getsockname (q330->cpath, addr(xyz), addr(lth)) ;
                                       ^~~~~~~~~
q330io.c:384:78: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
        err = getsockopt (q330->dpath, SOL_SOCKET, SO_RCVBUF, addr(bufsize), addr(lth)) ;
                                                                             ^~~~~~~~~
q330io.c:437:46: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
        getsockname (q330->dpath, addr(xyz), addr(lth)) ;
                                             ^~~~~~~~~
q330io.c:574:105: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  err = recvfrom (q330->dpath, addr(q330->datain.qdp), QDP_HDR_LTH + MAXDATA96, 0, addr(q330->dsockin), addr(lth)) ;
                                                                                                        ^~~~~~~~~
q330io.c:712:120: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
        err = recvfrom (q330->cpath, addr(q330->commands.cmsgin.qdp), QDP_HDR_LTH + MAXDATA96, 0, addr(q330->csockin), addr(lth)) ;
                                                                                                                       ^~~~~~~~~
libdss.c:624:50: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  getsockname (dssstr->q330->dsspath, addr(xyz), addr(lth)) ;
                                                 ^~~~~~~~~
libdss.c:1630:104: warning: passing 'int32_t *' (aka 'int *') to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
  err = recvfrom (dssstr->q330->dsspath, addr(dssstr->msgin), sizeof(tdss_msg), 0, addr(dssstr->sock), addr(lth)) ;
                                                                                                       ^~~~~~~~~

These calls do not properly implement the Open Group sockets API spec in sys/socket.h. They pass an int32_t type where the API specifies a socklen_t type. socklen_t is an unsigned opaque integer type, at least 32 bits. The compiler is warning that the caller is not passing an unsigned type. Also, socklen_t is at least 32 bits in the spec, which means it could just as easily be 64 bits on a 64-bit machine. int32_t is guaranteed to be 32 bits. It is especially dangerous to refer to the wrong type when passing a pointer, since the compiler will not convert the dereferenced actual argument to the proper dereferenced dummy argument type. The API says the socklen_t *option_len argument is updated by these calls. If a socklen_t is actually 64 bits, memory adjacent to an int32_t option_len will be clobbered. These kinds of coding errors are extremely difficult to track down when they occur.

libnetserv.c is the only file that uses int32_t flag as both a flag argument and a length argument. All the other files use int21_t lth as a length argument only. Also, libnetserv.c and q330io.c use the flag argument both for WIN32 API calls and Unix fcntl() calls. The flag argument is not the same type in those two APIs.

The fix for libnetserv.c and q330io.c is to use the correct type for the flag arguments, as is done in libpoc.c. The fix for all these warnings is to use socklen_t lth for the length arguments.

I also found numerous duplicate lines of code like this:

#ifdef X86_WIN32
  lth = sizeof(BOOL) ;
  setsockopt (nsstr->npath, SOL_SOCKET, SO_REUSEADDR, addr(flag2), lth) ;
#else
  lth = sizeof(int) ;
  setsockopt (nsstr->npath, SOL_SOCKET, SO_REUSEADDR, addr(flag2), lth) ;
#endif
  lth = sizeof(struct linger) ;
#ifdef X86_WIN32
  getsockopt (nsstr->npath, SOL_SOCKET, SO_LINGER, addr(lingeropt), addr(lth)) ;
#else
  getsockopt (nsstr->npath, SOL_SOCKET, SO_LINGER, addr(lingeropt), addr(lth)) ;
#endif

This practice of duplicating code is inconsistent. In some files, the same code sequence is duplicated in one file, but not in the other file. I simplified the code to this:

#ifdef X86_WIN32
  lth = sizeof(BOOL) ;
#else
  lth = sizeof(int) ;
#endif
  setsockopt (nsstr->npath, SOL_SOCKET, SO_REUSEADDR, addr(flag2), lth) ;
  lth = sizeof(struct linger) ;
  getsockopt (nsstr->npath, SOL_SOCKET, SO_LINGER, addr(lingeropt), addr(lth)) ;

N.b. There is actual no reason for the #ifdef X86_WIN32 conditional code at all. The two places where the sizeof() operator is used could more safely be written as lth = sizeof(flag2) and lth = sizeof(lingeropt). For example, suppose flag2 is changed from a BOOL to some other scalar type in the WIN32 code. The author will have to realize that there are instances of sizeof(BOOL) referring to flag2 that will have to be located and also changed. That is a bug waiting to happen. sizeof(flag2) will be correct no matter what scalar type flag2 is declared as.

Fixed in r7418 and r7422.

warning: incompatible pointer types assigning to 'struct sockaddr_in *' from 'struct sockaddr *'

Not fixed

libdss.c:851:13: warning: incompatible pointer types assigning to 'struct sockaddr_in *' from 'struct sockaddr *' [-Wincompatible-pointer-types]
      psock = addr(dssstr->sock) ;
            ^ ~~~~~~~~~~~~~~~~~~
libdss.c:905:9: warning: incompatible pointer types assigning to 'struct sockaddr_in *' from 'struct sockaddr *' [-Wincompatible-pointer-types]
  psock = addr(dssstr->sock) ;
        ^ ~~~~~~~~~~~~~~~~~~
libdss.c:2020:9: warning: incompatible pointer types assigning to 'struct sockaddr_in *' from 'struct sockaddr *' [-Wincompatible-pointer-types]
  psock = addr(dssstr->sock) ;
        ^ ~~~~~~~~~~~~~~~~~~
libdss.c:2244:9: warning: incompatible pointer types assigning to 'struct sockaddr_in *' from 'struct sockaddr *' [-Wincompatible-pointer-types]
  psock = addr(dssstr->sock) ;
        ^ ~~~~~~~~~~~~~~~~~~

The code in libdss.c interchanges struct sockaddr_in's and struct sockaddr's. struct sockaddr is the generic name for the structure containing a socket address. struct sockaddr_in is the particular struct sockaddr for IPv4 socket addresses. I saw no sign of IPv6 support in lib330. While it seems to be common practice to rely on the size of a struct sockaddr_in to be the same size as a struct sockaddr, the Open Group sockets API spec in sys/socket.h contains no such assurance. The spec defines a struct sockaddr_storage, which is large enough to contain all the supported protocol-specific socket address structures. It defines rules for casts of the socket address structures only so far as to say the address family fields—sa_family_t sa_family in a sockaddr structure, sa_family_t ss_family in a sockaddr_storage structure—must be mapped.

These warnings do not appear in the other lib330 networking modules, such as libnetserv.c, because the assignments in those modules are cast as a generic pointer type (typedef void *pointer in libtypes.h), psock = (pointer) addr(nsstr->nsockin). This prevents the compiler from detecting any potential type incompatibility, since assignments of pointers to void are compatible with any pointer type variable.

The sockets code in libdss.c as well as the other modules will have to be carefully examined for potential improper aliasing of sockaddr structures, particularly when IPv6 is implemented. That is not a chore I am willing to take on.

I did not fix these warnings.

warning: enumeration values not handled in switch

Not fixed

libarchive.c:179:11: warning: enumeration values 'PKC_EVENT' and 'PKC_CALIBRATE' not handled in switch [-Wswitch]
  switch (q->pack_class) begin
          ^
libcmds.c:1538:21: warning: enumeration values 'LIBSTATE_TERM', 'LIBSTATE_DEALLOC', and 'LIBSTATE_DEREG' not handled in switch [-Wswitch]
            switch (q330->libstate) begin
                    ^
libcmds.c:1570:21: warning: 10 enumeration values not handled in switch: 'LIBSTATE_TERM', 'LIBSTATE_PING', 'LIBSTATE_CONN'... [-Wswitch]
            switch (q330->libstate) begin
                    ^
libcmds.c:1595:21: warning: 13 enumeration values not handled in switch: 'LIBSTATE_IDLE', 'LIBSTATE_TERM', 'LIBSTATE_PING'... [-Wswitch]
            switch (q330->libstate) begin
                    ^
libcmds.c:1610:21: warning: 13 enumeration values not handled in switch: 'LIBSTATE_TERM', 'LIBSTATE_PING', 'LIBSTATE_CONN'... [-Wswitch]
            switch (q330->libstate) begin
                    ^
libcmds.c:1535:17: warning: 8 enumeration values not handled in switch: 'LIBSTATE_CONN', 'LIBSTATE_ANNC', 'LIBSTATE_REG'... [-Wswitch]
        switch (q330->share.target_state) begin
                ^
libcmds.c:1636:11: warning: 11 enumeration values not handled in switch: 'LIBSTATE_PING', 'LIBSTATE_CONN', 'LIBSTATE_ANNC'... [-Wswitch]
  switch (q330->libstate) begin
          ^
libcmds.c:1858:17: warning: 9 enumeration values not handled in switch: 'LIBSTATE_IDLE', 'LIBSTATE_TERM', 'LIBSTATE_PING'... [-Wswitch]
        switch (q330->libstate) begin
                ^
libsample.c:937:15: warning: enumeration values 'PKC_EVENT', 'PKC_CALIBRATE', and 'PKC_MESSAGE' not handled in switch [-Wswitch]
      switch (q->pack_class) begin
              ^
libsample.c:967:15: warning: 4 enumeration values not handled in switch: 'PKC_EVENT', 'PKC_CALIBRATE', 'PKC_TIMING'... [-Wswitch]
      switch (q->pack_class) begin
              ^

In libarchive.c, archive_512_record() does not handle two defined tpacket_class's, PKC_EVENT and PKC_CALIBRATE. I don't know what a tpacket is or what the tpacket_class's are. Depending on what the lib330 API says, archive_512_record() may or may not be properly handling PKC_EVENT and PKC_CALIBRATE. If the ommision of the handlers for PKC_EVENT and PKC_CALIBRATE is deliberate, I recommend comments to that effect along with no-op handlers in the switch block.

In libcmds.c, all the missing enumeration values are in the lib_timer() function, which is periodically called to manage Q330 state transitions. Surely this code is critical to the proper function of Q330's and likely has no errors. A debug version of the code might consider adding default clauses with preprocessor conditionals to the switch blocks that save a stack traceback and clues for debugging.

in libsample.c, flush_lcqs() and flush_dplcqs() appear to selectively flush tpackets, which is why they only handle a few of the possible tpacket_class'es.

I leave these for someone else to decide whether these warnings need further examination.

warning: statement with no effect

libsupport.c: In function ‘lib_file_open’:
libsupport.c:538:7: warning: statement with no effect [-Wunused-value]
       flags or O_RDONLY ;
       ^

This error is in the section of code in lib_file_open() where the file open flags are being determined. When the file is being opened for read only, the O_RDONLY bit is supposed to be set in flags. In this line, the assignment of the result to the flags variable is missing.

The fix is to assign the result to the flags variable.

Fixed in r7421.

Anachronisms and code cleanup

warning: unused typedef

libsupport.c:316:19: warning: unused typedef 'plword' [-Wunused-local-typedef]
typedef longword *plword ;
                  ^

Nowhere in the lib330 library is plword used. This typedef can be safely removed.

Fixed in r7378.

warning: variable set but not used

Not fixed

libstats.c:155:40: warning: variable ‘duty’ set but not used [-Wunused-but-set-variable]
   longint total, sentdif, resdif, val, duty ;
                                        ^
libstrucs.c:497:16: warning: variable ‘err’ set but not used [-Wunused-but-set-variable]
   integer res, err ;
                ^

lib330 is very clean. These are the only two instances of unnecessary variables. They are assigned values but never used. I think they can be safely removed. But, they should be examined by someone to decide if these are really errors because the variables were assigned values because they were intended to be used.

I did not fix these warnings.

Declare private functions static

I mentioned above that lib330 is very careful to include the header files containing the function prototypes it uses. I noticed that baler_file() in libsupport.c does not appear in libsupport.h. baler_file() in libsupport.c should be declared static.

Fixed in r7412.

I also noticed comments in libctrldet.c that the author had wanted to use nested procedures, including these lines:

/* Again, due to lack of nesting support, provide forward declarations */
extern void factor (texpand *pexp, ret *fac) ;
extern void express (texpand *pexp, ret *expr) ;
extern void term (texpand *pexp, ret *ter) ;

It is obvious the author intended these three functions to be private (static), not public (extern). For confirmation, notice they do not appear in the libctrldet.h header file. They should all be declared static.

Fixed in r7419.

Typo in libcvrt.h

lib330 function prototypes in the header files include the variable names. This one in libcvrt.h is missing the s in string *s:

extern void loadstring (pbyte *p, integer fieldwidth, string *) ;

Fixed in r7417.

All compilers support const now

In libmsgs.h, the prototypes for three functions in libmsgs.c are declared with const input string arguments only if CONSTMSG is defined:

#ifdef CONSTMSG
extern void libmsgadd (pq330 q330, word msgcode, const string95 *msgsuf) ;
extern void libdatamsg (pq330 q330, word msgcode, const string95 *msgsuf) ;
extern void msgadd (pq330 q330, word msgcode, longword dt, const string95 *msgsuf, boolean client) ;
#else
extern void libmsgadd (pq330 q330, word msgcode, string95 *msgsuf) ;
extern void libdatamsg (pq330 q330, word msgcode, string95 *msgsuf) ;
extern void msgadd (pq330 q330, word msgcode, longword dt, string95 *msgsuf, boolean client) ;
#endif

const has been available in standard C for almost 30 years. It should be used wherever possible. There are numerous instances already in lib330 of const. This is the only code that conditionalizes its use.

I removed the non-const function prototypes and the CONSTMSG macro.

Fixed in r7111 and r7419.

Add comments for 64-bit patches

I was looking over the lib330 patches for 64-bit support. I am suspicious when I see 32-bit hexadecimal constants with the high-order bits set (0xFF...) that the intent of the code was to sign-extend a value. When such code is converted to 64 bits, the 32-bit hexadecimal constant may have to be changed to properly sign extend the value, depending on the size of the result.

I saw such code in libtokens.c that set the high-order 24 bits of a byte value. In 32 bits, this produces a negative result. In 64 bits, it does not. I found where the result was used and saw that the only purpose of setting the high-order bits was to force a zero byte value to be non-zero later. A value of zero is reserved to mean not valid. When the value is non-zero, the low-order byte is isolated and used as a match in a search.

I added comments about the reason the high-order bits were being set and that any bits will do.

Fixed in r7420.

Incorrect string argument length in function prototype

Not fixed

There are warnings like this for the string argument mismatch in calls to add_cfg():

libslider.c:1030:42: warning: incompatible pointer types passing 'char [3]' to parameter of type 'string7 *' (aka 'char (*)[8]') [-Wincompatible-pointer-types]
                          add_cfg (paqs, "GL", p2, RAW_GLOBAL_SIZE, 0, 0) ;
                                         ^~~~

This is because the string argument to add_cfg() in libopaque.c is a pointer to a string7:

extern void add_cfg (paqstruc paqs, string7 *name, pointer buf, integer size,
                    integer num, byte flags) ;

Other than the usual plethora of warnings from the compiler for string argument mismatches, there are no warnings from the compiler for add_cfg(). However, I think the string7 *name argument should be string2 *name. The code only accesses name[0] and name[1]. I see only two cases where add_cfg() is called: from proc_insequence() in libslider.c with 2-character literals, and from start_cfgblks() in libopaque.c itself. There, add_cfg() is called with either 2-character literals, or with string3 s (which should also be string2 s, since q330id_dataport is LP_TEL1=0 .. LP_TEL4=3) after it is written by a sprintf(), such as:

 sprintf(s, "L%d", q330->par_create.q330id_dataport + 1) ;

lib330 is coded in a pseudo-Pascal style. It uses specific length string arguments. I assume the lengths should match what the function uses. In this case, the function argument list seems to be in error. As is the string3 s variable.

These and all the other string argument mismatches can be fixed when someone converts the use of character arrays in lib330 to be standard C. That would be a blessing. It is not clear lib330 will be around much longer, though, if IPv6 support is not soon forthcoming.

Change History

comment:1 Changed 3 months ago by baker

  • Status changed from new to closed
  • Resolution set to fixed

I fixed what I could. Some issues remain.

Note: See TracTickets for help on using tickets.