You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Iain Wade <iw...@optusnet.com.au> on 2007/12/11 14:24:07 UTC
apr_os_dir_put fixes
Hi,
I believe the apr_os_dir_put function is a bit flawed as it stands.
It seems to support two behaviours:
a/ If it is passed a NULL pointer for a dir handle, it will allocate a
new object to use
b/ If it is passed an existing dir handle it will just replace the dirstruct.
In the first case however, it fails to allocate an entry buffer, setup
the cleanup handler, or register a dirname.
In the second case, you are left with a dirstruct which does not match
the dirname.
Without a correct dirname, it would seem the apr_stat-fill in
apr_read_dir will always fail because it would not be able to
construct the full path to the entry.
The apr_dir_t typedef is opaque, so fiddling the dirname after the
fact is not possible.
I have attached a patch which I would like you to consider, or I would
appreciate some alternate suggestions.
It has the drawback of altering the apr_os_dir_put() function to
accept an extra argument (dirname). I don't know your policy of stable
interfaces, but I haven't really been able to find any other users of
this call in my searching.
Regards,
--Iain
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
Lucian Adrian Grijincu wrote:
> On Dec 11, 2007 6:02 PM, Michael Clark <mi...@metaparadigm.com> wrote:
>
>> Lucian Adrian Grijincu wrote:
>>
>>> apart from the versioning detail, Iain raises a valid problem (bug) in APR:
>>>
>> Yes, although even with the allocation fixed, without dirname set
>> correctly, it will also not be able to do the apr_stat calls if they are
>> 'wanted' in apr_dir_read.
>>
>>
>
> A few things can still be found without changing the interface.
> struct dirent {
> ino_t d_ino; /* inode number */
> off_t d_off; /* offset to the next dirent */
> unsigned short d_reclen; /* length of this record */
> unsigned char d_type; /* type of file */
> char d_name[256]; /* filename */
> };
>
> The ino, type and name of the file can still be found, but not more.
> so, if you only need APR_FINFO_NAME|APR_FINFO_INODE|DIRENT_TYPE or
> less, you could go from here.
> Also, if you have the dirname to begin with, you can concatenate that
> with the .name member set up by apr_dir_read in apr_finfo_t and do it
> manually.
> Or am I missing something?
>
A use case example is making some existing code that reads from an
apr_dir_t work with an apr_dir_t that has been created using apr_os_dir_put.
Some code (e.g. mod_autoindex) is minimally changed to call a portable
interface that normally just dispatches to apr_dir_open. When a
privilege seperation feature is enabled, it instead delegates to a
routine that requests the directory file descriptors be sent over a
socket (or potentially a file handle over a memory file on windows).
By being able to set the dirname, have the apr_dir_t properly allocated,
this existing code can be changed minimally (a couple of 1 line
changes). With the limitations mentioned above, it would need to be
changed in a much more complicated way - ie. changing the wanted mask,
adding extra apr_stat calls, changing the dir_close call (for cleanup to
work), etc
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Dec 11, 2007 6:27 PM, Lucian Adrian Grijincu
<lu...@gmail.com> wrote:
> On Dec 11, 2007 6:02 PM, Michael Clark <mi...@metaparadigm.com> wrote:
> >
> > Lucian Adrian Grijincu wrote:
> > > apart from the versioning detail, Iain raises a valid problem (bug) in APR:
> > Yes, although even with the allocation fixed, without dirname set
> > correctly, it will also not be able to do the apr_stat calls if they are
> > 'wanted' in apr_dir_read.
> >
>
> A few things can still be found without changing the interface.
> struct dirent {
> ino_t d_ino; /* inode number */
> off_t d_off; /* offset to the next dirent */
> unsigned short d_reclen; /* length of this record */
> unsigned char d_type; /* type of file */
> char d_name[256]; /* filename */
> };
>
> The ino, type and name of the file can still be found, but not more.
> so, if you only need APR_FINFO_NAME|APR_FINFO_INODE|DIRENT_TYPE or
> less, you could go from here.
> Also, if you have the dirname to begin with, you can concatenate that
> with the .name member set up by apr_dir_read in apr_finfo_t and do it
> manually.
This is against the 1.2.x HEAD.
It adds allocation for ->entry for unix.
There's a comment for the doxygen header files stating the limitation.
Index: file_io/unix/dir.c
===================================================================
--- file_io/unix/dir.c (revision 603897)
+++ file_io/unix/dir.c (working copy)
@@ -312,12 +312,15 @@
apr_status_t apr_os_dir_put(apr_dir_t **dir, apr_os_dir_t *thedir,
apr_pool_t *pool)
{
+ const apr_size_t dirent_size =
+ (sizeof((*dir)->entry->d_name) > 1 ?
+ sizeof(struct dirent) : sizeof (struct dirent) + 255);
if ((*dir) == NULL) {
(*dir) = (apr_dir_t *)apr_pcalloc(pool, sizeof(apr_dir_t));
(*dir)->pool = pool;
+ (*dir)->entry = apr_pcalloc(pool, dirent_size);
}
(*dir)->dirstruct = thedir;
return APR_SUCCESS;
}
-
Index: include/apr_portable.h
===================================================================
--- include/apr_portable.h (revision 603897)
+++ include/apr_portable.h (working copy)
@@ -373,6 +373,11 @@
* @param dir The apr dir we are converting to.
* @param thedir The os specific dir to convert
* @param cont The pool to use when creating to apr directory.
+ * @bug If you use *dir with apr_dir_read, you can only ask for
+ * APR_FINFO_INODE, APR_FINFO_TYPE and APR_FINFO_NAME.
+ * If you want other fields you should concatenate the
+ * name of the file with the directory name and issue a
+ * apr_stat on the full path to the file.
*/
APR_DECLARE(apr_status_t) apr_os_dir_put(apr_dir_t **dir,
apr_os_dir_t *thedir,
--
Lucian
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Dec 11, 2007 6:02 PM, Michael Clark <mi...@metaparadigm.com> wrote:
>
> Lucian Adrian Grijincu wrote:
> > apart from the versioning detail, Iain raises a valid problem (bug) in APR:
> Yes, although even with the allocation fixed, without dirname set
> correctly, it will also not be able to do the apr_stat calls if they are
> 'wanted' in apr_dir_read.
>
A few things can still be found without changing the interface.
struct dirent {
ino_t d_ino; /* inode number */
off_t d_off; /* offset to the next dirent */
unsigned short d_reclen; /* length of this record */
unsigned char d_type; /* type of file */
char d_name[256]; /* filename */
};
The ino, type and name of the file can still be found, but not more.
so, if you only need APR_FINFO_NAME|APR_FINFO_INODE|DIRENT_TYPE or
less, you could go from here.
Also, if you have the dirname to begin with, you can concatenate that
with the .name member set up by apr_dir_read in apr_finfo_t and do it
manually.
Or am I missing something?
> So the 'bug' can't really be fixed without changing the interface or
> adding an apr_dir_set_dirname(d, dirname)
>
> I don't think it is a very useful interface - should it be deprecated
> perhaps?
>
> We have some code that uses the existing interface but it needs to
> include arch/unix/apr_arch_file_io.h to access the dirname member to do
> the fixups.
>
Either that or add some new functionality to route around the problem.
a function like apr_dir_set_dirname, or making part of the apr_dir_t
public and some of this an *impl pointer.
I'd favor a better _ex() variant though.
--
Lucian
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Tue, Apr 15, 2008 at 6:07 PM, William A. Rowe, Jr.
<wr...@rowe-clan.net> wrote:
>
> Lucian Adrian Grijincu wrote:
>
> > We discussed in this[1] thread has discussed the problem, patches were
> > sent, a bug[2] was filled.
> >
> > Was this issue fixed and I missed the commit, or was it forgotten :) ?
> >
> > [1] http://www.mail-archive.com/dev@apr.apache.org/msg19497.html
> > [2] https://issues.apache.org/bugzilla/show_bug.cgi?id=44129
> >
> > Shouldn't this go in 1.3.x before the release?
> > And for that matter in trunk/ also?
> >
>
> Righto... first patch broke the ABI rule, attached to 44129 resolved this
> issue for trunk/1.3.x.
>
> I'm not able to now, hopefully someone else can so there is extra review
> (I'd commit just before rolling 1.3.0 - it looks right, but can't now).
I see William is sweeping through the bugzilla, so I'm bumping this once more.
--
Lucian
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Tue, Apr 15, 2008 at 6:07 PM, William A. Rowe, Jr.
<wr...@rowe-clan.net> wrote:
> Lucian Adrian Grijincu wrote:
>>
>> We discussed in this[1] thread has discussed the problem, patches were
>> sent, a bug[2] was filled.
>>
>> Was this issue fixed and I missed the commit, or was it forgotten :) ?
>>
>> [1] http://www.mail-archive.com/dev@apr.apache.org/msg19497.html
>> [2] https://issues.apache.org/bugzilla/show_bug.cgi?id=44129
>>
>> Shouldn't this go in 1.3.x before the release?
>> And for that matter in trunk/ also?
>
> Righto... first patch broke the ABI rule, attached to 44129 resolved this
> issue for trunk/1.3.x.
>
> I'm not able to now, hopefully someone else can so there is extra review
> (I'd commit just before rolling 1.3.0 - it looks right, but can't now).
>
William, or anyone else for that matter, could you have a last look at this?
This patch is in dire need of some commiter love.
The last change (apart from the taging) on 1.3.x's unix/dir.c was way
before the patch was commited so this should fit perfectly.
--
Lucian
Re: apr_os_dir_put fixes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
> We discussed in this[1] thread has discussed the problem, patches were
> sent, a bug[2] was filled.
>
> Was this issue fixed and I missed the commit, or was it forgotten :) ?
>
> [1] http://www.mail-archive.com/dev@apr.apache.org/msg19497.html
> [2] https://issues.apache.org/bugzilla/show_bug.cgi?id=44129
>
> Shouldn't this go in 1.3.x before the release?
> And for that matter in trunk/ also?
Righto... first patch broke the ABI rule, attached to 44129 resolved this
issue for trunk/1.3.x.
I'm not able to now, hopefully someone else can so there is extra review
(I'd commit just before rolling 1.3.0 - it looks right, but can't now).
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
We discussed in this[1] thread has discussed the problem, patches were
sent, a bug[2] was filled.
Was this issue fixed and I missed the commit, or was it forgotten :) ?
[1] http://www.mail-archive.com/dev@apr.apache.org/msg19497.html
[2] https://issues.apache.org/bugzilla/show_bug.cgi?id=44129
Shouldn't this go in 1.3.x before the release?
And for that matter in trunk/ also?
--
Lucian
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
Iain Wade wrote:
> New summary:
>
> dir.patch:
> 1/ add apr_dir_put_ex, adding dirname and flags
> 2/ refactor apr_dir_open to use apr_dir_put_ex
> 3/ add apr_dir_name_get
> 4/ add apr_dir_pool_get
>
> file.patch:
> 1/ add apr_os_file_put_ex, changing behavior to honour
> APR_FILE_NOCLEANUP set/unset.
> 2/ refactor apr_file_open to use apr_dir_put_ex
>
OK. So the _ex variants register a cleanup unless you pass
APR_(FILE|DIR)_NOCLEANUP flag - so both behaviours are possible.
I have tested the privsep patches with changes to use these new APIs and
they work as expected.
There is no longer a need to delve into the internals of the apr_dir_t
struct to get a working apr_dir_t from a apr_os_dir_t or to access non
public APIs to get files cleaned up when apr_file_close is called on an
apr_file_t created with apr_os_file_put_ex.
I've also tested these patches with a standard httpd-trunk exercising
the normal path apr_dir_open/apr_file_open.
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
Lucian Adrian Grijincu wrote:
> I'd like to raise an API consistency issue here.
> (I've included Iain's original patches so that you can look at the code easily)
> the file and pipe APIs allocate space for apr_file_t regardless of the
> value of the passed in parameter.
> the dir API allocates space for apr_dir_t only if the passed in
> parameter is not a pointer to NULL. If it's a pointer to a non-NULL
> value, we consider that at that location is a full apr_dir_t object
> and use it as such.
>
> Apart from the different behavior there's an ugliness issues here too:
> say I have two pools : p0 and p1.
> I allocate a valid apr_dir_t from p0 with apr_dir_open(&dir, ..., p0).
> then I do a apr_os_dir_put_ex(&dir, ..., p1).
> now my object uses two pools to allocate it's data and is dependent on
> both, which I find ugly.
> Ugly are the next alternatives too ...
>
> A) decide the pool to use based on the value of the apr_dir_t.
> if apr_dir_t * is NULL allocate it from the pool pased in to
> apr_os_dir_put[_ex]
> if it's not NULL ignore the passed in pool and allocate it from the
> original pool (which can be accessed through dir->pool
>
> B) provide a flag through the user can specify which behavior he wants
> in case the passed in parameter is not null
> this can currently be done by making dir = NULL before passing it
> to apr_os_dir_put[_ex] but with a flag it's more explicit.
>
> C) always allocate from the new pool and ignore the value of the
> apr_dir_t object (this is the behavior for apr_file_t objects)
>
I would go for C.
The particular use case that raised the need for the patches was to
create a file or directory from an apr_os_file_t or apr_os_dir_t (which
means we always need it to allocate from the passed in pool).
We have the existing apr_os_file_put and apr_os_dir_put for where we
want to replace the descriptor in an existing apr_file_t or apr_dir_t
(well at least the put sort of implies that)
Perhaps we could instead name them "apr_file_open_os" and
"apr_dir_open_os" (then we don't need to have an _ex variant) and make
them always allocate as do apr_file_open and apr_dir_open?
Michael.
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Dec 13, 2007 9:12 AM, Iain Wade <iw...@optusnet.com.au> wrote:
[snip]
> New summary:
>
> dir.patch:
> 1/ add apr_dir_put_ex, adding dirname and flags
> 2/ refactor apr_dir_open to use apr_dir_put_ex
> 3/ add apr_dir_name_get
> 4/ add apr_dir_pool_get
>
> file.patch:
> 1/ add apr_os_file_put_ex, changing behavior to honour
> APR_FILE_NOCLEANUP set/unset.
> 2/ refactor apr_file_open to use apr_dir_put_ex
>
> pipe.patch (not stricly necessary):
> 1/ add apr_os_pipe_flags_put, replacing register_cleanup bool with flags
> 2/ refactor apr_file_pipe_create to use apr_os_pipe_flags_put
>
I'd like to raise an API consistency issue here.
(I've included Iain's original patches so that you can look at the code easily)
the file and pipe APIs allocate space for apr_file_t regardless of the
value of the passed in parameter.
the dir API allocates space for apr_dir_t only if the passed in
parameter is not a pointer to NULL. If it's a pointer to a non-NULL
value, we consider that at that location is a full apr_dir_t object
and use it as such.
Apart from the different behavior there's an ugliness issues here too:
say I have two pools : p0 and p1.
I allocate a valid apr_dir_t from p0 with apr_dir_open(&dir, ..., p0).
then I do a apr_os_dir_put_ex(&dir, ..., p1).
now my object uses two pools to allocate it's data and is dependent on
both, which I find ugly.
Ugly are the next alternatives too ...
A) decide the pool to use based on the value of the apr_dir_t.
if apr_dir_t * is NULL allocate it from the pool pased in to
apr_os_dir_put[_ex]
if it's not NULL ignore the passed in pool and allocate it from the
original pool (which can be accessed through dir->pool
B) provide a flag through the user can specify which behavior he wants
in case the passed in parameter is not null
this can currently be done by making dir = NULL before passing it
to apr_os_dir_put[_ex] but with a flag it's more explicit.
C) always allocate from the new pool and ignore the value of the
apr_dir_t object (this is the behavior for apr_file_t objects)
One more thing about apr_os_file_put[_ex]:
rv = apr_thread_mutex_create(&((*file)->thlock),
APR_THREAD_MUTEX_DEFAULT, pool);
We allocate this mutex for every call. Couldn't we reuse a previously
allocated one (in case the passed in apr_file_t* was not NULL and
pointing to a valid object)?
--
Lucian
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
On 12/13/07, Michael Clark <mi...@metaparadigm.com> wrote:
> William A. Rowe, Jr. wrote:
> > Iain Wade wrote:
> >> 1/ Create a new apr_os_dir_put_ex function, which adds a dirname field
> >> and register_cleanup flag.
> >
> > IFF you agree with my comments on 2) below, is there an advantage to an
> > extensible flags arg, with the same flag bit for this function?
>
> an additional flag argument might make more sense - and then use
> APR_DIR_CLEANUP?
Ok, I've got it using flags now with APR_DIR_CLEANUP.
I agree it's nicer.
> >> 2/ Create a new apr_os_file_put_ex function, which adds a
> >> register_cleanup flag.
> >
> > Why a bool? We already pass a flags arg, why not a toggle bit?
>
> I guess it was because of the existing precedence for the
> register_cleanup arg on apr_os_pipe_put_ex
It was.
I now have an apr_os_pipe_flags_put function which may or may not be worthwhile.
> So for this one we would use the existing apr_os_file_put but add an
> extra APR_FILE_CLEANUP flag?
The apr_os_file_put function already took a flags set, but it overrode
APR_FILE_CLEANUP.
I've got a new _ex version which corrects the flag behaviour.
> Do you think it is reasonable to have an apr_dir_pool_get?
Sure. It's only a few lines to write.
New summary:
dir.patch:
1/ add apr_dir_put_ex, adding dirname and flags
2/ refactor apr_dir_open to use apr_dir_put_ex
3/ add apr_dir_name_get
4/ add apr_dir_pool_get
file.patch:
1/ add apr_os_file_put_ex, changing behavior to honour
APR_FILE_NOCLEANUP set/unset.
2/ refactor apr_file_open to use apr_dir_put_ex
pipe.patch (not stricly necessary):
1/ add apr_os_pipe_flags_put, replacing register_cleanup bool with flags
2/ refactor apr_file_pipe_create to use apr_os_pipe_flags_put
--Iain
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
William A. Rowe, Jr. wrote:
> Iain Wade wrote:
>> 1/ Create a new apr_os_dir_put_ex function, which adds a dirname field
>> and register_cleanup flag.
>
> IFF you agree with my comments on 2) below, is there an advantage to an
> extensible flags arg, with the same flag bit for this function?
an additional flag argument might make more sense - and then use
APR_DIR_CLEANUP?
>> 2/ Create a new apr_os_file_put_ex function, which adds a
>> register_cleanup flag.
>
> Why a bool? We already pass a flags arg, why not a toggle bit?
I guess it was because of the existing precedence for the
register_cleanup arg on apr_os_pipe_put_ex
So for this one we would use the existing apr_os_file_put but add an
extra APR_FILE_CLEANUP flag?
>> 3/ Create a new apr_dir_name_get function which returns the directory
>> name
>>
>> 4/ Re-factor apr_(file|dir)_open to use apr_os_(file|dir)_put for
>> consistency.
>
> Interesting, I'll need to review this a bit. Perhaps that should be
> broken
> out as a seperate patch, and 1 and 3 considered together, 2 considered
> on it's
> own (in conjunction with 1).
I will do some testing on these patches.
From what I can see, API-wise, this solves most of the issues we had
with apr in implementing a wrapped apr_dir_open that creates an
apr_dir_t from a file descriptor and doing this only with public apr
interfaces (i.e. not looking inside and/or fiddling with the opaque
apr_dir_t structure using platform headers).
There is one outstanding issue from an apr API point of view. We have a
wrapper for apr_dir_read that needs the pool but there is no pool
argument so the only context is the apr_dir_t itself but we can't access
the pool from the opaque structure from outside of apr.
Do you think it is reasonable to have an apr_dir_pool_get?
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
On 12/13/07, Iain Wade <iw...@optusnet.com.au> wrote:
> > If I depended on the existing flag, maintaining existing behavior of
> > apr_os_file_put would be tough.
>
> Hmm .. I think I'm confusing something else here. It doesn't seem correct.
Oh right, I remember now:
If apr_os_file_put defaults to no cleanup, and _ex does cleanup by
default, and later on _ex becomes the default - is it ok that the new
api has a behavioural change with the same signature?
It seems very fragile: how would anyone notice?
I'd be happy enough to change all the register_cleanup bool's to
flags, but what should I do with apr_os_pipe_ex?
leave it inconsistent or fix?
should the fixed one be called _ex_ex etc..?
--Iain
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
> If I depended on the existing flag, maintaining existing behavior of
> apr_os_file_put would be tough.
Hmm .. I think I'm confusing something else here. It doesn't seem correct.
--Iain
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
> > 2/ Create a new apr_os_file_put_ex function, which adds a register_cleanup flag.
>
> Why a bool? We already pass a flags arg, why not a toggle bit?
I know what you mean, I was torn in adding the bool myself but chose to because:
I note that apr_os_pipe_put_ex already exists with a register_cleanup bool.
APR_FILE_NOCLEANUP is actually APR_FOPEN_NOCLEANUP which doesn't seem
correctly named for a non-open case.
If I depended on the existing flag, maintaining existing behavior of
apr_os_file_put would be tough.
The apr_*dir* functions don't accept flags yet, and if I add one then
the dir function would take a different flag - confusing.
> Interesting, I'll need to review this a bit. Perhaps that should be broken
> out as a seperate patch, and 1 and 3 considered together, 2 considered on it's
> own (in conjunction with 1).
Sure, I'll get them over shortly.
--Iain
Re: apr_os_dir_put fixes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Iain Wade wrote:
>
> Ok, I've got a new version of the patch attached.
You definately have the right idea, well done! Comments below
> In summary:
>
> 1/ Create a new apr_os_dir_put_ex function, which adds a dirname field
> and register_cleanup flag.
IFF you agree with my comments on 2) below, is there an advantage to an
extensible flags arg, with the same flag bit for this function?
> 2/ Create a new apr_os_file_put_ex function, which adds a register_cleanup flag.
Why a bool? We already pass a flags arg, why not a toggle bit?
> 3/ Create a new apr_dir_name_get function which returns the directory name
>
> 4/ Re-factor apr_(file|dir)_open to use apr_os_(file|dir)_put for consistency.
Interesting, I'll need to review this a bit. Perhaps that should be broken
out as a seperate patch, and 1 and 3 considered together, 2 considered on it's
own (in conjunction with 1).
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
> > If there was a newer interface added, it might be nice to have an
> > 'owner' argument in addition to dirname and let it register the cleanup
> > if it is owner.
> >
>
> Yeah, that's nice.
Ok, I've got a new version of the patch attached.
In summary:
1/ Create a new apr_os_dir_put_ex function, which adds a dirname field
and register_cleanup flag.
2/ Create a new apr_os_file_put_ex function, which adds a register_cleanup flag.
3/ Create a new apr_dir_name_get function which returns the directory name
4/ Re-factor apr_(file|dir)_open to use apr_os_(file|dir)_put for consistency.
It passes the test cases (such that they are) on Linux .. I have
included changes for os2/win32 but they have not been tested.
If you would like me to break the patch down into separate parts I'm
happy to do so.
Thanks,
--Iain
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Dec 11, 2007 6:19 PM, Michael Clark <mi...@metaparadigm.com> wrote:
> Michael Clark wrote:
> > Lucian Adrian Grijincu wrote:
> >> I don't know if dir_cleanup should be registered to handle the
> >> cleanup of
> >> this apr_dir_t (I'm against it). I got the apr_os_dir_t from some place
> >> else, I should manage it's death manually.
> >> If I want to let APR call closedir() on this object I can register a
> >> cleanup function for it on the same pool, manually.
>
> If there was a newer interface added, it might be nice to have an
> 'owner' argument in addition to dirname and let it register the cleanup
> if it is owner.
>
Yeah, that's nice.
> Otherwise it is not possible to get apr_dir_close to work on one of
> these dirs as dir_cleanup is static and can't be registered manually
> from outside or apr - so you a forced to get lazy cleanup only.
>
Also it's not part of the interface, so APR is free to diss it, rename
it, inline it etc.
You could have a my_portable_os_kill_dir function with different
implementations for each platfrom you target.
Register your own killer. If you use a apr_os_dir_t you have platform
specific code anyway (or so I presume), so this shouldn't be too ugly
to integrate.
--
Lucian
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
Michael Clark wrote:
> Lucian Adrian Grijincu wrote:
>> I don't know if dir_cleanup should be registered to handle the
>> cleanup of
>> this apr_dir_t (I'm against it). I got the apr_os_dir_t from some place
>> else, I should manage it's death manually.
>> If I want to let APR call closedir() on this object I can register a
>> cleanup function for it on the same pool, manually.
If there was a newer interface added, it might be nice to have an
'owner' argument in addition to dirname and let it register the cleanup
if it is owner.
Otherwise it is not possible to get apr_dir_close to work on one of
these dirs as dir_cleanup is static and can't be registered manually
from outside or apr - so you a forced to get lazy cleanup only.
Re: apr_os_dir_put fixes
Posted by Michael Clark <mi...@metaparadigm.com>.
Lucian Adrian Grijincu wrote:
> apart from the versioning detail, Iain raises a valid problem (bug) in APR:
>
> apr_dir_t objects are used to iterate dir entries (and get apr_finfo_t
> structures with some info about each dir entry).
>
> Scenario:
> I have a apr_os_dir_t object from some irelevant place in my program and
> I want to list all the files in that directory.
>
> For this I use apr_os_dir_put as such:
>
> apr_dir_t * d = NULL;
> apr_os_dir_put(&d, os_dir, pool);
> do{
> rc = apr_dir_read(&finfo, wanted, d);
> printf("%s\n", finfo.name);
> }while(APR_SUCCESS == rc);
>
>
> apr_os_dir_put does not initialize the apr_dir_t->entry field, but
> apr_dir_read does:
> ret = readdir64_r(thedir->dirstruct, thedir->entry, &retent);
>
> This tries to fill the thedir->entry with some valid data about a file.
> (ino_t, d_off, d_name, etc.).
>
> This is a real problem only if APR endorses such a scenario.
>
> I don't know if dir_cleanup should be registered to handle the cleanup of
> this apr_dir_t (I'm against it). I got the apr_os_dir_t from some place
> else, I should manage it's death manually.
> If I want to let APR call closedir() on this object I can register a
> cleanup function for it on the same pool, manually.
>
Yes, although even with the allocation fixed, without dirname set
correctly, it will also not be able to do the apr_stat calls if they are
'wanted' in apr_dir_read.
So the 'bug' can't really be fixed without changing the interface or
adding an apr_dir_set_dirname(d, dirname)
I don't think it is a very useful interface - should it be deprecated
perhaps?
We have some code that uses the existing interface but it needs to
include arch/unix/apr_arch_file_io.h to access the dirname member to do
the fixups.
I can't see a nice way to create an apr_dir_t from a DIR* with apr 1.2
~mc
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
apart from the versioning detail, Iain raises a valid problem (bug) in APR:
apr_dir_t objects are used to iterate dir entries (and get apr_finfo_t
structures with some info about each dir entry).
Scenario:
I have a apr_os_dir_t object from some irelevant place in my program and
I want to list all the files in that directory.
For this I use apr_os_dir_put as such:
apr_dir_t * d = NULL;
apr_os_dir_put(&d, os_dir, pool);
do{
rc = apr_dir_read(&finfo, wanted, d);
printf("%s\n", finfo.name);
}while(APR_SUCCESS == rc);
apr_os_dir_put does not initialize the apr_dir_t->entry field, but
apr_dir_read does:
ret = readdir64_r(thedir->dirstruct, thedir->entry, &retent);
This tries to fill the thedir->entry with some valid data about a file.
(ino_t, d_off, d_name, etc.).
This is a real problem only if APR endorses such a scenario.
I don't know if dir_cleanup should be registered to handle the cleanup of
this apr_dir_t (I'm against it). I got the apr_os_dir_t from some place
else, I should manage it's death manually.
If I want to let APR call closedir() on this object I can register a
cleanup function for it on the same pool, manually.
On Dec 11, 2007 3:44 PM, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Iain Wade wrote:
> >
> > It has the drawback of altering the apr_os_dir_put() function to
> > accept an extra argument (dirname). I don't know your policy of stable
> > interfaces, but I haven't really been able to find any other users of
> > this call in my searching.
>
> Irrelevant, our versioning policy is really clear (and becoming more
> pedantic <shrug/>).
>
> http://apr.apache.org/versioning.html
>
> If you look, you'll find some _ex() flavor functions. These are the
> byproduct that we can add a new facility to 1.3.0 (which is still in
> development) and cannot modify an existing one.
>
> _ex() flavors tend to go poof when the original is marked @deprecated
> and the new _ex() flavor becomes the 'normal' flavor in 2.0.0, which
> is some ways off.
>
--
Lucian
Re: apr_os_dir_put fixes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Iain Wade wrote:
>
> It has the drawback of altering the apr_os_dir_put() function to
> accept an extra argument (dirname). I don't know your policy of stable
> interfaces, but I haven't really been able to find any other users of
> this call in my searching.
Irrelevant, our versioning policy is really clear (and becoming more
pedantic <shrug/>).
http://apr.apache.org/versioning.html
If you look, you'll find some _ex() flavor functions. These are the
byproduct that we can add a new facility to 1.3.0 (which is still in
development) and cannot modify an existing one.
_ex() flavors tend to go poof when the original is marked @deprecated
and the new _ex() flavor becomes the 'normal' flavor in 2.0.0, which
is some ways off.
Re: apr_os_dir_put fixes
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On Dec 11, 2007 3:24 PM, Iain Wade <iw...@optusnet.com.au> wrote:
> Hi,
[snip]
> It has the drawback of altering the apr_os_dir_put() function to
> accept an extra argument (dirname). I don't know your policy of stable
> interfaces, but I haven't really been able to find any other users of
> this call in my searching.
>
http://apr.apache.org/versioning.html
In the 1.2.x releases no type, identifier, interpretation of a
parameter, function, etc. shall be added/modified.
This release series is a bug-fix only.
http://apr.apache.org/anonsvn.html
the trunk now contains the next-to-be APR-1.3.x
Until 1.3.0 is released we can add new functions/types/etc. but
anything that was in 1.2.x must be preserved in 1.3.x as also.
So, if your patch is to be considered it must be done against the
1.3.x trunk, and must create a new function.
--
Lucian
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
Patch attached. doh!
--Iain
On 12/12/07, Iain Wade <iw...@optusnet.com.au> wrote:
> I appreciate the feedback Lucian & William,
>
> I have created an _ex() version against the SVN trunk.
>
> Also, all the apr_dir_open and apr_os_dir_put functions now call up to
> apr_os_dir_put_ex to avoid duplication.
>
> Would this patch be considered for a 1.3 release?
>
> Regards,
> --Iain
>
> On 12/12/07, Iain Wade <iw...@optusnet.com.au> wrote:
> > Hi,
> >
> > I believe the apr_os_dir_put function is a bit flawed as it stands.
> >
> > It seems to support two behaviours:
> >
> > a/ If it is passed a NULL pointer for a dir handle, it will allocate a
> > new object to use
> > b/ If it is passed an existing dir handle it will just replace the dirstruct.
> >
> > In the first case however, it fails to allocate an entry buffer, setup
> > the cleanup handler, or register a dirname.
> >
> > In the second case, you are left with a dirstruct which does not match
> > the dirname.
> >
> > Without a correct dirname, it would seem the apr_stat-fill in
> > apr_read_dir will always fail because it would not be able to
> > construct the full path to the entry.
> >
> > The apr_dir_t typedef is opaque, so fiddling the dirname after the
> > fact is not possible.
> >
> > I have attached a patch which I would like you to consider, or I would
> > appreciate some alternate suggestions.
> >
> > It has the drawback of altering the apr_os_dir_put() function to
> > accept an extra argument (dirname). I don't know your policy of stable
> > interfaces, but I haven't really been able to find any other users of
> > this call in my searching.
> >
> > Regards,
> > --Iain
> >
> >
>
Re: apr_os_dir_put fixes
Posted by Iain Wade <iw...@optusnet.com.au>.
I appreciate the feedback Lucian & William,
I have created an _ex() version against the SVN trunk.
Also, all the apr_dir_open and apr_os_dir_put functions now call up to
apr_os_dir_put_ex to avoid duplication.
Would this patch be considered for a 1.3 release?
Regards,
--Iain
On 12/12/07, Iain Wade <iw...@optusnet.com.au> wrote:
> Hi,
>
> I believe the apr_os_dir_put function is a bit flawed as it stands.
>
> It seems to support two behaviours:
>
> a/ If it is passed a NULL pointer for a dir handle, it will allocate a
> new object to use
> b/ If it is passed an existing dir handle it will just replace the dirstruct.
>
> In the first case however, it fails to allocate an entry buffer, setup
> the cleanup handler, or register a dirname.
>
> In the second case, you are left with a dirstruct which does not match
> the dirname.
>
> Without a correct dirname, it would seem the apr_stat-fill in
> apr_read_dir will always fail because it would not be able to
> construct the full path to the entry.
>
> The apr_dir_t typedef is opaque, so fiddling the dirname after the
> fact is not possible.
>
> I have attached a patch which I would like you to consider, or I would
> appreciate some alternate suggestions.
>
> It has the drawback of altering the apr_os_dir_put() function to
> accept an extra argument (dirname). I don't know your policy of stable
> interfaces, but I haven't really been able to find any other users of
> this call in my searching.
>
> Regards,
> --Iain
>
>