Re: [PATCH] [MINOR] Capture display more data from health checks

2009-10-09 Thread Krzysztof Oledzki 



On Mon, 5 Oct 2009, Krzysztof Oledzki wrote:




On Mon, 5 Oct 2009, Willy Tarreau wrote:


On Mon, Oct 05, 2009 at 12:51:07AM +0200, Krzysztof Oledzki wrote:

Non ascii or HTML control characters are masked.


I don't see the reason for masking HTML characters, since the output
is sent over syslog. I'm OK for masking non-ascii characters however.


Because the output is presented on html page - it is added to a popup
generated by td title. But you are right - we should also add it to
syslog.


I'd prefer that we correctly encode at the destination, which means
to do this in the html stats functions. If necessary I can write an
HTML encoder which would take two chunks for instance.




OK, how about this one? If this version is more or less acceptable I'll 
clean it and split into three patches:

 - adding cut_crlf,
 - adding chunk_htmlencode, chunk_asciiencode,
 - adding the main functionality.

Comments?

diff --git a/include/common/defaults.h b/include/common/defaults.h
index c09f5a4..b0aee86 100644
--- a/include/common/defaults.h
+++ b/include/common/defaults.h
@@ -174,4 +174,9 @@
 #define MAX_HOSTNAME_LEN   32
 #endif

+/* Maximum health check description length */
+#ifndef HCHK_DESC_LEN
+#define HCHK_DESC_LEN  128
+#endif
+
 #endif /* _COMMON_DEFAULTS_H */
diff --git a/include/common/standard.h b/include/common/standard.h
index 715ed24..8dd2731 100644
--- a/include/common/standard.h
+++ b/include/common/standard.h
@@ -282,6 +282,18 @@ extern int strl2ic(const char *s, int len);
 extern int strl2irc(const char *s, int len, int *ret);
 extern int strl2llrc(const char *s, int len, long long *ret);

+static inline char *cut_crlf(char *s) {
+   while (*s != '\r' || *s == '\n') {
+   char *p = s++;
+
+   if (!*p)
+   return p;
+   }
+
+   *s++ = 0;
+   return s;
+}
+
 /* This function converts the time_t value now into a broken out struct tm
  * which must be allocated by the caller. It is highly recommended to use this
  * function intead of localtime() because that one requires a time_t* which
diff --git a/include/types/server.h b/include/types/server.h
index dae1a71..9971da2 100644
--- a/include/types/server.h
+++ b/include/types/server.h
@@ -127,6 +127,7 @@ struct server {
struct timeval check_start; /* last health check start time 
*/
unsigned long check_duration;   /* time in ms took to finish 
last health check */
short check_status, check_code; /* check result, check code */
+   char check_desc[HCHK_DESC_LEN]; /* healt check descritpion */

struct freq_ctr sess_per_sec;   /* sessions per second on this 
server */
int puid;   /* proxy-unique server ID, used 
for SNMP */
diff --git a/src/buffers.c b/src/buffers.c
index ee00f1c..2da3b16 100644
--- a/src/buffers.c
+++ b/src/buffers.c
@@ -10,6 +10,7 @@
  *
  */

+#include ctype.h
 #include stdarg.h
 #include stdio.h
 #include string.h
