86474 - nsFileTransports won't truncate files

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[gordon]

NSPR doesn't have an xp truncate function, so we'll need to access the private
OS specific osfd and call platform specific code to truncate files.

Comment 1

[Gagan]

Lets try these on the trunk and if they do well there we can try bringing them 
back in the 0.9.2 branch.

Comment 2

[gordon]

Darin, I hope to get some time this evening to make a rough draft of the
truncation code.  I'll post either a patch or an apology later tonight.

Comment 3

[gordon]

I apologize, I wasn't able to get to this before I had to leave.  Simon Fraser
may be able to help you with the Mac code  - once you find the right place to
call "truncate".

Comment 4

[Darin Fisher]

Attachment: v0.9 preliminary patch (needs WIN32 port)

Comment 5

[Darin Fisher]

Attachment: v1.0 linux, windows, mac platforms covered

Comment 6

[Darin Fisher]

v1.0 of the patch gets the job done on linux, windows, and mac.  i would have
coded it for all unix platforms, but it's unclear to me what unix platforms
support ftruncate and also what flags/include-files are necessary to make it
visible.  linux required a "#define _BSD_SOURCE 1" line.

mkaply: what is needed for OS/2?

Comment 7

[Mike Kaply [:mkaply]]

#elif defined(XP_OS2)
    if (DosSetFileSize((HFILE) PR_FileDesc2NativeHandle(mFD), offset) 
!= NO_ERROR) {
        NS_ERROR("DosSetFileSize failed");
        return NS_ERROR_FAILURE;
    }
#endif

I think the #includes are already there for OS/2 from including 
nsILocalFile.h. I'll try building this today.

Comment 8

[Darin Fisher]

Attachment: v1.1 linux, win32, macos, os/2 platforms covered

Comment 9

[Doug Turner (:dougt)]

mike, beard, can you both review?

Comment 10

[Doug Turner (:dougt)]

simon, can you review this patch

Comment 11

[Simon Fraser [no longer active]]

This won't work on Mac:
+#include "private/pprio.h"
If you want to #include "pprio.h", you'll have to add a MANIFEST in nsprpub/pr//
include/private. The comment in that header also says, about 
PR_FileDesc2NativeHandle:

" This function will be obsoleted with the next release; avoid using it."

Why can't we get the NSPR guys to add a PR_SetEOF to the API?

Comment 12

[Wan-Teh Chang]

The RFE for PR_SetEOF is bug 88341.

We can only add new functions in a minor release.
The next NSPR minor release is NSPR 4.2.  Mozilla
client is still using NSPR 4.0.2.  So the PR_SetEOF
RFE depends on the landing of NSPR 4.2 Beta (bug 78471).

Comment 13

[Doug Turner (:dougt)]

what wtc said, and this won't happen until after 0.9.3 is out the door.

Comment 14

[Simon Fraser [no longer active]]

So I need to see a patch that builds on Mac.

Comment 15

[gordon]

I'll take this.

Comment 16

[Gagan]

pdt+ per discussion with selmer, chofmann, clayton.

Comment 17

[selmer (gone)]

Did this make it into the 7/18 build?

Comment 18

[gordon]

Attachment: updated patch that builds on Mac.

Comment 19

[gordon]

Seems to work ok on Mac.  Gagan, Doug, can you try the latest patch on
Windows/Linux branch builds and post the results?

I'll r=gordon for Darin's code.  My changes we're simply a conditional include
of pprio.h since was already being exported, but had a different path on the
Mac, and adding a '::' to disambiguate nsFileStream::SetEOF() from the MacOS call.

Doug, we still need an sr=.

Comment 20

[gordon]

I don't think this is ready to check in tonight.  The patch covers linux, but no
other UNIX platforms.  I'm having difficulty reaching people now.  We need
another day to check coverage on other platforms.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I built attachment 42794 [details] [diff] [review] on a current (an hour or two old) Windows branch debug
build and tested it for a few minutes.  I verified that the cache bug that the
change was supposed to fix is indeed fixed.

Comment 22

[selmer (gone)]

It's possible that missing the build at 3:00 means missing the release.

Comment 23

