Re: [Xymon-developer] Possible patch for 4.3.0

[Henrik S. <he...@hs...>]

Hi Cade,


Thanks for your patches, it's always nice to have someone
find and fix bugs :-)

I have a couple of questions about some of them, though.


> This is to up the ip string length to be able to include the port number:
> xxx.xxx.xxx.xxx:99999
> Some status messages were truncated without this.

> Index: include/libbbgen.h
> ===================================================================
> --- include/libbbgen.h  (revision 6373)
> +++ include/libbbgen.h  (working copy)
> @@ -27,7 +27,7 @@
>  #define STRBUF(buf) (buf->s)
>  #define STRBUFLEN(buf) (buf->used)
>  
> -#define IP_ADDR_STRLEN 16
> +#define IP_ADDR_STRLEN 22
>  
>  #include "version.h"
>  #include "config.h" 

This one surprises me, because the idea with this definition is that
variables defined with IP_ADDR_STRLEN can only contain an IP-address,
not a port number. So if something gets truncated because IP_ADDR_STRLEN
is too short, then it really is because we're doing something wrong
and not handling a portnumber correctly. And then this should be fixed
properly.

Can you give me an example of where you ran into this issue ? Was it
a configuration file that was mis-read, or something on a web page that
didn't display properly ?


> This again was so I could see the WHOLE sender rather than truncated.
>
> Index: hobbitd/hobbitd.c
> ===================================================================
> --- hobbitd/hobbitd.c   (revision 6373)

[series of sscanf("..%16s..") -> sscanf("..%s..") changes]

Yes, it relates to the first patch in your series. I don't want to
apply this - the %16s guards against the sscanf overflowing the
buffer that receives the data. So if anything we should change
it from %16s to %22s to match the IP_ADDR_STRLEN change.

But I cannot see why it should be necessary. What's the point in 
seeing the source portnumber of a received message ? It is assigned 
more or less at random by the server that sends us the message, it 
doesn't identify anything.



> This allows for a server to start with a number and not have it think it is
> a time.
> 
> EG: 28512dbs as a server name would make xymon treat it as 28512 days.
>
> Index: lib/timefunc.c
> ===================================================================
> --- lib/timefunc.c      (revision 6373)

Interesting. I have a slightly different fix for this, that I'll commit
shortly.



> This allows you to indent your bb-hosts file(s)
> 
> EG:
> 
>     page ndprodunixservers Linux Servers
>         title North Dakota Production Linux Servers
>             include bb-hosts.ndprodunixservers
> 
> Without this none of the indented lines were seen.

OK, this happens because the "include" is not recognized when indented.
I changed this a bit differently, and also fixed the "directory"
include which had the same problem.



> This keeps hobbitfect from crashing when it reads 8192 bytes
> 
> Index: hobbitd/hobbitfetch.c
> ===================================================================
> --- hobbitd/hobbitfetch.c       (revision 6373)
> +++ hobbitd/hobbitfetch.c       (working copy)
> @@ -346,7 +346,7 @@
>         char buf[8192];
>  
>         /* Read data from a peer connection (client or server) */
> -        n = read(conn->sockfd, buf, sizeof(buf));
> +    n = read(conn->sockfd, buf, sizeof(buf)-1);

Classic off-by-one bug. Thanks for spotting it.



> I don't remember why I did this but I did it for some reason.
> I think it was partially the server name starting with a number
> Index: hobbitd/bbcombotest.c
> ===================================================================
> --- hobbitd/bbcombotest.c       (revision 6373)
> +++ hobbitd/bbcombotest.c       (working copy)

Some of this doesn't look right to me.

The first part causes bbcombotest to query the hobbitd daemon for 
every value used in the combo-test expression - that causes a lot of 
load on the server for no good reason. The status is unlikely
to change during the time bbcombotest takes to evaluate the 
expression.

The second part changes the value of a yellow status when it goes
into an expression. With the current logic yellow is OK (like green),
your change makes yellow be not OK (like red). This is a change in
behaviour that must at least be documented, and it could break some
setups.

The third part looks like the "hostname-begins-with-number" change.
I have to do some more testing to make sure this is OK.

> @@ -295,13 +297,14 @@
>  
>         done = 0; inp=symbolicexpr; outp=expr; symp = NULL;
>         while (!done) {
> -               if (isalpha((int)*inp)) {
> +               //if (isalpha((int)*inp)) {
> +        	  if (isalpha((int)*inp) || isdigit((int) *inp) || *inp == '.') {
>                         if (!insymbol) { insymbol = 1; symp = symbol; }
>                         *symp = *inp; symp++;
>                 }
> -               else if (insymbol && (isdigit((int) *inp) || (*inp == '.'))) {
> -                       *symp = *inp; symp++;
> -               }
> +//             else if (insymbol && (isdigit((int) *inp) || (*inp == '.'))) {
> +//                     *symp = *inp; symp++;
> +//             }
>                 else if (insymbol && ((*inp == '\\') && (*(inp+1) > ' '))) {
>                         *symp = *(inp+1); symp++; inp++;
>                 }



Regards,
Henrik