You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Karl Fogel <kf...@galois.collab.net> on 2000/11/27 18:06:12 UTC
Re: cvs commit: apr/threadproc/win32 proc.c
I'm curious -- how does it cause warnings to have a const in the
prototype of a function you're calling with non-const data? It seems
odd to me that this would cause a problem... Perhaps there's something
else going on here?
I mean, we're writing the functions, and we know we're not going to
change that data, right?
(Or is the issue that when we pass some of the data through to
execve() or whatever, it's making less strict promises than our
wrapper does, so we'd have to, ick, cast?)
-K
rbb@covalent.net writes:
> -1. From the execve man page on Linux:
>
> int execve (const char *filename, char *const argv [],
> char *const envp[]);
>
> and Single UNIX:
>
>
> int execve(const char *path, char *const argv[], char *const envp[]);
>
>
> And we use execve. We shouldn't be holding our users up to a higher level
> on const-ification than POSIX calls for. Plus, this causes warnings that
> are almost impossible to get rid of in Apache.
>
> Ryan
>
> _______________________________________________________________________________
> Ryan Bloom rbb@apache.org
> 406 29th St.
> San Francisco, CA 94131
> -------------------------------------------------------------------------------
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 27, 2000 at 01:35:43PM -0800, rbb@covalent.net wrote:
>...
> > In any case, I'm working on the problem above. Thanks for pointing it out.
>
> As a warning. I looked at the problem earlier today. This has some
> far-reaching effects in Apache, which is why I didn't fix them. I didn't
> have the time today to really look at how much this would effect Apache in
> the long-term.
It could have some ripple effects if we altered ap_create_environment(), but
that return type looks fine (all the return data comes from the pool, so it
can legally be changed).
I'm just about done with the mod_cgi changes and am about to test it out.
Commit coming in about 15 minutes.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by rb...@covalent.net.
> > If you want to fix this problem, then I will grudginly give this change a
> > -0. I personally think we should match the POSIX standard however.
>
> Thank you. I really do think this is the proper semantic change for our
> APIs. If there was going to be an underlying problem in the POSIX calls,
> then I'd definitely agree with you and put them back (or possibly keep the
> const to help callers, but copy the data within the routine).
>
> In any case, I'm working on the problem above. Thanks for pointing it out.
As a warning. I looked at the problem earlier today. This has some
far-reaching effects in Apache, which is why I didn't fix them. I didn't
have the time today to really look at how much this would effect Apache in
the long-term.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 27, 2000 at 01:22:05PM -0800, rbb@covalent.net wrote:
>
> > > We are asking for data that is const char * const *, but passing in
> > > char * const * data. This causes incompatible pointer type warnings when
> > > compiling Apache.
> >
> > I tried to get all of those changed. Where did I miss?
>
> mod_cgi.c calls ap_create_priviledge_process, which was changed because of
> this change.
Ah, crap. I remember looking at that, but deferred. Forgot to come back to
it and get it fixed :-)
> If you want to fix this problem, then I will grudginly give this change a
> -0. I personally think we should match the POSIX standard however.
Thank you. I really do think this is the proper semantic change for our
APIs. If there was going to be an underlying problem in the POSIX calls,
then I'd definitely agree with you and put them back (or possibly keep the
const to help callers, but copy the data within the routine).
In any case, I'm working on the problem above. Thanks for pointing it out.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by rb...@covalent.net.
> > We are asking for data that is const char * const *, but passing in
> > char * const * data. This causes incompatible pointer type warnings when
> > compiling Apache.
>
> I tried to get all of those changed. Where did I miss?
mod_cgi.c calls ap_create_priviledge_process, which was changed because of
this change.
If you want to fix this problem, then I will grudginly give this change a
-0. I personally think we should match the POSIX standard however.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 27, 2000 at 11:28:52AM -0800, rbb@covalent.net wrote:
> On 27 Nov 2000, Karl Fogel wrote:
> > I'm curious -- how does it cause warnings to have a const in the
> > prototype of a function you're calling with non-const data? It seems
> > odd to me that this would cause a problem... Perhaps there's something
> > else going on here?
> >
> > I mean, we're writing the functions, and we know we're not going to
> > change that data, right?
>
> We are asking for data that is const char * const *, but passing in
> char * const * data. This causes incompatible pointer type warnings when
> compiling Apache.
I tried to get all of those changed. Where did I miss?
> > (Or is the issue that when we pass some of the data through to
> > execve() or whatever, it's making less strict promises than our
> > wrapper does, so we'd have to, ick, cast?)
>
> APR is actually not complaining, but it does seem a bit odd to me that we
> are asking for stricter access than the underlying functions we are
> using.
Those are using the "char * const *data" for historical purposes. Before
"const" came along, they were just "char **data". POSIX was able to change
that to "char * const *data" in a revision because that is "pointer
compatible" with historical usage. If they put the extra const in there,
then old programs would get warnings.
> One of the arguments made for this commit was that we may actually
> get const data, so we had better be sure that we treat it as const. I
> disagree. If we are given const data, then we have a problem, because
> POSIX doesn't state that the C Run-Time won't change the data underneath
> us.
Are you talking about the stuff getting changed when we call execve()? Or
do you mean argc/argv?
For the former: it better not change it. I've seen plenty of uses like this:
{
char *args[5];
args[0] = "diff";
args[1] = "-u";
args[2] = file1;
args[3] = file2;
args[4] = NULL;
execve(...);
}
If the CRT tried to change any of the strings, then you'd get a segfault
because those strings are in a readonly segment.
If you're referring to argc/argv, then "volatile" is the proper qualifier.
> We NEED to match POSIX in this, and require char *const * data.
No. We should have the semantics that work. And the lack of const fails for
the above example.
If we need to cast going into execve(), fine. That is under the covers of
our function's declaration. There isn't going to be a platform out there
that modifies what we pass in.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 27, 2000 at 11:17:01AM -0600, Karl Fogel wrote:
> rbb@covalent.net writes:
> > We are asking for data that is const char * const *, but passing in
> > char * const * data. This causes incompatible pointer type warnings when
> > compiling Apache.
>
> In that case, it seems appropriate to cast the data being passed in --
> adding more const is always okay.
Yup, totally okay. And in many cases, that data is *already* const; removing
the const to be compatible with "char * const *" is REALLY bad.
>...
> > APR is actually not complaining, but it does seem a bit odd to me that we
> > are asking for stricter access than the underlying functions we are
> > using. One of the arguments made for this commit was that we may actually
> > get const data, so we had better be sure that we treat it as const. I
> > disagree. If we are given const data, then we have a problem, because
> > POSIX doesn't state that the C Run-Time won't change the data underneath
> > us.
> >
> > We NEED to match POSIX in this, and require char *const * data.
>
> Ah, yes. If the underlying functions aren't promising that, then we
> can't either. Too bad.
It isn't "promising" it solely for historical reasons. It *does* have to
promise it for the reasons in my previous mail. You'd get segfaults if it
didn't treat the strings as const.
> (Somehow I suspect that they never do change the data, and that their
> prototypes are just because someone was being loose, but we can't know
> that with certainty, I suppose.)
Historical.
> Okay. I withdraw my (only implied) +1 on adding the consts. :-)
yes/no/yes/no. Fickle, fickle boy :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Karl Fogel <kf...@galois.collab.net>.
rbb@covalent.net writes:
> We are asking for data that is const char * const *, but passing in
> char * const * data. This causes incompatible pointer type warnings when
> compiling Apache.
In that case, it seems appropriate to cast the data being passed in --
adding more const is always okay.
(Yes, it's true, I have been totally, yea willingly, brainwashed by
Greg Stein. :-) )
> APR is actually not complaining, but it does seem a bit odd to me that we
> are asking for stricter access than the underlying functions we are
> using. One of the arguments made for this commit was that we may actually
> get const data, so we had better be sure that we treat it as const. I
> disagree. If we are given const data, then we have a problem, because
> POSIX doesn't state that the C Run-Time won't change the data underneath
> us.
>
> We NEED to match POSIX in this, and require char *const * data.
Ah, yes. If the underlying functions aren't promising that, then we
can't either. Too bad.
(Somehow I suspect that they never do change the data, and that their
prototypes are just because someone was being loose, but we can't know
that with certainty, I suppose.)
Okay. I withdraw my (only implied) +1 on adding the consts. :-)
-K
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by rb...@covalent.net.
On 27 Nov 2000, Karl Fogel wrote:
> I'm curious -- how does it cause warnings to have a const in the
> prototype of a function you're calling with non-const data? It seems
> odd to me that this would cause a problem... Perhaps there's something
> else going on here?
>
> I mean, we're writing the functions, and we know we're not going to
> change that data, right?
We are asking for data that is const char * const *, but passing in
char * const * data. This causes incompatible pointer type warnings when
compiling Apache.
> (Or is the issue that when we pass some of the data through to
> execve() or whatever, it's making less strict promises than our
> wrapper does, so we'd have to, ick, cast?)
APR is actually not complaining, but it does seem a bit odd to me that we
are asking for stricter access than the underlying functions we are
using. One of the arguments made for this commit was that we may actually
get const data, so we had better be sure that we treat it as const. I
disagree. If we are given const data, then we have a problem, because
POSIX doesn't state that the C Run-Time won't change the data underneath
us.
We NEED to match POSIX in this, and require char *const * data.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------