[Gagan]

Confirmed that the patch works fine on Windows and Linux. I am making the call 
that this should go into the branch asap (and can have a sr past the checkins 
and if things are wrong we can fix it or back it out then-- but I really want 
this to not miss the daily spins) So I am going to check this in on gordon's 
behalf...  

Comment 24

[Gagan]

Fix checked into the branch...

Comment 25

[Doug Turner (:dougt)]

nsFileStream::SetEOF should assert if it not implemented on a given platform. 
The comment is not really good enough.  

The implemention in nsSocketIS and nsSocketOS of SetEOF should not return
NS_ERROR_NOT_IMPLEMENTED, but NS_ERROR_FAILURE.  It doesn't make sense to set
EOF on a socket stream.

Comment 26

[selmer (gone)]

benc or bbaetz: pls verify fix on branch. thanks. (from lchiang)

Comment 27

[Mike Kaply [:mkaply]]

Attachment: One more piece of patch for OS/2

Comment 28

[Tom Everingham]

verified on the branch, needs tested on the trunk for each platform once checked 
in

WinNT4 20010719 br
Mac os9 20010719 br
Linux rh6 20010719 br

to test: 

edit a page on a web server - delete a few lines and save
hit reload
view page source to see if lines are showing up deleted

Comment 29

[basic]

*** Bug 92112 has been marked as a duplicate of this bug. ***

Comment 30

[gordon]

I just landed the 7/18/2001 patch on the trunk.

Comment 31

[gordon]

Moving target milestone to mozilla0.9.4 to track fixing this for various Unix
platforms.  We probably need to land a new version of NSPR first.

Comment 32

[Brendan Eich [:brendan]]

I'm hacking FastLoad stuff, being a C++ and COM luddite, so don't get me wrong:
I am not requiring a distinct interface for setEOF.  Just a couple of questions,
though -- mainly I'm looking for a record of your thoughts on the tradeoffs
(sorry if I missed a newsgroup post):

1.  Why put setEOF in nsISeekableStream?  Wouldn't it make more sense in an
interface where there is known to be a file whose size should be set?  I may
have a seekable stream that's not backed by a file.

2.  Isn't this truncate on Unix, or setsize in general (truncate can extend a
file on modern Unixes)?  If so, we might rather have a single method to map to
the atomic system call, instead of this racy seek-then-tell&setEOF.  IOW, if I
can pass an offset into the method, why should I have to seek or tell?  I don't
even need to have a stream open to truncate (rather than ftruncate), so why
should I have to have an nsISeekableStream?

BTW, the FASTLOAD branch moves nsISeekableStream into its own .idl file over in
xpcom/io -- stream seekability is primitive and not confined to necko (unless
you want to move all the xpcom/io interfaces into necko, but then we shall go to
war... ;-).

/be

3.  

Comment 33

[Darin Fisher]

brendan:

on windows, you have to seek before you set eof, while on unix you can just set
eof at any arbitrary offset.  so, it seemed like a matter of choosing which
platform to optimize for until i realized that we need to call SetEOF right after
calling Seek anyways (in nsFileTransport.cpp).  also, nsIRandomAccessStore (which
nsISeekableStream basically duplicates) has a SetEOF method.

Comment 34

[Chris Hiner]

*** Bug 98164 has been marked as a duplicate of this bug. ***

Comment 35

[lidl]

How about the following diff to extend this patch to all the XP_UNIX
machines?  I cannot imagine a machine that claims to be XP_UNIX
and doesn't have ftruncate().

For what it's worth, with this patch applied, my BSD/OS builds now
no longer have this problem.

Comment 36

[lidl]

Attachment: patch to extend the linux patch across all XP_UNIX machines

Comment 37

[Jaime Rodriguez, Jr.]

Is this fixed? Can we resolve it and get it off the radar?

Comment 38

[Brendan Eich [:brendan]]

This is not fixed, obviously (from the outstanding patch).  If non-Linux Unix
platforms don't need to be supported on the branch, then remove the keywords,
but do not resolve this bug.

gordon, can you review the latest patch?

/be

Comment 39

[gordon]

