Closed Bug 86474 Opened 23 years ago Closed 23 years ago

nsFileTransports won't truncate files

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: gordon, Assigned: gordon)

References

Details

Attachments

(6 files)

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.
Priority: -- → P3
Target Milestone: --- → mozilla0.9.2
Lets try these on the trunk and if they do well there we can try bringing them back in the 0.9.2 branch.
Keywords: nsBranch
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Priority: P3 → P2
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.
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".
Assignee: gordon → darin
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?
#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.
mike, beard, can you both review?
Assignee: darin → dougt
simon, can you review this patch
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?
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).
what wtc said, and this won't happen until after 0.9.3 is out the door.
So I need to see a patch that builds on Mac.
I'll take this.
Assignee: dougt → gordon
pdt+ per discussion with selmer, chofmann, clayton.
Whiteboard: PDT+
Did this make it into the 7/18 build?
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=.
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.
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.
It's possible that missing the build at 3:00 means missing the release.
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...
Fix checked into the branch...
Whiteboard: PDT+ → PDT+ (fix checked in on the branch)
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.
benc or bbaetz: pls verify fix on branch. thanks. (from lchiang)
Keywords: vbranch
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
*** Bug 92112 has been marked as a duplicate of this bug. ***
I just landed the 7/18/2001 patch on the trunk.
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.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
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.
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.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
*** Bug 98164 has been marked as a duplicate of this bug. ***
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.
Blocks: 99142
Is this fixed? Can we resolve it and get it off the radar?
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
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.
Keywords: nsbranchnsbranch+
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?
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.
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);
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.
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...
Keywords: nsbranch+, vbranch
Whiteboard: PDT+ (fix checked in on the branch)
gagan: what about supporting solaris?
let someone who can build on solaris confirm if this works ok... (like other platforms)
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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...
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
Okay, I'll watch for tinderbox burning...
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 :)
Attachment #48233 - Flags: superreview+
patch checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 99603 has been marked as a duplicate of this bug. ***
*** Bug 112074 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: