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...@newton.ch.collab.net> on 2002/07/02 21:48:25 UTC
proposal to add apr_check_dir_empty() to APR
Currently, apr_check_dir_empty() is living in the Subversion source
tree. The implementation looks portable, though; is there any reason
not to move this into APR?
apr_status_t
apr_check_dir_empty (const char *path,
apr_pool_t *pool)
{
apr_status_t apr_err, retval;
apr_dir_t *dir;
apr_finfo_t finfo;
apr_err = apr_dir_open (&dir, path, pool);
if (apr_err)
return apr_err;
/* All systems return "." and ".." as the first two files, so read
past them unconditionally. */
apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
if (apr_err) return apr_err;
apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
if (apr_err) return apr_err;
/* Now, there should be nothing left. If there is something left,
return EGENERAL. */
apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
if (APR_STATUS_IS_ENOENT (apr_err))
retval = APR_SUCCESS;
else if (! apr_err)
retval = APR_EGENERAL;
else
retval = apr_err;
apr_err = apr_dir_close (dir);
if (apr_err)
return apr_err;
return retval;
}
Thoughts?,
-Karl
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Glenn A. Thompson" <gt...@cdr.net> writes:
> Is that a tough process?
> 'cause you guys have some hash sorting stuff that should go over there
> also.
You're asking on the wrong list. :-)
(Or, at least, you want to cross post to dev@apr.apache.org.)
It's not that tough, just a matter of some APR developer finding time
(the set of APR developers intersects with the set of Subversion
developers, which is both an advantage and a disadvantage.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by "Glenn A. Thompson" <gt...@cdr.net>.
Hey:
Is that a tough process?
'cause you guys have some hash sorting stuff that should go over there
also.
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Aaron Bannert <aa...@clove.org>.
On Tue, Jul 02, 2002 at 02:31:05PM -0700, Justin Erenkrantz wrote:
> And, I think APR_EEXIST makes the most sense for
> apr_dir_check_empty().
+1
-aaron
Re: proposal to add apr_check_dir_empty() to APR
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, Jul 02, 2002 at 02:15:16PM -0700, Ryan Bloom wrote:
> which it isn't. 2) APR_EGENERAL doesn't really mean anything and
> should never have been added to the code. There is no such thing as a
APR_EGENERAL is probably used where an APR_FAILURE would work much
better. In fact, I really like that idea. In a lot of functions,
it is possible to either return an apr status code, success, or just
plain failure where it doesn't make sense to create yet another error
code for one function (esp. when that function isn't within APR).
IMHO, APR_TRUE and APR_FALSE are really bad ideas...
And, I think APR_EEXIST makes the most sense for
apr_dir_check_empty(). -- justin
Re: proposal to add apr_check_dir_empty() to APR
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, Jul 02, 2002 at 02:15:16PM -0700, Ryan Bloom wrote:
> which it isn't. 2) APR_EGENERAL doesn't really mean anything and
> should never have been added to the code. There is no such thing as a
APR_EGENERAL is probably used where an APR_FAILURE would work much
better. In fact, I really like that idea. In a lot of functions,
it is possible to either return an apr status code, success, or just
plain failure where it doesn't make sense to create yet another error
code for one function (esp. when that function isn't within APR).
IMHO, APR_TRUE and APR_FALSE are really bad ideas...
And, I think APR_EEXIST makes the most sense for
apr_dir_check_empty(). -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Aaron Bannert [mailto:aaron@clove.org]
>
> On Tue, Jul 02, 2002 at 04:53:28PM -0400, Cliff Woolley wrote:
> > My suggestion was going to be to just add a parameter that is an
int*
> and
> > return true/false that way. If false is returned, you can check the
> > apr_status_t to see what happened. Or maybe it should be the other
way
> > around, with int returned and an apr_status_t* in the parameter
list.
>
> I was considering that as well, and at this point I think that is a
> better general solution. Non-APR functions that want to use APR's
> apr_status_t codes don't have the luxury of being able to add another
> code, so given the performance tradeoff maybe this is the way to go...
No. Passing in an int * to get a Boolean back is completely insane. We
spent months coming up with an extensible error reporting framework that
could be extended by applications using APR. There are status ranges
that are usable by external apps.
This is really simple to solve, and it was how APR was designed. Return
a proper status code from the function. APR_EGENERAL has two problems
in this case. 1) The APR_E signifies that this is an error condition,
which it isn't. 2) APR_EGENERAL doesn't really mean anything and
should never have been added to the code. There is no such thing as a
general error, either there is a problem, and the user needs to know
what happened, or there isn't. A general error means that something bad
happened, but the user doesn't need to know what it was.
Ryan
Re: proposal to add apr_check_dir_empty() to APR
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Jul 2002, Aaron Bannert wrote:
> Non-APR functions that want to use APR's apr_status_t codes don't have
> the luxury of being able to add another code, so given the performance
> tradeoff maybe this is the way to go...
Well, they *can*, but APR doesn't know what to do with them. That's not a
problem in general -- apr-util does it for example. The part that strikes
me as silly is having this massive explosion of status codes when all
we're trying to return is "true" or "false". Why not just provide for
that in APR, leaving 3rd party status codes for things that REALLY need
them (actual unusual conditions, errors, statuses, whatever). Why make
people add their own "APR_FALSE"?
--Cliff
Re: proposal to add apr_check_dir_empty() to APR
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Jul 2002, Aaron Bannert wrote:
> Non-APR functions that want to use APR's apr_status_t codes don't have
> the luxury of being able to add another code, so given the performance
> tradeoff maybe this is the way to go...
Well, they *can*, but APR doesn't know what to do with them. That's not a
problem in general -- apr-util does it for example. The part that strikes
me as silly is having this massive explosion of status codes when all
we're trying to return is "true" or "false". Why not just provide for
that in APR, leaving 3rd party status codes for things that REALLY need
them (actual unusual conditions, errors, statuses, whatever). Why make
people add their own "APR_FALSE"?
--Cliff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Aaron Bannert [mailto:aaron@clove.org]
>
> On Tue, Jul 02, 2002 at 04:53:28PM -0400, Cliff Woolley wrote:
> > My suggestion was going to be to just add a parameter that is an
int*
> and
> > return true/false that way. If false is returned, you can check the
> > apr_status_t to see what happened. Or maybe it should be the other
way
> > around, with int returned and an apr_status_t* in the parameter
list.
>
> I was considering that as well, and at this point I think that is a
> better general solution. Non-APR functions that want to use APR's
> apr_status_t codes don't have the luxury of being able to add another
> code, so given the performance tradeoff maybe this is the way to go...
No. Passing in an int * to get a Boolean back is completely insane. We
spent months coming up with an extensible error reporting framework that
could be extended by applications using APR. There are status ranges
that are usable by external apps.
This is really simple to solve, and it was how APR was designed. Return
a proper status code from the function. APR_EGENERAL has two problems
in this case. 1) The APR_E signifies that this is an error condition,
which it isn't. 2) APR_EGENERAL doesn't really mean anything and
should never have been added to the code. There is no such thing as a
general error, either there is a problem, and the user needs to know
what happened, or there isn't. A general error means that something bad
happened, but the user doesn't need to know what it was.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Aaron Bannert <aa...@clove.org>.
On Tue, Jul 02, 2002 at 04:53:28PM -0400, Cliff Woolley wrote:
> My suggestion was going to be to just add a parameter that is an int* and
> return true/false that way. If false is returned, you can check the
> apr_status_t to see what happened. Or maybe it should be the other way
> around, with int returned and an apr_status_t* in the parameter list.
I was considering that as well, and at this point I think that is a
better general solution. Non-APR functions that want to use APR's
apr_status_t codes don't have the luxury of being able to add another
code, so given the performance tradeoff maybe this is the way to go...
-aaron
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Aaron Bannert [mailto:aaron@clove.org]
>
> On Tue, Jul 02, 2002 at 01:07:59PM -0700, Ryan Bloom wrote:
> > The return code should not be EGENERAL if the directory isn't empty.
> > Create a new status code if you have to, but a non-empty directory
is
> > not an error condition.
>
> I agree. EGENERAL is not the polar opposite of SUCCESS. APR isn't
> really friendly with simple binary functions that want to return
errors.
> It seems like you just want to return true or false in the normal
case,
> but also have the option to return an error if something blows up.
> Adding another apr_status_t code would be ok. Maybe APR_FAILURE to
> complement APR_SUCCESS, or we could go all out and add APR_TRUE
> and APR_FALSE...
No. APR is incredibly friendly with simple binary functions that want
to return errors. Take a look at apr_errno.h, we have a whole section
of return codes that do not imply success or failure, they are STATUS
codes. In this case, add a new STATUS code for non-empty directory, and
then you can return APR_SUCCESS for empty, the new code for non-empty,
and an error value for an error.
Ryan
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Aaron Bannert [mailto:aaron@clove.org]
>
> On Tue, Jul 02, 2002 at 01:07:59PM -0700, Ryan Bloom wrote:
> > The return code should not be EGENERAL if the directory isn't empty.
> > Create a new status code if you have to, but a non-empty directory
is
> > not an error condition.
>
> I agree. EGENERAL is not the polar opposite of SUCCESS. APR isn't
> really friendly with simple binary functions that want to return
errors.
> It seems like you just want to return true or false in the normal
case,
> but also have the option to return an error if something blows up.
> Adding another apr_status_t code would be ok. Maybe APR_FAILURE to
> complement APR_SUCCESS, or we could go all out and add APR_TRUE
> and APR_FALSE...
No. APR is incredibly friendly with simple binary functions that want
to return errors. Take a look at apr_errno.h, we have a whole section
of return codes that do not imply success or failure, they are STATUS
codes. In this case, add a new STATUS code for non-empty directory, and
then you can return APR_SUCCESS for empty, the new code for non-empty,
and an error value for an error.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Jul 2002, Aaron Bannert wrote:
> Adding another apr_status_t code would be ok. Maybe APR_FAILURE to
> complement APR_SUCCESS, or we could go all out and add APR_TRUE
> and APR_FALSE...
I was thinking about that. But then APR_FALSE == APR_SUCCESS for the
normal semantics to work out. Does APR_TRUE != APR_SUCCESS make sense?
I'm not sure.
With APR_FAILURE, it would seem to me as easy target for abuse in the same
way that APR_EGENERAL has been since it was created. Originally it was
very tightly scoped in meaning, but since then it's been used for all
kinds of things other than what was originally intended.
My suggestion was going to be to just add a parameter that is an int* and
return true/false that way. If false is returned, you can check the
apr_status_t to see what happened. Or maybe it should be the other way
around, with int returned and an apr_status_t* in the parameter list.
Whatever.
--Cliff
Re: proposal to add apr_check_dir_empty() to APR
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Jul 2002, Aaron Bannert wrote:
> Adding another apr_status_t code would be ok. Maybe APR_FAILURE to
> complement APR_SUCCESS, or we could go all out and add APR_TRUE
> and APR_FALSE...
I was thinking about that. But then APR_FALSE == APR_SUCCESS for the
normal semantics to work out. Does APR_TRUE != APR_SUCCESS make sense?
I'm not sure.
With APR_FAILURE, it would seem to me as easy target for abuse in the same
way that APR_EGENERAL has been since it was created. Originally it was
very tightly scoped in meaning, but since then it's been used for all
kinds of things other than what was originally intended.
My suggestion was going to be to just add a parameter that is an int* and
return true/false that way. If false is returned, you can check the
apr_status_t to see what happened. Or maybe it should be the other way
around, with int returned and an apr_status_t* in the parameter list.
Whatever.
--Cliff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Aaron Bannert <aa...@clove.org>.
I agree with Ryan on all points, and I have one additional comment:
On Tue, Jul 02, 2002 at 01:07:59PM -0700, Ryan Bloom wrote:
> The return code should not be EGENERAL if the directory isn't empty.
> Create a new status code if you have to, but a non-empty directory is
> not an error condition.
I agree. EGENERAL is not the polar opposite of SUCCESS. APR isn't
really friendly with simple binary functions that want to return errors.
It seems like you just want to return true or false in the normal case,
but also have the option to return an error if something blows up.
Adding another apr_status_t code would be ok. Maybe APR_FAILURE to
complement APR_SUCCESS, or we could go all out and add APR_TRUE
and APR_FALSE...
just my 2c,
-aaron
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > Just use
> >
> > APR_DIR_EMPTY
>
> Btw, we needed the reverse sense. That is, if APR_EEXIST would have
> indicated that the dir is not empty, then if we're not going to use
> it, we should use APR_DIR_NOT_EMPTY instead.
>
> The question is, should APR_SUCCESS indicate that the dir is empty, or
> do we need both
>
> APR_DIR_EMPTY
> APR_DIR_NOT_EMPTY
Sorry, use APR_SUCCESS for success and APR_DIR_NOT_EMPTY for failure.
Ryan
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > Just use
> >
> > APR_DIR_EMPTY
>
> Btw, we needed the reverse sense. That is, if APR_EEXIST would have
> indicated that the dir is not empty, then if we're not going to use
> it, we should use APR_DIR_NOT_EMPTY instead.
>
> The question is, should APR_SUCCESS indicate that the dir is empty, or
> do we need both
>
> APR_DIR_EMPTY
> APR_DIR_NOT_EMPTY
Sorry, use APR_SUCCESS for success and APR_DIR_NOT_EMPTY for failure.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> Just use
>
> APR_DIR_EMPTY
Btw, we needed the reverse sense. That is, if APR_EEXIST would have
indicated that the dir is not empty, then if we're not going to use
it, we should use APR_DIR_NOT_EMPTY instead.
The question is, should APR_SUCCESS indicate that the dir is empty, or
do we need both
APR_DIR_EMPTY
APR_DIR_NOT_EMPTY
?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> Just use
>
> APR_DIR_EMPTY
Btw, we needed the reverse sense. That is, if APR_EEXIST would have
indicated that the dir is not empty, then if we're not going to use
it, we should use APR_DIR_NOT_EMPTY instead.
The question is, should APR_SUCCESS indicate that the dir is empty, or
do we need both
APR_DIR_EMPTY
APR_DIR_NOT_EMPTY
?
-K
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> APR_DIR_EMPTY
>
> Ryan
>
> P.S. I am getting a say in the child's name, just not as big a say as
> Kelly. :-)
You're naming your kid "APR_DIR_EMPTY" ??? You're crazy, man...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> APR_DIR_EMPTY
>
> Ryan
>
> P.S. I am getting a say in the child's name, just not as big a say as
> Kelly. :-)
You're naming your kid "APR_DIR_EMPTY" ??? You're crazy, man...
Re: proposal to add apr_check_dir_empty() to APR
Posted by Ben Laurie <be...@algroup.co.uk>.
Cliff Woolley wrote:
> On Tue, 2 Jul 2002, Ryan Bloom wrote:
>
>
>>P.S. I am getting a say in the child's name, just not as big a say as
>>Kelly. :-)
>
>
> As long as you don't name him/her "void" or "apr_bloom_child" or
> something, you'll be fine. ;-)
That would never happen, coz it'd be obvious what the child was. He'd
name it something like "apr_prospective_bloom_parent". ;-)
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff
RE: proposal to add apr_check_dir_empty() to APR
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 2 Jul 2002, Ryan Bloom wrote:
> P.S. I am getting a say in the child's name, just not as big a say as
> Kelly. :-)
As long as you don't name him/her "void" or "apr_bloom_child" or
something, you'll be fine. ;-)
--Cliff
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > I am strongly against using APR_EEXISTS for this. APR_E* error
codes
> > imply that something went wrong. In this case, it is a status code,
not
> > an error code, and the APR convention is that ERROR codes start with
> > APR_E and STATUS codes start with APR_
>
> Ryan,
>
> You name the new status code, and I'll add it right now and change the
> code to use it.
Great, ask the guy who can't name things to save his life. My wife
won't even let me have a say in what we name our child, and you're
letting me name a macro. :-)
Just use
APR_DIR_EMPTY
Ryan
P.S. I am getting a say in the child's name, just not as big a say as
Kelly. :-)
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > I am strongly against using APR_EEXISTS for this. APR_E* error
codes
> > imply that something went wrong. In this case, it is a status code,
not
> > an error code, and the APR convention is that ERROR codes start with
> > APR_E and STATUS codes start with APR_
>
> Ryan,
>
> You name the new status code, and I'll add it right now and change the
> code to use it.
Great, ask the guy who can't name things to save his life. My wife
won't even let me have a say in what we name our child, and you're
letting me name a macro. :-)
Just use
APR_DIR_EMPTY
Ryan
P.S. I am getting a say in the child's name, just not as big a say as
Kelly. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> I am strongly against using APR_EEXISTS for this. APR_E* error codes
> imply that something went wrong. In this case, it is a status code, not
> an error code, and the APR convention is that ERROR codes start with
> APR_E and STATUS codes start with APR_
Ryan,
You name the new status code, and I'll add it right now and change the
code to use it.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> I am strongly against using APR_EEXISTS for this. APR_E* error codes
> imply that something went wrong. In this case, it is a status code, not
> an error code, and the APR convention is that ERROR codes start with
> APR_E and STATUS codes start with APR_
Ryan,
You name the new status code, and I'll add it right now and change the
code to use it.
-K
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Tue, Jul 02, 2002 at 04:00:01PM -0500, Karl Fogel wrote:
> > We should either make a new status code explicitly for this, or
return
> > via an int* parameter. Personally I think the int* solution is
> > cleaner: the function should return an error status iff something
went
> > wrong, and use an entirely separate channel for returning the
answer.
> >
> > Would anyone object to this solution?
>
> Count me with Ryan - int* is a really bad idea. Either adding a new
> status code or just using EEXISTS works for me. -- Justin
I am strongly against using APR_EEXISTS for this. APR_E* error codes
imply that something went wrong. In this case, it is a status code, not
an error code, and the APR convention is that ERROR codes start with
APR_E and STATUS codes start with APR_
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Tue, Jul 02, 2002 at 04:00:01PM -0500, Karl Fogel wrote:
> > We should either make a new status code explicitly for this, or
return
> > via an int* parameter. Personally I think the int* solution is
> > cleaner: the function should return an error status iff something
went
> > wrong, and use an entirely separate channel for returning the
answer.
> >
> > Would anyone object to this solution?
>
> Count me with Ryan - int* is a really bad idea. Either adding a new
> status code or just using EEXISTS works for me. -- Justin
I am strongly against using APR_EEXISTS for this. APR_E* error codes
imply that something went wrong. In this case, it is a status code, not
an error code, and the APR convention is that ERROR codes start with
APR_E and STATUS codes start with APR_
Ryan
Re: proposal to add apr_check_dir_empty() to APR
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, Jul 02, 2002 at 04:00:01PM -0500, Karl Fogel wrote:
> We should either make a new status code explicitly for this, or return
> via an int* parameter. Personally I think the int* solution is
> cleaner: the function should return an error status iff something went
> wrong, and use an entirely separate channel for returning the answer.
>
> Would anyone object to this solution?
Count me with Ryan - int* is a really bad idea. Either adding a new
status code or just using EEXISTS works for me. -- justin
Re: proposal to add apr_check_dir_empty() to APR
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, Jul 02, 2002 at 04:00:01PM -0500, Karl Fogel wrote:
> We should either make a new status code explicitly for this, or return
> via an int* parameter. Personally I think the int* solution is
> cleaner: the function should return an error status iff something went
> wrong, and use an entirely separate channel for returning the answer.
>
> Would anyone object to this solution?
Count me with Ryan - int* is a really bad idea. Either adding a new
status code or just using EEXISTS works for me. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> > Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
>
> We should either make a new status code explicitly for this, or return
> via an int* parameter. Personally I think the int* solution is
> cleaner: the function should return an error status iff something went
> wrong, and use an entirely separate channel for returning the answer.
>
> Would anyone object to this solution?
Yes, I do. The whole point of the apr_status_t is to allow for both
error codes and status codes. Use them. We use status codes throughout
APR to signify that there is important information that is neither
success nor failure. Take a look at apr_proc_wait for just one example.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> > Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
>
> We should either make a new status code explicitly for this, or return
> via an int* parameter. Personally I think the int* solution is
> cleaner: the function should return an error status iff something went
> wrong, and use an entirely separate channel for returning the answer.
>
> Would anyone object to this solution?
Yes, I do. The whole point of the apr_status_t is to allow for both
error codes and status codes. Use them. We use status codes throughout
APR to signify that there is important information that is neither
success nor failure. Take a look at apr_proc_wait for just one example.
Ryan
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> writes:
> One, the name sucks... perhaps apr_dir_is_empty()? Keep it named with the
> other apr_dir_ family members and our general schema. We have to work hard
> on consistency so we avoid symbol renames of newly created functions.
I've changed it to apr_dir_is_empty() in the Subversion sources, so it
will be that name when we port it over to APR.
> Other than that, I would _NOT_ assume all first two entries are "."/"..", but
> if you make that an explicit string test, I'd be +1.
Fixed now.
> Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
We should either make a new status code explicitly for this, or return
via an int* parameter. Personally I think the int* solution is
cleaner: the function should return an error status iff something went
wrong, and use an entirely separate channel for returning the answer.
Would anyone object to this solution?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> writes:
> One, the name sucks... perhaps apr_dir_is_empty()? Keep it named with the
> other apr_dir_ family members and our general schema. We have to work hard
> on consistency so we avoid symbol renames of newly created functions.
I've changed it to apr_dir_is_empty() in the Subversion sources, so it
will be that name when we port it over to APR.
> Other than that, I would _NOT_ assume all first two entries are "."/"..", but
> if you make that an explicit string test, I'd be +1.
Fixed now.
> Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
We should either make a new status code explicitly for this, or return
via an int* parameter. Personally I think the int* solution is
cleaner: the function should return an error status iff something went
wrong, and use an entirely separate channel for returning the answer.
Would anyone object to this solution?
-K
Re: proposal to add apr_check_dir_empty() to APR
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:48 PM 7/2/2002, Karl Fogel wrote:
>Currently, apr_check_dir_empty() is living in the Subversion source
>tree. The implementation looks portable, though; is there any reason
>not to move this into APR?
One, the name sucks... perhaps apr_dir_is_empty()? Keep it named with the
other apr_dir_ family members and our general schema. We have to work hard
on consistency so we avoid symbol renames of newly created functions.
Other than that, I would _NOT_ assume all first two entries are "."/"..", but
if you make that an explicit string test, I'd be +1.
Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
> apr_status_t
> apr_check_dir_empty (const char *path,
> apr_pool_t *pool)
> {
> apr_status_t apr_err, retval;
> apr_dir_t *dir;
> apr_finfo_t finfo;
>
> apr_err = apr_dir_open (&dir, path, pool);
> if (apr_err)
> return apr_err;
>
> /* All systems return "." and ".." as the first two files, so read
> past them unconditionally. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
>
> /* Now, there should be nothing left. If there is something left,
> return EGENERAL. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (APR_STATUS_IS_ENOENT (apr_err))
> retval = APR_SUCCESS;
> else if (! apr_err)
> retval = APR_EGENERAL;
> else
> retval = apr_err;
>
> apr_err = apr_dir_close (dir);
> if (apr_err)
> return apr_err;
>
> return retval;
> }
>
>Thoughts?,
>-Karl
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> The docs may say that, but the code doesn't do anything about "." and
> "..". I just double-checked unix/dir.c. :-( I actually remember
> looking at this years ago when I wrote the original code, and thinking
> through the "." and ".." problem, which is why I said that APR will
> return those two values first, but I don't think I ever actually coded
> it, and I'm not sure how it would be done.
Heh! Pardon me while I feel smug :-)...
I also felt a bit suspicious about that promise, and therefore wrote
this code in Subversion a while back:
for (err = svn_io_dir_read (&finfo, flags, dir, pool);
err == SVN_NO_ERROR;
svn_pool_clear (subpool),
err = svn_io_dir_read (&finfo, flags, dir, pool))
{
const char *this_path, *this_edit_path;
if (finfo.filetype == APR_DIR)
{
/* Skip entries for this dir and its parent.
(APR promises that they'll come first, so technically
this guard could be moved outside the loop. But somehow
that feels iffy. */
if (finfo.name[0] == '.'
&& (finfo.name[1] == '\0'
|| (finfo.name[1] == '.' && finfo.name[2] == '\0')))
continue;
[...]
}
[...]
}
Anyway, will do something similar in apr_dir_check_empty().
-K
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> The docs may say that, but the code doesn't do anything about "." and
> "..". I just double-checked unix/dir.c. :-( I actually remember
> looking at this years ago when I wrote the original code, and thinking
> through the "." and ".." problem, which is why I said that APR will
> return those two values first, but I don't think I ever actually coded
> it, and I'm not sure how it would be done.
Heh! Pardon me while I feel smug :-)...
I also felt a bit suspicious about that promise, and therefore wrote
this code in Subversion a while back:
for (err = svn_io_dir_read (&finfo, flags, dir, pool);
err == SVN_NO_ERROR;
svn_pool_clear (subpool),
err = svn_io_dir_read (&finfo, flags, dir, pool))
{
const char *this_path, *this_edit_path;
if (finfo.filetype == APR_DIR)
{
/* Skip entries for this dir and its parent.
(APR promises that they'll come first, so technically
this guard could be moved outside the loop. But somehow
that feels iffy. */
if (finfo.name[0] == '.'
&& (finfo.name[1] == '\0'
|| (finfo.name[1] == '.' && finfo.name[2] == '\0')))
continue;
[...]
}
[...]
}
Anyway, will do something similar in apr_dir_check_empty().
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > This isn't portable. POSIX specifically states that you don't need
to
> > return "." or "..", and APR doesn't make any comments about them.
>
> Actually, APR does promise that "." and ".." will be returned, and
> will be returned first, on all platforms.
>
> See apr/include/apr_file_info.h:
>
> /**
> * Read the next entry from the specified directory.
> * @param finfo the file info structure and filled in by
apr_dir_read
> * @param wanted The desired apr_finfo_t fields, as a bit flag of \
> APR_FINFO_ values
> * @param thedir the directory descriptor returned from
apr_dir_open
> * @remark All systems return . and .. as the first two files.
> */
> APR_DECLARE(apr_status_t) apr_dir_read(apr_finfo_t *finfo,
The docs may say that, but the code doesn't do anything about "." and
"..". I just double-checked unix/dir.c. :-( I actually remember
looking at this years ago when I wrote the original code, and thinking
through the "." and ".." problem, which is why I said that APR will
return those two values first, but I don't think I ever actually coded
it, and I'm not sure how it would be done.
Ryan
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> "Ryan Bloom" <rb...@covalent.net> writes:
> > This isn't portable. POSIX specifically states that you don't need
to
> > return "." or "..", and APR doesn't make any comments about them.
>
> Actually, APR does promise that "." and ".." will be returned, and
> will be returned first, on all platforms.
>
> See apr/include/apr_file_info.h:
>
> /**
> * Read the next entry from the specified directory.
> * @param finfo the file info structure and filled in by
apr_dir_read
> * @param wanted The desired apr_finfo_t fields, as a bit flag of \
> APR_FINFO_ values
> * @param thedir the directory descriptor returned from
apr_dir_open
> * @remark All systems return . and .. as the first two files.
> */
> APR_DECLARE(apr_status_t) apr_dir_read(apr_finfo_t *finfo,
The docs may say that, but the code doesn't do anything about "." and
"..". I just double-checked unix/dir.c. :-( I actually remember
looking at this years ago when I wrote the original code, and thinking
through the "." and ".." problem, which is why I said that APR will
return those two values first, but I don't think I ever actually coded
it, and I'm not sure how it would be done.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> This isn't portable. POSIX specifically states that you don't need to
> return "." or "..", and APR doesn't make any comments about them.
Actually, APR does promise that "." and ".." will be returned, and
will be returned first, on all platforms.
See apr/include/apr_file_info.h:
/**
* Read the next entry from the specified directory.
* @param finfo the file info structure and filled in by apr_dir_read
* @param wanted The desired apr_finfo_t fields, as a bit flag of \
APR_FINFO_ values
* @param thedir the directory descriptor returned from apr_dir_open
* @remark All systems return . and .. as the first two files.
*/
APR_DECLARE(apr_status_t) apr_dir_read(apr_finfo_t *finfo,
> The return code should not be EGENERAL if the directory isn't empty.
> Create a new status code if you have to, but a non-empty directory is
> not an error condition.
Agreed, can make up a better status code for this.
> If you fix those two problems, then I am all for this going into APR.
> However, can we get better performance on some platforms by using native
> functions???
I don't know. Anyone know?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: proposal to add apr_check_dir_empty() to APR
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> This isn't portable. POSIX specifically states that you don't need to
> return "." or "..", and APR doesn't make any comments about them.
Actually, APR does promise that "." and ".." will be returned, and
will be returned first, on all platforms.
See apr/include/apr_file_info.h:
/**
* Read the next entry from the specified directory.
* @param finfo the file info structure and filled in by apr_dir_read
* @param wanted The desired apr_finfo_t fields, as a bit flag of \
APR_FINFO_ values
* @param thedir the directory descriptor returned from apr_dir_open
* @remark All systems return . and .. as the first two files.
*/
APR_DECLARE(apr_status_t) apr_dir_read(apr_finfo_t *finfo,
> The return code should not be EGENERAL if the directory isn't empty.
> Create a new status code if you have to, but a non-empty directory is
> not an error condition.
Agreed, can make up a better status code for this.
> If you fix those two problems, then I am all for this going into APR.
> However, can we get better performance on some platforms by using native
> functions???
I don't know. Anyone know?
-K
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
It isn't portable. :-( Comments below.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> Currently, apr_check_dir_empty() is living in the Subversion source
> tree. The implementation looks portable, though; is there any reason
> not to move this into APR?
>
> apr_status_t
> apr_check_dir_empty (const char *path,
> apr_pool_t *pool)
> {
> apr_status_t apr_err, retval;
> apr_dir_t *dir;
> apr_finfo_t finfo;
>
> apr_err = apr_dir_open (&dir, path, pool);
> if (apr_err)
> return apr_err;
>
> /* All systems return "." and ".." as the first two files, so
read
> past them unconditionally. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
This isn't portable. POSIX specifically states that you don't need to
return "." or "..", and APR doesn't make any comments about them.
> /* Now, there should be nothing left. If there is something
left,
> return EGENERAL. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (APR_STATUS_IS_ENOENT (apr_err))
> retval = APR_SUCCESS;
> else if (! apr_err)
> retval = APR_EGENERAL;
> else
> retval = apr_err;
>
> apr_err = apr_dir_close (dir);
> if (apr_err)
> return apr_err;
>
> return retval;
> }
The return code should not be EGENERAL if the directory isn't empty.
Create a new status code if you have to, but a non-empty directory is
not an error condition.
If you fix those two problems, then I am all for this going into APR.
However, can we get better performance on some platforms by using native
functions???
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: proposal to add apr_check_dir_empty() to APR
Posted by Ryan Bloom <rb...@covalent.net>.
It isn't portable. :-( Comments below.
> From: Karl Fogel [mailto:kfogel@newton.ch.collab.net]
>
> Currently, apr_check_dir_empty() is living in the Subversion source
> tree. The implementation looks portable, though; is there any reason
> not to move this into APR?
>
> apr_status_t
> apr_check_dir_empty (const char *path,
> apr_pool_t *pool)
> {
> apr_status_t apr_err, retval;
> apr_dir_t *dir;
> apr_finfo_t finfo;
>
> apr_err = apr_dir_open (&dir, path, pool);
> if (apr_err)
> return apr_err;
>
> /* All systems return "." and ".." as the first two files, so
read
> past them unconditionally. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
This isn't portable. POSIX specifically states that you don't need to
return "." or "..", and APR doesn't make any comments about them.
> /* Now, there should be nothing left. If there is something
left,
> return EGENERAL. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (APR_STATUS_IS_ENOENT (apr_err))
> retval = APR_SUCCESS;
> else if (! apr_err)
> retval = APR_EGENERAL;
> else
> retval = apr_err;
>
> apr_err = apr_dir_close (dir);
> if (apr_err)
> return apr_err;
>
> return retval;
> }
The return code should not be EGENERAL if the directory isn't empty.
Create a new status code if you have to, but a non-empty directory is
not an error condition.
If you fix those two problems, then I am all for this going into APR.
However, can we get better performance on some platforms by using native
functions???
Ryan
Re: proposal to add apr_check_dir_empty() to APR
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:48 PM 7/2/2002, Karl Fogel wrote:
>Currently, apr_check_dir_empty() is living in the Subversion source
>tree. The implementation looks portable, though; is there any reason
>not to move this into APR?
One, the name sucks... perhaps apr_dir_is_empty()? Keep it named with the
other apr_dir_ family members and our general schema. We have to work hard
on consistency so we avoid symbol renames of newly created functions.
Other than that, I would _NOT_ assume all first two entries are "."/"..", but
if you make that an explicit string test, I'd be +1.
Also, APR_EGENERAL seems a bit vague, how about APR_EEXISTS?
> apr_status_t
> apr_check_dir_empty (const char *path,
> apr_pool_t *pool)
> {
> apr_status_t apr_err, retval;
> apr_dir_t *dir;
> apr_finfo_t finfo;
>
> apr_err = apr_dir_open (&dir, path, pool);
> if (apr_err)
> return apr_err;
>
> /* All systems return "." and ".." as the first two files, so read
> past them unconditionally. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (apr_err) return apr_err;
>
> /* Now, there should be nothing left. If there is something left,
> return EGENERAL. */
> apr_err = apr_dir_read (&finfo, APR_FINFO_NAME, dir);
> if (APR_STATUS_IS_ENOENT (apr_err))
> retval = APR_SUCCESS;
> else if (! apr_err)
> retval = APR_EGENERAL;
> else
> retval = apr_err;
>
> apr_err = apr_dir_close (dir);
> if (apr_err)
> return apr_err;
>
> return retval;
> }
>
>Thoughts?,
>-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org