[PATCH] ap_sendfile try #1

[John Zedlewski <ze...@Princeton.EDU>]

Well, here's an initial look at an implementation for ap_sendfile in the Unix
APR.  I work on Linux, but I also wrote (untested) versions for the HP-UX and
FreeBSD APIs so I could see how different they'd be.  Currently, the three
OS-specific versions are completely separate.  What will be the neatest way to
format these eventually?  AIX is quite weird, but it does fit with the interface
described above and I can give a crack at it some time soon if this format turns
out to be acceptable.
ap_sendfile( ) should, as I think Manoj pointed out, work just like read or
write: it might not do a full write, but it stores the number of bytes written in
*len.  It would fail to write the complete number if (a) It's used on a
non-blocking socket, or (b) it's interrupted by a signal.
This stuff is REALLY platform-specific, but at least it's all going to be stuck
in APR instead of trailing throughout the main code.  I suspect there will be
some issues/objections with this code, so I didn't make the necessary changes to
get http_core to use the new API or to change the Win32 interface.  I can do that
when/if a final version is decided upon.
Thanks!
--JRZ

Re: [PATCH] ap_sendfile try #1

[Greg Stein <gs...@lyra.org>]

On Fri, 12 Nov 1999, John Zedlewski wrote:
> Well, here's an initial look at an implementation for ap_sendfile in the Unix
> APR.  I work on Linux, but I also wrote (untested) versions for the HP-UX and
> FreeBSD APIs so I could see how different they'd be.  Currently, the three
> OS-specific versions are completely separate.  What will be the neatest way to
> format these eventually?

Uh oh.. don't go there... :-)

We had this "discussion" a while back on the list. Some people said to
share the function definition and #ifdef the interior contents; others
said to #ifdef the entire function (including the definition line(s)).

I think the end result was "whoever implements the code will use their
preferred pattern, but people can always go through and retrofit if it
spins their bottle."

About the patch you attached. Couple things:
1) there is a lot of extraneous, unrelated stuff that appears to be a diff
   caused by formatting/spacing differences. That should be removed from
   the patch so that it is easier to look at the "real" change.
2) the patch is reversed. All your changes have "-" in front of them,
   rather than adding the lines :-)

Cheers,
-g

--
Greg Stein, http://www.lyra.org/

Re: [PATCH] ap_sendfile try #1

[Ryan Bloom <rb...@raleigh.ibm.com>]

On Fri, 12 Nov 1999, John Zedlewski wrote:

> Well, here's an initial look at an implementation for ap_sendfile in the Unix
> APR.  I work on Linux, but I also wrote (untested) versions for the HP-UX and
> FreeBSD APIs so I could see how different they'd be.  Currently, the three
> OS-specific versions are completely separate.  What will be the neatest way to
> format these eventually?  AIX is quite weird, but it does fit with the interface
> described above and I can give a crack at it some time soon if this format turns
> out to be acceptable.

As Greg said, there have been multiple discussions about this in the past.
The rule I have been working with is if >50% of the code is common try to
put everything in one function and use #ifdef's.  If < 50% of the code is
common, just #ifdef around the whole function.

As a side note, I had a chance to look through some code that used the
always #ifdef the code within the function earlier this week.  I couldn't
read it.  I couldn't follow which ifdef was for which platform, and it
only dealt with three platforms.  Everybody else I showed the code to
also had the same problem.

> This stuff is REALLY platform-specific, but at least it's all going to be stuck
> in APR instead of trailing throughout the main code.  I suspect there will be
> some issues/objections with this code, so I didn't make the necessary changes to
> get http_core to use the new API or to change the Win32 interface.  I can do that
> when/if a final version is decided upon.

Could you please clean up the patch a bit.  Greg had a couple of good
points, but there is more cleaning needed as well.

1)  Please try to keep the lines wrapping nicely, it just makes the code
easier to look at.

2)  Please remove all of the printf's.  This is a library function, and
the printf's don't make sense in a library function.  You should be
returning sensable error codes (Errno in most cases), and if you need to
document why it failed, use comments.

3)  There is no reason for ap_sendfile (an APR function) to use
ap_get_os_file, inside APR, the compiler won't complain if you just grab
the field from the structure.

4)  Please review the Apache style guide (on dev.apache.org) and the
APRDesign (from 2.0 CVS tree) docs.  Your format is a little off, and
either it needs to be fixed in the next patch or whoever applies the
patch has to do it by hand.  It is easier for you to just fix it for the
next patch.

5)  Please put docs into the code above the first copy of ap_sendfile.
The format for APR docs is relatively easy, and can be seen above ALL of
the current APR functions.

I'll look at it in more detail later today.

Ryan

_______________________________________________________________________
Ryan Bloom		rbb@raleigh.ibm.com
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.