No Brendan, I can't review it.  I could have written the original code that way,
but I am not familiar enough with all the various unix flavors to know what
issues we would run into. dbaron described the problem as tricky.

I would welcome reviews/verification of the patch from people more knowledgable
on the other unix platforms.

Comment 40

[lidl]

Far be it for me to submit supporting evidence for my own minor
extension patch to this, but I will give the following details:

ftruncate was in SVR4.  So, all SVR4 derived things have it.
(Solaris, among others.  SunOS 4.1.3 definately had it, but
you needed to #include <sys/types.h>, not <unistd.h>.)

ftruncate, in the BSD world, showed up in the NET/2 release.
So, all the BSD4.4 derived varients have it (BSD/OS, FreeBSD,
NetBSD, OpenBSD).

Linux, of course (looking at the original patch) has this.

I found a HP/UX 9.0.5 manual page that claims it has ftruncate.
I can't imagine them having ditched the system call since then.

I found an AIX 4.3 manual page resource online, that claims
that AIX 4.3 has ftruncate. I assume this is a relatively up-to-date
version of AIX.

Looking at an online IRIX 6.5 manual page, they claim support
for ftruncate.

I found an online reference for OSF1 (v4) that claims support
for ftruncate.  It also notes that ftruncate is XPG4-UNIX standard.

Since XPG4 is the  X/Open CAE specifications, Issue 4, July 1992
you're only relying on a standard of the last 9 years.

I cannot find a BeOS manual page cgi site, so I cannot check on that
particular OS.  Other than that, I think I've account for everything
in either the Tinderbox or the Ports-Tinderbox.

How about just checking the my extension to the patch into at least
the post 0.9.4 tip-o-the-tree and seeing if BeOS breaks?

Comment 41

[Darin Fisher]

the problem is not whether or not platforms support ftruncate, but rather how to
declare it properly.  on linux with the way the mozilla build system works, just
including unistd.h is not enough... we had to declare BSD_SOURCE first.  my
concern is then: do other unix platforms require other special defines?  perhaps
we won't know until we just slam the tree with your patch, but it sure would be
nice if there were a way to test this before possibly breaking the ports page.

Comment 42

[timeless]

beos5headers are easy...
wget ftp://ftp.be.com/pub/beos/BeOS5-DevTools.zip
unzip BeOS5-DevTools.zip DevTools.zip
unzip DevTools.zip "develop/headers/*"
grep ftruncate `find .`
./posix/unistd.h:extern int      ftruncate(int fd, off_t newsize);

Comment 43

[Darin Fisher]

timeless: but that doesn't answer my question.  is there any special define
required to cause #include <unistd.h> to actually declare ftruncate.  under
linux, there is... so i suspect under other platforms there would also be.

Comment 44

[Gagan]

considering the fact that this works fine for our three primary platforms, I am
taking off the nsbranch+ nomination. Clearing out older PDT+ stuff as well...

Comment 45

[Darin Fisher]

gagan: what about supporting solaris?

Comment 46

[Gagan]

let someone who can build on solaris confirm if this works ok... (like other
platforms)

Comment 47

[lidl]

Has anybody been able to test this on Solaris?  I think that is
the only other tinderbox of the "supported platforms" that needs
to have this tested on.  Of course, there is the "ports page"
as well, but I think there is a pretty good chance that it will
just work there too.

Anybody, anybody?

Please...

Comment 48

[Brendan Eich [:brendan]]

What are we waiting for?  Let's get this in for 0.9.6, please.  Test it via
tinderbox burning if necessary -- we need to make progress somehow.

/be

Comment 49

[gordon]

Okay, I'll watch for tinderbox burning...

Comment 50

[Darin Fisher]

Comment on attachment 48233 [details] [diff] [review]
patch to extend the linux patch across all XP_UNIX machines

sr=darin.. i agree.. a bit of time on tinderbox and we'll have a good patch :)

Comment 51

[gordon]

patch checked in.

Comment 52

[Christopher Hoess (gone)]

*** Bug 99603 has been marked as a duplicate of this bug. ***

Comment 53

[R.K.Aa.]

*** Bug 112074 has been marked as a duplicate of this bug. ***