apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@apache.org>
Subject Re: apr_mmap_t
Date Sun, 24 Nov 2002 16:07:23 GMT


On Sat, 23 Nov 2002, Justin Erenkrantz wrote:

> --On Saturday, November 23, 2002 7:31 AM -0800 rbb@apache.org wrote:
>
> > Incomplete types are not the solution to binary compatibility.  The
> > correct way to solve binary compat is to have developers who
> > understand what it means and why it is important.
>
> I'm with Jeff and Will here.  In apr_mmap_t, we're exposing lots of
> OS-dependent structures (see all of the #ifdefs).  There is no good
> reason for having these structures exposed.  If a program
> legitimately needs this information, we should have an
> apr_os_mmap_get or something like that.  Don't care much, but
> apr_mmap_t as it stands right now should be an incomplete type as it
> contains non-portable structure members that is intricately tied to
> the implementation.  That's a no-no.  The structure shouldn't be tied
> to the implementation.  -- justin

Let me make myself clear.  I haven't expressed an opinion about whether
they should or should not be incomplete.  I have stated why incomplete
types were originally used in APR, namely for portability.  I have also
said that having MMAPs be represented by anything other than a void
pointer and a size is a bit ludicrous.

This change was made over 2 years ago, and it was made to allow Apache to
do something.  I can't for the life of me remember what the change was for
in Apache, and we seem to have lost revision history for a lot of the
bucket code.  (I can't even get to the apache-2.0 tree from viewcvs right
now).  The best I can tell, the original implementation of apr_bucket_file
wanted access to the internals of the MMAP type.  That implementation
seems to have been changed though.

If you do change how the MMAP type works, I have a suggestion.  MMAP's are
void *'s with size's, and that is the only way to really make sense of an
MMAP.  Make the new structure something like:

/* $(platform)_mmap_t are incomplete types implemented per OS */
typedef union {
    win_mmap_t *win;
    unix_mmap_t *unix;
    boes_mmap_t *beos;
} apr_mmap_internal_t;

struct apr_mmap_t {
    apr_pool_t *p;
    apr_mmap_internal_t *os_specific;
    void *mm;
    apr_size_t size;
    int is_owner;   /* Is this still necessary? */
}

This gives all of the benefits of incomplete types (portability and
maintainability), but it also opens the truly portable portions of the
structure to the developer.  The goal with this design is to make the
portable parts of APR portable, and the non-portable parts hidden.

I thought of this a few months ago, but I have been sitting on it while we
try to get 1.0 out the door.  This is my vision for how APR 2.0 will be
implemented (I was going to propose it to the list after 1.0 had been
released).  This makes the API cleaner and smaller, because many of the
accessor functions can cleanly be deprecated (Think all of the userdata
functions).  It also solves a lot of problems with apr_proc_t's on
Windows, where we don't have anyplace to store OS specific information.

When I said a few days ago that I had hoped that many of the incomplete
types would go away in APR 2.0, this is what I meant.  I don't think we
should re-write APR 1.0 now, but if you are going to change MMAPs, then
please let's do it the right way.

Ryan


Mime
View raw message