@@ -325,6 +326,89 @@ int chunk_printf(struct chunk *chk, const char *fmt, ...)
 }

 /*
+ * Encode chunk src into chunk dst, respecting the limit of at most 
chk-size chars.
+ * If the chk-len is over, nothing is added. Returns the new chunk size.
+ */
+int chunk_htmlencode(struct chunk *dst, struct chunk *src) {
+
+   int i, l;
+   int olen, free;
+   char c, t[sizeof(#255;)];
+
+   olen = dst-len;
+
+   for (i = 0; i  src-len; i++) {
+   free = dst-size - dst-len;
+
+   if (!free) {
+   dst-len = olen;
+   return dst-len;
+   }
+
+   c = src-str[i];
+
+   if (!isascii(c) || !isprint(c) || c == '' || c == '' || c == '\'' 
|| c == '' || c == '') {
+   sprintf(t, #%d;, c);
+   l = strlen(t);
+
+   if (free  l) {
+   dst-len = olen;
+   return dst-len;
+   }
+
+   memcpy(dst-str + dst-len, t, l);
+   dst-len += l;
+   } else {
+   dst-str[dst-len] = c;
+   dst-len++;
+   }
+   }
+
+   return dst-len;
+}
+
+/*
+ * Encode chunk src into chunk dst, respecting the limit of at most 
chk-size chars.
+ * If the chk-len is over, nothing is added. Returns the new chunk size.
+ */
+int chunk_asciiencode(struct chunk *dst, struct chunk *src, char qc) {
+   int i, l;
+   int olen, free;
+   char c, t[sizeof(FF)];
+
+   olen = dst-len;
+
+   for (i = 0; i  src-len; i++) {
+   free = dst-size - dst-len;
+
+   if (!free) {
+   dst-len = olen;
+   return dst-len;
+   }
+
+   c = src-str[i];
+
+   if (!isascii(c) || !isprint(c) || c == '' || c == '' || c == 
qc) {
+

Re: [PATCH] [MINOR] Capture display more data from health checks

2009-10-05 Thread Krzysztof Oledzki 



On Mon, 5 Oct 2009, Willy Tarreau wrote:


On Mon, Oct 05, 2009 at 12:51:07AM +0200, Krzysztof Oledzki wrote:

Non ascii or HTML control characters are masked.


I don't see the reason for masking HTML characters, since the output
is sent over syslog. I'm OK for masking non-ascii characters however.


Because the output is presented on html page - it is added to a popup
generated by td title. But you are right - we should also add it to
syslog.


I'd prefer that we correctly encode at the destination, which means
to do this in the html stats functions. If necessary I can write an
HTML encoder which would take two chunks for instance.


Sure, it would be great.


Feature can be disabled by defining HCHK_DESC_LEN to 0.


What could be the reason for disabling this feature ? After
all, if people enable log-health-checks, it means they want
more verbose info.


To save HCHK_DESC_LEN==128 bytes for each server. If you found it
unnecessary I'm more than happy to remove this check.


I don't care much about a proxy or a server size. I don't like to
waste, but overall it does not change much the memory usage. Even
large configs with 1000 servers will still only be hit by 128 kB.
You can bet that those running 1000 servers are not counting their
RAM in kilobytes ;-)




What I care about however, is that most of the hot struct members
are grouped so that they remain hot in the CPU caches. The other
important thing is to avoid increasing the session size.


Yes, I noticed you moved the place where the counters structuers are 
located. Next time I'll try to play with pahole from DWARF2 to find a 
better location for new variables. ;)


Best regards,

Krzysztof Olędzki

Re: [PATCH] [MINOR] Capture display more data from health checks

2009-10-04 Thread Willy Tarreau 
On Sun, Oct 04, 2009 at 09:35:33PM +0200, Krzysztof Piotr Oledzki wrote:
 From 1299a2fe5768c502786ef28cd78dae83a31f0c83 Mon Sep 17 00:00:00 2001
 From: Krzysztof Piotr Oledzki o...@ans.pl
 Date: Sun, 4 Oct 2009 21:28:53 +0200
 Subject: [MINOR] Capture  display more data from health checks
 
 Capture  display more data from health checks, like
 strerror(errno) for L4 failed checks or a first line
 from a response for L7 successes/failed checks.

I have a few questions about this one.

 Non ascii or HTML control characters are masked.

I don't see the reason for masking HTML characters, since the output
is sent over syslog. I'm OK for masking non-ascii characters however.

 Feature can be disabled by defining HCHK_DESC_LEN to 0.

What could be the reason for disabling this feature ? After
all, if people enable log-health-checks, it means they want
more verbose info.

 @@ -656,50 +679,78 @@ static int event_srv_chk_r(int fd)
   (memcmp(trash, HTTP/1., 7) != 0 ||
   (trash[12] != ' '  trash[12] != '\r')) ||
   !isdigit(trash[9]) || !isdigit(trash[10]) || 
 !isdigit(trash[11])) {
 - set_server_check_status(s, HCHK_STATUS_L7RSP);
 +
 + desc = trash;
 + p = strchr(desc, '\r');
 + if (p)
 + *p = '\0';

Here we should also check for '\n' alone, let's add this function
to standard.h to find and replace CR/LFs with a zero. It's small
and efficient enough to be reused for generic uses :

static inline char *cut_crlf(char *s)
{
do {
if (*s == '\n' || *s == '\r') 
*s = 0;
} while (*s++);
return s;
}

Then :
desc = trash;
cut_crlf(desc);

 +
 + set_server_check_status(s, HCHK_STATUS_L7RSP, desc);
 +
   goto out_wakeup;
   }
  
   s-check_code = str2uic(trash[9]);
  
 + desc = trash[12];
 + p = strchr(desc, '\r');
 + if (p)
 + *p = '\0';

same above

 + while(*desc == ' ')
 + desc++;
 +
(...)
   else if (s-proxy-options  PR_O_SMTP_CHK) {
   /* Check if the server speaks SMTP */
   if ((len  strlen(000\r)) ||
   (trash[3] != ' '  trash[3] != '\r') ||
   !isdigit(trash[0]) || !isdigit(trash[1]) || 
 !isdigit(trash[2])) {
 - set_server_check_status(s, HCHK_STATUS_L7RSP);
 +
 + desc = trash;
 + p = strchr(desc, '\r');
 + if (p)
 + *p = '\0';

and here.

 +
 + set_server_check_status(s, HCHK_STATUS_L7RSP, desc);
 +
   goto out_wakeup;
   }
  
   s-check_code = str2uic(trash[0]);
  
 + desc = trash[3];
 + p = strchr(desc, '\r');
 + if (p)
 + *p = '\0';

and here.

I think that if the outputs are small enough, we'll be able to
make them appear as automatic popups in the HTML stats page
when the check column is highlighted.

Regards,
Willy

Re: [PATCH] [MINOR] Capture display more data from health checks

2009-10-04 Thread Krzysztof Oledzki 



On Mon, 5 Oct 2009, Willy Tarreau wrote:


On Sun, Oct 04, 2009 at 09:35:33PM +0200, Krzysztof Piotr Oledzki wrote:

From 1299a2fe5768c502786ef28cd78dae83a31f0c83 Mon Sep 17 00:00:00 2001

From: Krzysztof Piotr Oledzki o...@ans.pl
Date: Sun, 4 Oct 2009 21:28:53 +0200
Subject: [MINOR] Capture  display more data from health checks

Capture  display more data from health checks, like
strerror(errno) for L4 failed checks or a first line
from a response for L7 successes/failed checks.


I have a few questions about this one.


Sure :)


Non ascii or HTML control characters are masked.


I don't see the reason for masking HTML characters, since the output
is sent over syslog. I'm OK for masking non-ascii characters however.


Because the output is presented on html page - it is added to a popup 
generated by td title. But you are right - we should also add it to 
syslog.



Feature can be disabled by defining HCHK_DESC_LEN to 0.


What could be the reason for disabling this feature ? After
all, if people enable log-health-checks, it means they want
more verbose info.


To save HCHK_DESC_LEN==128 bytes for each server. If you found it 
unnecessary I'm more than happy to remove this check.



@@ -656,50 +679,78 @@ static int event_srv_chk_r(int fd)
(memcmp(trash, HTTP/1., 7) != 0 ||
(trash[12] != ' '  trash[12] != '\r')) ||
!isdigit(trash[9]) || !isdigit(trash[10]) || 
!isdigit(trash[11])) {
-   set_server_check_status(s, HCHK_STATUS_L7RSP);
+
+   desc = trash;
+   p = strchr(desc, '\r');
+   if (p)
+   *p = '\0';


Here we should also check for '\n' alone, let's add this function
to standard.h to find and replace CR/LFs with a zero. It's small
and efficient enough to be reused for generic uses :

static inline char *cut_crlf(char *s)
{
   do {
   if (*s == '\n' || *s == '\r')
   *s = 0;
   } while (*s++);
return s;
}

Then :
desc = trash;
cut_crlf(desc);


Sure. How about adding a break after a first match?


I think that if the outputs are small enough, we'll be able to
make them appear as automatic popups in the HTML stats page
when the check column is highlighted.


Yep, this is exactly the place where you can find them currently. ;)

Best regards,

Krzysztof Olędzki