Closed
Bug 86474
Opened 23 years ago
Closed 23 years ago
nsFileTransports won't truncate files
Categories
(Core :: Networking, defect, P2)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla0.9.6
People
(Reporter: gordon, Assigned: gordon)
References
Details
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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.
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
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
Comment 4•23 years ago
|
||
Comment 5•23 years ago
|
||
Comment 6•23 years ago
|
||
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•23 years ago
|
||
#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•23 years ago
|
||
Comment 10•23 years ago
|
||
simon, can you review this patch
Comment 11•23 years ago
|
||
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•23 years ago
|
||
Comment 13•23 years ago
|
||
what wtc said, and this won't happen until after 0.9.3 is out the door.
Comment 14•23 years ago
|
||
So I need to see a patch that builds on Mac.
Comment 17•23 years ago
|
||
Did this make it into the 7/18 build?
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
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=.
Assignee | ||
Comment 20•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
It's possible that missing the build at 3:00 means missing the release.
Comment 23•23 years ago
|
||
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•23 years ago
|
||
Fix checked into the branch...
Comment 25•23 years ago
|
||
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•23 years ago
|
||
benc or bbaetz: pls verify fix on branch. thanks. (from lchiang)
Keywords: vbranch
Comment 27•23 years ago
|
||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
*** Bug 92112 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 30•23 years ago
|
||
I just landed the 7/18/2001 patch on the trunk.
Assignee | ||
Comment 31•23 years ago
|
||
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
Comment 32•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
*** Bug 98164 has been marked as a duplicate of this bug. ***
Comment 35•23 years ago
|
||
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•23 years ago
|
||
Comment 37•23 years ago
|
||
Is this fixed? Can we resolve it and get it off the radar?
Comment 38•23 years ago
|
||
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
Assignee | ||
Comment 39•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
gagan: what about supporting solaris?
Comment 46•23 years ago
|
||
let someone who can build on solaris confirm if this works ok... (like other
platforms)
Comment 47•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 49•23 years ago
|
||
Okay, I'll watch for tinderbox burning...
Comment 50•23 years ago
|
||
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+
Assignee | ||
Comment 51•23 years ago
|
||
patch checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 52•23 years ago
|
||
*** Bug 99603 has been marked as a duplicate of this bug. ***
Comment 53•23 years ago
|
||
*** 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.
Description
•