You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Dean Gaudet <dg...@arctic.org> on 1998/04/07 11:51:02 UTC
[PATCH] inaddr printf stuff
Sorry to detract from the holy naming wars. Here's a patch I'm wavering
on. This is the evolution of a patch I posted last october to give us a
method of easily printing struct inaddr and such. The intent was for this
to be used to improve the content of the error messages in apache 1.3
(which has less content than 1.2 -- 1.2 includes the client address in a
lot more errors than 1.3)
But I'm not sure any more that I want to go this way. The advantage to
this is that it's easy to print multiple inet addrs should you need to
(i.e. in the proxy). But the disadvantage is that it still requires a lot
of code to be fixed, and folks will still forget this stuff in their error
messages.
So in addition I'd like to propose:
API_EXPORT(void) aplog_rerror(const char *file, int line, int level,
const request_rec *s, const char *fmt, ...)
__attribute__((format(printf,5,6)));
Which takes a request_rec instead of a server_rec, and prints the relevant
client fields at the beginning of the message.
Why not add a flag to aplog_error()? Well because that would ditch type
checking... if we changed it to take a void * instead of a server_rec *
(so that we could pass a server_rec or a request_rec) then we'd lose type
checking. I'd rather not do that. I'd rather we did this, and have a
static routine in http_log.c which does both.
Anyhow, here's the patch for the printf extensions, search down to
include/ap.h for docs. Oh yeah there's a performance improvement in
there that I'll commit regardless.
Dean
Index: ap/ap_snprintf.c
===================================================================
RCS file: /export/home/cvs/apache-1.3/src/ap/ap_snprintf.c,v
retrieving revision 1.18
diff -u -r1.18 ap_snprintf.c
--- ap_snprintf.c 1998/03/31 12:52:13 1.18
+++ ap_snprintf.c 1998/04/07 09:31:03
@@ -384,6 +384,43 @@
+static char *conv_in_addr(struct in_addr *ia, char *buf_end, int *len)
+{
+ unsigned addr = ntohl(ia->s_addr);
+ char *p = buf_end;
+ bool_int is_negative;
+ int sub_len;
+
+ p = conv_10((addr & 0x000000FF) , TRUE, &is_negative, p, &sub_len);
+ *--p = '.';
+ p = conv_10((addr & 0x0000FF00) >> 8, TRUE, &is_negative, p, &sub_len);
+ *--p = '.';
+ p = conv_10((addr & 0x00FF0000) >> 16, TRUE, &is_negative, p, &sub_len);
+ *--p = '.';
+ p = conv_10((addr & 0xFF000000) >> 24, TRUE, &is_negative, p, &sub_len);
+
+ *len = buf_end - p;
+ return (p);
+}
+
+
+
+static char *conv_sockaddr_in(struct sockaddr_in *si, char *buf_end, int *len)
+{
+ char *p = buf_end;
+ bool_int is_negative;
+ int sub_len;
+
+ p = conv_10(ntohs(si->sin_port), TRUE, &is_negative, p, &sub_len);
+ *--p = ':';
+ p = conv_in_addr(&si->sin_addr, p, &sub_len);
+
+ *len = buf_end - p;
+ return (p);
+}
+
+
+
/*
* Convert a floating point number to a string formats 'f', 'e' or 'E'.
* The result is placed in buf, and len denotes the length of the string
@@ -660,33 +697,27 @@
i_num = va_arg(ap, u_wide_int);
else
i_num = (wide_int) va_arg(ap, unsigned int);
- /*
- * The rest also applies to other integer formats, so fall
- * into that case.
- */
+ s = conv_10(i_num, 1, &is_negative,
+ &num_buf[NUM_BUF_SIZE], &s_len);
+ FIX_PRECISION(adjust_precision, precision, s, s_len);
+ break;
+
case 'd':
case 'i':
- /*
- * Get the arg if we haven't already.
- */
- if ((*fmt) != 'u') {
- if (is_long)
- i_num = va_arg(ap, wide_int);
- else
- i_num = (wide_int) va_arg(ap, int);
- };
- s = conv_10(i_num, (*fmt) == 'u', &is_negative,
+ if (is_long)
+ i_num = va_arg(ap, wide_int);
+ else
+ i_num = (wide_int) va_arg(ap, int);
+ s = conv_10(i_num, 0, &is_negative,
&num_buf[NUM_BUF_SIZE], &s_len);
FIX_PRECISION(adjust_precision, precision, s, s_len);
- if (*fmt != 'u') {
- if (is_negative)
- prefix_char = '-';
- else if (print_sign)
- prefix_char = '+';
- else if (print_blank)
- prefix_char = ' ';
- }
+ if (is_negative)
+ prefix_char = '-';
+ else if (print_sign)
+ prefix_char = '+';
+ else if (print_blank)
+ prefix_char = ' ';
break;
@@ -801,26 +832,77 @@
break;
/*
- * Always extract the argument as a "char *" pointer. We
- * should be using "void *" but there are still machines
- * that don't understand it.
- * If the pointer size is equal to the size of an unsigned
- * integer we convert the pointer to a hex number, otherwise
- * we print "%p" to indicate that we don't handle "%p".
+ * This is where we extend the printf format, with a second
+ * type specifier
*/
case 'p':
- ui_num = (u_wide_int) va_arg(ap, char *);
+ switch(*++fmt) {
+ /*
+ * If the pointer size is equal to the size of an unsigned
+ * integer we convert the pointer to a hex number, otherwise
+ * we print "%p" to indicate that we don't handle "%p".
+ */
+ case 'p':
+ ui_num = (u_wide_int) va_arg(ap, void *);
+
+ if (sizeof(char *) <= sizeof(u_wide_int))
+ s = conv_p2(ui_num, 4, 'x',
+ &num_buf[NUM_BUF_SIZE], &s_len);
+ else {
+ s = "%p";
+ s_len = 2;
+ }
+ pad_char = ' ';
+ break;
- if (sizeof(char *) <= sizeof(u_wide_int))
- s = conv_p2(ui_num, 4, 'x',
- &num_buf[NUM_BUF_SIZE], &s_len);
- else {
- s = "%p";
- s_len = 2;
+ /* print a struct sockaddr_in as a.b.c.d:port */
+ case 'I':
+ {
+ struct sockaddr_in *si;
+
+ si = va_arg(ap, struct sockaddr_in *);
+ if (si != NULL) {
+ s = conv_sockaddr_in(si, &num_buf[NUM_BUF_SIZE], &s_len);
+ if (adjust_precision && precision < s_len)
+ s_len = precision;
+ }
+ else {
+ s = S_NULL;
+ s_len = S_NULL_LEN;
+ }
+ pad_char = ' ';
+ }
+ break;
+
+ /* print a struct in_addr as a.b.c.d */
+ case 'A':
+ {
+ struct in_addr *ia;
+
+ ia = va_arg(ap, struct in_addr *);
+ if (ia != NULL) {
+ s = conv_in_addr(ia, &num_buf[NUM_BUF_SIZE], &s_len);
+ if (adjust_precision && precision < s_len)
+ s_len = precision;
+ }
+ else {
+ s = S_NULL;
+ s_len = S_NULL_LEN;
+ }
+ pad_char = ' ';
+ }
+ break;
+
+ case NUL:
+ /* if %p ends the string, oh well ignore it */
+ continue;
+
+ default:
+ s = "bogus %p";
+ s_len = 8;
+ break;
}
- pad_char = ' ';
break;
-
case NUL:
/*
Index: include/ap.h
===================================================================
RCS file: /export/home/cvs/apache-1.3/src/include/ap.h,v
retrieving revision 1.11
diff -u -r1.11 ap.h
--- ap.h 1998/03/31 12:52:17 1.11
+++ ap.h 1998/04/07 09:31:05
@@ -81,8 +81,21 @@
#endif
#endif /* WIN32 */
-/* ap_vformatter() is a generic printf-style formatting routine
- * with some extensions.
+/* apapi_vformatter() is a generic printf-style formatting routine
+ * with some extensions. The extensions are:
+ *
+ * %pA takes a struct in_addr *, and prints it as a.b.c.d
+ * %pI takes a struct sockaddr_in * and prints it as a.b.c.d:port
+ * %pp takes a void * and outputs it in hex
+ *
+ * The %p hacks are to force gcc's printf warning code to skip
+ * over a pointer argument without complaining. This does
+ * mean that the ANSI-style %p (output a void * in hex format) won't
+ * work as expected at all, but that seems to be a fair trade-off
+ * for the increased robustness of having printf-warnings work.
+ *
+ * Additionally, apapi_vformatter allows for arbitrary output methods
+ * using the apapi_vformatter_buff and flush_func.
*
* The ap_vformatter_buff has two elements curpos and endpos.
* curpos is where ap_vformatter will write the next byte of output.
Index: modules/standard/mod_access.c
===================================================================
RCS file: /export/home/cvs/apache-1.3/src/modules/standard/mod_access.c,v
retrieving revision 1.33
diff -u -r1.33 mod_access.c
--- mod_access.c 1998/03/31 12:53:05 1.33
+++ mod_access.c 1998/04/07 09:31:06
@@ -377,7 +377,8 @@
if (ret == FORBIDDEN
&& (satisfies(r) != SATISFY_ANY || !some_auth_required(r))) {
aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, r->server,
- "Client denied by server configuration: %s", r->filename);
+ "client %pI denied by server configuration: %s",
+ &r->connection->remote_addr, r->filename);
}
return ret;
Re: [PATCH] inaddr printf stuff
Posted by Dean Gaudet <dg...@arctic.org>.
Right, both are fine... well, %pA otherwise we have to disable gcc's
printf warnings and I don't want to do that.
Dean
On Tue, 7 Apr 1998, Martin Kraemer wrote:
> On Tue, Apr 07, 1998 at 02:27:06PM -0700, Dean Gaudet wrote:
> > Take a peek at 1.2 code, look for all instances of log_reason()... every
> > one of those includes the client hostname. Those have all been converted
> > to aplog_error() in 1.3 and no longer include the client hostname. This
> > is a big problem in my opinion, the messages are mostly useless now.
>
> Don't you think that the existance of a %A format would have helped?
> Okay, I see, the request_rec is a prerequisite for the formatter to
> know about the address. But can't we have both: an aplog_rerror() taking
> the request_rec _and_ the %A format to go with it which prints the
> address at the place where it's useful, and not always at the beginning?
>
> Martin
> --
> | S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
> | ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
> | N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
> ~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request
>
Re: [PATCH] inaddr printf stuff
Posted by Martin Kraemer <Ma...@mch.sni.de>.
On Tue, Apr 07, 1998 at 02:27:06PM -0700, Dean Gaudet wrote:
> Take a peek at 1.2 code, look for all instances of log_reason()... every
> one of those includes the client hostname. Those have all been converted
> to aplog_error() in 1.3 and no longer include the client hostname. This
> is a big problem in my opinion, the messages are mostly useless now.
Don't you think that the existance of a %A format would have helped?
Okay, I see, the request_rec is a prerequisite for the formatter to
know about the address. But can't we have both: an aplog_rerror() taking
the request_rec _and_ the %A format to go with it which prints the
address at the place where it's useful, and not always at the beginning?
Martin
--
| S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
| ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
| N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request
Re: [PATCH] inaddr printf stuff
Posted by Dean Gaudet <dg...@arctic.org>.
Take a peek at 1.2 code, look for all instances of log_reason()... every
one of those includes the client hostname. Those have all been converted
to aplog_error() in 1.3 and no longer include the client hostname. This
is a big problem in my opinion, the messages are mostly useless now.
I'm proposing aplog_rerror() print the same stuff that log_reason() used
to print.
Dean
On Tue, 7 Apr 1998, Martin Kraemer wrote:
> On Tue, Apr 07, 1998 at 03:51:02AM -0700, Dean Gaudet wrote:
> > But the disadvantage is that it still requires a lot
> > of code to be fixed, and folks will still forget this stuff in their error
> > messages.
>
> If they forget to fix it, then aplog_rerror() won't help either.
>
> > So in addition I'd like to propose:
> >
> > API_EXPORT(void) aplog_rerror(const char *file, int line, int level,
> > const request_rec *s, const char *fmt, ...)
> > __attribute__((format(printf,5,6)));
> >
> > Which takes a request_rec instead of a server_rec, and prints the relevant
> > client fields at the beginning of the message.
>
> What do you mean by "the relevant client fields"? Does
> aplog_rerror() insert info from the request_req without asking? Or
> is it simply a wrapper to aplog_error(...., r->server,...)? In the
> first case, IMO the number of cases where exactly "the relevant
> client fields" should be printed are limited. I opt for the more
> flexible solution where everything you want to be printed must be
> specified in the format.
>
> Martin
> --
> | S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
> | ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
> | N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
> ~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request
>
Re: [PATCH] inaddr printf stuff
Posted by Marc Slemko <ma...@worldgate.com>.
On Tue, 7 Apr 1998, Martin Kraemer wrote:
> On Tue, Apr 07, 1998 at 03:51:02AM -0700, Dean Gaudet wrote:
> > But the disadvantage is that it still requires a lot
> > of code to be fixed, and folks will still forget this stuff in their error
> > messages.
>
> If they forget to fix it, then aplog_rerror() won't help either.
If I have to add a error message, and I have a choice between formatting
the whole message myself, keeping a consistent format, etc. and just
including the minimal information, I will include the minimal information.
If I have a choice between using a function that includes extra
information automatically and using one that doesn't, I will probably use
one that includes extra information.
Consistent log messages are good. Not having a zillion different formats
for the same thing is good. "template" functions for logging specific
types of messages are good, they just can't go overboard.
Re: [PATCH] inaddr printf stuff
Posted by Martin Kraemer <Ma...@mch.sni.de>.
On Tue, Apr 07, 1998 at 03:51:02AM -0700, Dean Gaudet wrote:
> But the disadvantage is that it still requires a lot
> of code to be fixed, and folks will still forget this stuff in their error
> messages.
If they forget to fix it, then aplog_rerror() won't help either.
> So in addition I'd like to propose:
>
> API_EXPORT(void) aplog_rerror(const char *file, int line, int level,
> const request_rec *s, const char *fmt, ...)
> __attribute__((format(printf,5,6)));
>
> Which takes a request_rec instead of a server_rec, and prints the relevant
> client fields at the beginning of the message.
What do you mean by "the relevant client fields"? Does
aplog_rerror() insert info from the request_req without asking? Or
is it simply a wrapper to aplog_error(...., r->server,...)? In the
first case, IMO the number of cases where exactly "the relevant
client fields" should be printed are limited. I opt for the more
flexible solution where everything you want to be printed must be
specified in the format.
Martin
--
| S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
| ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
| N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request
Re: [PATCH] inaddr printf stuff
Posted by Dean Gaudet <dg...@arctic.org>.
It's only for warnings, it's not strict type checking or anything, so
there's really nothing to work around in any compiler. It's just that gcc
is used by so many of us that it seems useful to cater to it -- especially
when it can find mistakes for us.
I'm not aware of similar warnings in other compilers. But if they do
exist, they're probably not configured to warn for these functions because
this is triggered by the __attribute__((printf,5,6)) stuff, which is a gcc
extension (and disabled for non-gcc). If they do warn, and they don't
treat the %p as an arbitrary pointer then they're broken, because %p is
ANSI for "expect a void *, print it in hex"... and the character after the
%p isn't part of the formatter normally so they shouldn't even consider
it.
Dean
On Tue, 7 Apr 1998, Martin Kraemer wrote:
> On Tue, Apr 07, 1998 at 03:51:02AM -0700, Dean Gaudet wrote:
> > Sorry to detract from the holy naming wars. Here's a patch I'm wavering
> > on. This is the evolution of a patch I posted last october to give us a
> > method of easily printing struct inaddr and such. The intent was for this
> > to be used to improve the content of the error messages in apache 1.3
> > (which has less content than 1.2 -- 1.2 includes the client address in a
> > lot more errors than 1.3)
>
> Looks good. But is there really no alternative to the %pI / %pA formats?
> There are compilers other than gcc out there.... Okay, we win gcc's
> half-baked type checking (which here is almost defeated because it
> accepts any pointer anyway). But such a format is not "clean" and
> consistent with the other format types (%.p or %+p or %#p? Nope, that's
> not clean either.... %A and %I will probably make gcc choke. Too bad.)
>
> Hmmm... I cabn't think of a better solution either.
>
> Martin
> --
> | S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
> | ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
> | N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
> ~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request
>
Re: [PATCH] inaddr printf stuff
Posted by Martin Kraemer <Ma...@mch.sni.de>.
On Tue, Apr 07, 1998 at 03:51:02AM -0700, Dean Gaudet wrote:
> Sorry to detract from the holy naming wars. Here's a patch I'm wavering
> on. This is the evolution of a patch I posted last october to give us a
> method of easily printing struct inaddr and such. The intent was for this
> to be used to improve the content of the error messages in apache 1.3
> (which has less content than 1.2 -- 1.2 includes the client address in a
> lot more errors than 1.3)
Looks good. But is there really no alternative to the %pI / %pA formats?
There are compilers other than gcc out there.... Okay, we win gcc's
half-baked type checking (which here is almost defeated because it
accepts any pointer anyway). But such a format is not "clean" and
consistent with the other format types (%.p or %+p or %#p? Nope, that's
not clean either.... %A and %I will probably make gcc choke. Too bad.)
Hmmm... I cabn't think of a better solution either.
Martin
--
| S I E M E N S | <Ma...@mch.sni.de> | Siemens Nixdorf
| ------------- | Voice: +49-89-636-46021 | Informationssysteme AG
| N I X D O R F | FAX: +49-89-636-44994 | 81730 Munich, Germany
~~~~~~~~~~~~~~~~My opinions only, of course; pgp key available on request