You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ben Laurie <be...@algroup.co.uk> on 2000/06/11 22:56:57 UTC
[PATCH] Start of command handlers patch
Just to make sure everyone knows what I had in mind, here's the start of
the patch. BTW, if we're all agreed this is worth doing, I'd really like
to commit it part-done and share the load of converting every single
command handler!
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html
Coming to ApacheCon Europe 2000? http://apachecon.com/
Re: [PATCH] Start of command handlers patch
Posted by Ben Laurie <be...@algroup.co.uk>.
Greg Stein wrote:
>
> On Sun, Jun 11, 2000 at 09:56:57PM +0100, Ben Laurie wrote:
> > Just to make sure everyone knows what I had in mind, here's the start of
> > the patch. BTW, if we're all agreed this is worth doing, I'd really like
> > to commit it part-done and share the load of converting every single
> > command handler!
>
> I'm fine with a broken repository for a while :-)
>
> >...
> > --- include/http_config.h 2000/06/06 01:20:01 1.30
> > +++ include/http_config.h 2000/06/11 20:48:03
> > @@ -96,9 +96,11 @@
> > TAKE13 /* one or three arguments */
> > };
> >
> > +typedef struct cmd_parms_struct cmd_parms;
> > +
> > typedef struct command_struct {
> > const char *name; /* Name of this command */
> > - const char *(*func) (); /* Function invoked */
> > + const char *(*func_) (cmd_parms *); /* Function invoked */
>
> The trailing underscore is ugly ... what's it in there for?
A development aid - it'll be gone before I commit.
> >...
> > --- main/http_config.c 2000/06/06 01:20:02 1.60
> > +++ main/http_config.c 2000/06/11 20:48:09
> > @@ -645,22 +645,23 @@
> >
> > parms->info = cmd->cmd_data;
> > parms->cmd = cmd;
> > + parms->mconfig = mconfig;
> > + parms->args = NULL;
> >
>
> Why is parms->args set to NULL, but none of the others are? Can't we just
> assume that parms has been zeroed by default and skip this?
I forgot to do the others - I don't think we can assume that, no -
consider a TAKE12 used twice.
>
> >...
> > --- main/http_core.c 2000/06/06 21:45:11 1.70
> > +++ main/http_core.c 2000/06/11 20:48:10
> > @@ -1374,10 +1374,10 @@
> > "> directive missing closing '>'", NULL);
> > }
> >
> > -static const char *dirsection(cmd_parms *cmd, void *dummy, const char *arg)
> > +static const char *dirsection(cmd_parms *cmd)
> > {
> > const char *errmsg;
> > - char *endp = strrchr(arg, '>');
> > + char *endp = strrchr(cmd->args, '>');
> > int old_overrides = cmd->override;
> > char *old_path = cmd->path;
> > core_dir_config *conf;
> > @@ -1397,14 +1397,14 @@
> >
> > *endp = '\0';
> >
> > - cmd->path = ap_getword_conf(cmd->pool, &arg);
> > + cmd->path = ap_getword_conf(cmd->pool, &cmd->args);
>
> Hmm. Unsure on this one... I'm a bit more comfortable thinking that
> cmd->args should not have its value altered.
Well, strictly, perhaps, but it is _only_ used by the command handler.
It did give me pause, too.
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html
Coming to ApacheCon Europe 2000? http://apachecon.com/
Re: [PATCH] Start of command handlers patch
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Jun 11, 2000 at 09:56:57PM +0100, Ben Laurie wrote:
> Just to make sure everyone knows what I had in mind, here's the start of
> the patch. BTW, if we're all agreed this is worth doing, I'd really like
> to commit it part-done and share the load of converting every single
> command handler!
I'm fine with a broken repository for a while :-)
>...
> --- include/http_config.h 2000/06/06 01:20:01 1.30
> +++ include/http_config.h 2000/06/11 20:48:03
> @@ -96,9 +96,11 @@
> TAKE13 /* one or three arguments */
> };
>
> +typedef struct cmd_parms_struct cmd_parms;
> +
> typedef struct command_struct {
> const char *name; /* Name of this command */
> - const char *(*func) (); /* Function invoked */
> + const char *(*func_) (cmd_parms *); /* Function invoked */
The trailing underscore is ugly ... what's it in there for?
>...
> --- main/http_config.c 2000/06/06 01:20:02 1.60
> +++ main/http_config.c 2000/06/11 20:48:09
> @@ -645,22 +645,23 @@
>
> parms->info = cmd->cmd_data;
> parms->cmd = cmd;
> + parms->mconfig = mconfig;
> + parms->args = NULL;
>
Why is parms->args set to NULL, but none of the others are? Can't we just
assume that parms has been zeroed by default and skip this?
>...
> --- main/http_core.c 2000/06/06 21:45:11 1.70
> +++ main/http_core.c 2000/06/11 20:48:10
> @@ -1374,10 +1374,10 @@
> "> directive missing closing '>'", NULL);
> }
>
> -static const char *dirsection(cmd_parms *cmd, void *dummy, const char *arg)
> +static const char *dirsection(cmd_parms *cmd)
> {
> const char *errmsg;
> - char *endp = strrchr(arg, '>');
> + char *endp = strrchr(cmd->args, '>');
> int old_overrides = cmd->override;
> char *old_path = cmd->path;
> core_dir_config *conf;
> @@ -1397,14 +1397,14 @@
>
> *endp = '\0';
>
> - cmd->path = ap_getword_conf(cmd->pool, &arg);
> + cmd->path = ap_getword_conf(cmd->pool, &cmd->args);
Hmm. Unsure on this one... I'm a bit more comfortable thinking that
cmd->args should not have its value altered.
Otherwise... this looks good!
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Start of command handlers patch
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Ben Laurie wrote:
>
> Just to make sure everyone knows what I had in mind, here's the
> start of the patch. BTW, if we're all agreed this is worth doing,
> I'd really like to commit it part-done and share the load of
> converting every single command handler!
+1
--
#ken P-)}
Ken Coar <http://Golux.Com/coar/>
Apache Software Foundation <http://www.apache.org/>
"Apache Server for Dummies" <http://Apache-Server.Com/>
"Apache Server Unleashed" <http://ApacheUnleashed.Com/>
Re: [PATCH] Start of command handlers patch
Posted by Jeff Trawick <tr...@bellsouth.net>.
> Date: Mon, 12 Jun 2000 11:25:50 -0700
> From: Greg Stein <gs...@lyra.org>
>
> As I pointed out yesterday, though, that still doesn't mean the function
> will examine the right arguments in the cmd_parms :-)
One could always memset the arg fields to 0xFE before setting the ones
that are valid for the flavor of command handler and actually calling the
command handler... It won't help with misuse of the onoff flag but it
will certainly catch inappropriate access of the string pointers.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Start of command handlers patch
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jun 12, 2000 at 04:47:22AM -0700, dean gaudet wrote:
> On Sun, 11 Jun 2000, Ben Laurie wrote:
> > Just to make sure everyone knows what I had in mind, here's the start of
> > the patch. BTW, if we're all agreed this is worth doing, I'd really like
> > to commit it part-done and share the load of converting every single
> > command handler!
>
> ok... i kind of don't see the point -- 'cause it just makes the compile
> work without really providing type safety.
At the moment, a person can put any function into that field and it will
compile. With Ben's change, it will compile and run (no signature mismatch).
As I pointed out yesterday, though, that still doesn't mean the function
will examine the right arguments in the cmd_parms :-)
> we could do something we've done before -- set things up to give us
> warnings on the majority of platforms we develop on, but have no warnings
> on other platforms.
Even better... That would be coolness.
>...
> #define INIT_TAKE1(name, func, cmd_data, req_override, errmsg) \
> { name, { .take1 = func }, cmd_data, req_override, TAKE1, errmsg }
> #define INIT_TAKE2(name, func, cmd_data, req_override, errmsg) \
> { name, { .take2 = func }, cmd_data, req_override, TAKE2, errmsg }
> ...
>
> at least i think that's the right syntax.
>
> and if the compiler doesn't support that syntax we just cast in the
> INIT_xxx() macros.
+1 as long as the INIT_xxx() macros are prefixed with AP_. I'd also
recommend using the c9x format ([].take1 =...) that Dean posted in a
followup message, rather than the gcc-specific form (we can at least hope
for some compatibility from non-gcc compilers).
[ note some configure magic will be needed, too ]
A nice benefit from this approach is that the command functions don't have
to change at all. Just the initialization. A *much* lower impact change.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Start of command handlers patch
Posted by dean gaudet <dg...@arctic.org>.
On Mon, 12 Jun 2000, Ben Laurie wrote:
> dean gaudet wrote:
> > am i making you sick yet? :)
> Am _I_ making _you_ sick yet? :-)
nope :)
-dean
Re: [PATCH] Start of command handlers patch
Posted by Ben Laurie <be...@algroup.co.uk>.
dean gaudet wrote:
>
> On Mon, 12 Jun 2000, Ben Laurie wrote:
>
> > Cool - didn't know about this! Are you sure?
>
> yup, just did a little test:
>
> % cat t.c
> struct {
> int a;
> union {
> void *p;
> int b;
> } u;
> int c;
> } foo = {
> 1, { [].b = 2 }, 3
> };
> % cat t.s
> .file "t.c"
> .version "01.01"
> gcc2_compiled.:
> .globl foo
> .data
> .align 4
> .type foo,@object
> .size foo,12
> foo:
> .long 1
> .zero 4
> .long 3
> .ident "GCC: (GNU) egcs-2.91.66 19990314/Linux (egcs-1.1.2 release)"
>
>
> there's a gcc syntax for this and a c9x syntax, i'm pretty sure the above
> is the c9x syntax (istr digging out the standard to verify it before i
> started using it in my own code).
Cool.
> now only if c9x had overloaded function prototypes :)
+1!
> speaking of which -- if you wanted to do another gcc-ism you could try
> this:
>
> % cat t.c
> #include <string.h>
>
> #define ap_strchr(s, c) ((typeof(s))(strchr(s, c)))
>
> const char *a(const char *s, int c)
> {
> return ap_strchr(s, c);
> }
>
> char *b(char *s, int c)
> {
> return ap_strchr(s, c);
> }
>
> char *c(const char *s, int c)
> {
> return ap_strchr(s, c);
> }
> % gcc -Wall -c t.c
> t.c: In function `c':
> t.c:17: warning: return discards `const' from pointer target type
>
> am i making you sick yet? :)
Nah! This is cool - I get C++ features that Dean approves of! And
there's less code. And we get the native functions in all cases!
Am _I_ making _you_ sick yet? :-)
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html
Coming to ApacheCon Europe 2000? http://apachecon.com/
Re: [PATCH] Start of command handlers patch
Posted by dean gaudet <dg...@arctic.org>.
On Mon, 12 Jun 2000, Ben Laurie wrote:
> Cool - didn't know about this! Are you sure?
yup, just did a little test:
% cat t.c
struct {
int a;
union {
void *p;
int b;
} u;
int c;
} foo = {
1, { [].b = 2 }, 3
};
% cat t.s
.file "t.c"
.version "01.01"
gcc2_compiled.:
.globl foo
.data
.align 4
.type foo,@object
.size foo,12
foo:
.long 1
.zero 4
.long 3
.ident "GCC: (GNU) egcs-2.91.66 19990314/Linux (egcs-1.1.2 release)"
there's a gcc syntax for this and a c9x syntax, i'm pretty sure the above
is the c9x syntax (istr digging out the standard to verify it before i
started using it in my own code).
now only if c9x had overloaded function prototypes :)
speaking of which -- if you wanted to do another gcc-ism you could try
this:
% cat t.c
#include <string.h>
#define ap_strchr(s, c) ((typeof(s))(strchr(s, c)))
const char *a(const char *s, int c)
{
return ap_strchr(s, c);
}
char *b(char *s, int c)
{
return ap_strchr(s, c);
}
char *c(const char *s, int c)
{
return ap_strchr(s, c);
}
% gcc -Wall -c t.c
t.c: In function `c':
t.c:17: warning: return discards `const' from pointer target type
am i making you sick yet? :)
-dean
Re: [PATCH] Start of command handlers patch
Posted by Ben Laurie <be...@algroup.co.uk>.
dean gaudet wrote:
>
> On Sun, 11 Jun 2000, Ben Laurie wrote:
>
> > Just to make sure everyone knows what I had in mind, here's the start of
> > the patch. BTW, if we're all agreed this is worth doing, I'd really like
> > to commit it part-done and share the load of converting every single
> > command handler!
>
> ok... i kind of don't see the point -- 'cause it just makes the compile
> work without really providing type safety.
It provides more type safety than we have. And making the compile work
is worthwhile in itself.
> we could do something we've done before -- set things up to give us
> warnings on the majority of platforms we develop on, but have no warnings
> on other platforms.
>
> specifically we could use a c9x or gcc feature -- the ability to
> initialise a specific member of a union.
>
> change:
> const char *(*func) (); /* Function invoked */
>
> to:
>
> union {
> const char *(take1) (cmd_parms *cmd, void *mconfig, const char *arg1);
> const char *(take2) (cmd_parms *cmd, void *mconfig, const char *arg1, const char *arg2);
> ...
> } ufunc;
>
> and add:
>
> #define INIT_TAKE1(name, func, cmd_data, req_override, errmsg) \
> { name, { .take1 = func }, cmd_data, req_override, TAKE1, errmsg }
> #define INIT_TAKE2(name, func, cmd_data, req_override, errmsg) \
> { name, { .take2 = func }, cmd_data, req_override, TAKE2, errmsg }
> ...
>
> at least i think that's the right syntax.
Cool - didn't know about this! Are you sure?
> and if the compiler doesn't support that syntax we just cast in the
> INIT_xxx() macros.
And throw all the union stuff away where we call it... anyway, if this
works, I'll have a go at it...
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html
Coming to ApacheCon Europe 2000? http://apachecon.com/
Re: [PATCH] Start of command handlers patch
Posted by dean gaudet <dg...@arctic.org>.
On Sun, 11 Jun 2000, Ben Laurie wrote:
> Just to make sure everyone knows what I had in mind, here's the start of
> the patch. BTW, if we're all agreed this is worth doing, I'd really like
> to commit it part-done and share the load of converting every single
> command handler!
ok... i kind of don't see the point -- 'cause it just makes the compile
work without really providing type safety.
we could do something we've done before -- set things up to give us
warnings on the majority of platforms we develop on, but have no warnings
on other platforms.
specifically we could use a c9x or gcc feature -- the ability to
initialise a specific member of a union.
change:
const char *(*func) (); /* Function invoked */
to:
union {
const char *(take1) (cmd_parms *cmd, void *mconfig, const char *arg1);
const char *(take2) (cmd_parms *cmd, void *mconfig, const char *arg1, const char *arg2);
...
} ufunc;
and add:
#define INIT_TAKE1(name, func, cmd_data, req_override, errmsg) \
{ name, { .take1 = func }, cmd_data, req_override, TAKE1, errmsg }
#define INIT_TAKE2(name, func, cmd_data, req_override, errmsg) \
{ name, { .take2 = func }, cmd_data, req_override, TAKE2, errmsg }
...
at least i think that's the right syntax.
and if the compiler doesn't support that syntax we just cast in the
INIT_xxx() macros.
-dean