embedders, consumers of nsIUploadChannel probably need a way to rewind streams. personally, i'm happy with the interface as is. we'd need to beef it out with some documentation. specifically, it should be made clear that |setEOF| may be unimplemented or otherwise generate an error (e.g., can only truncate a writable stream).

/* * Sets the stream pointer to the size of the stream plus the value * of the offset parameter. */ Should that say "minus"? I'm not sure why one would want to position the pointer past the end....

Er, nevermind. That's obviously supposed to take a negative offset.... still a little confusing, but...

Should this interface use some kind of typedef for the file offsets rather than fixing it at PRUint32. Just thinking ahead when we have gigabit internet throughput and people are browsing multi-gigabyte HTML pages ;-) Realistically though I've started seeing downloads that are greater than 1gig. So this may not be that far off.

void seek(in long whence, in long offset); PRUint32 tell(); Should the return value type of tell maybe be the same as the type of the offset argument of seek?

Oh yeah, and fwiw I agree with dbradley... Can we make this simply use a 64 bit type? NSPR has functions for such file accesses, at least Linux supports such files, and files are getting larger and larger, so I don't think we should freeze the 32 bits into an interface. Also note that nsIFile uses: 212 attribute PRInt64 fileSize; http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIFile.idl#212 (I wonder why it's signed...)

the reset of necko is 32 bit. we don't directly support long long buffers/offsets. David - the whence bits are a direct mapping of NSPR constants. NSPR hasn't added any in years, why would we? I will fix up the PRInt32 --> long thing.

>David - the whence bits are a direct mapping of NSPR constants. NSPR hasn't >added any in years, why would we? Not sure what you mean here. I was only talking about the size of the "offset" and how 2gigs seemed like a rather small limit for this interface.

david, ignore that part of the comment. Basically my point was that the 32bit offset is consistent with the rest of Necko APIs.

see: nsIStreamListener::OnDataAvailable(..., PRUint32 offset, PRUint32 count) nsIInputStream::Available(PRUint32 *result) nsIChannel::GetContentLength(PRInt32 *result) these are all frozen interfaces. we'd also need to rev these interfaces if we cared about supporting >2G files. should support for >2G files really be a requirement right now?

IMO we should serious consider support of downloading files >2gigs, but I don't know if these interfaces are involved or not. While even with broadband this can take a while, I'm sure we'll see people swaping large files, game demos, mpegs, etc.

this would be a pretty big overhaul of necko. I just created the tracking bug 184452.

Damn, what the hell was I thinking. I apologize for being a dumbass – too preoccupied with the rumor mill.

posted: news://news.mozilla.org:119/3E5301A7.2010000@netscape.com

i'm ok with freezing this interface. i do have a couple nits though... - NS_SEEK_SET ... it's unfortunate that we have to have this NS_ prefix. any clever ideas of how we could eliminate that? - |const PRInt32| should probably be |const long| to match the type of |whence|. otherwise, i think this interface is a good one to freeze. (i'm also a little concerned about the fact that some of our implementations only implement Tell. this is one of those interfaces where a reference to the interface is not really an indication of what functionality is really available. maybe that's an implementation bug.)

> any clever ideas of how we could eliminate that? Why not just rename the constants? We'll have to change some implementations, but....

but to what? SEEK_SET is #define'd by most operating systems. suppose the OS values are not compatible with the NSPR values, which these are based on? so, any suggestions?

Ah. Didn't realize we had a name collision... No bright ideas offhand (GK_SEEK_SET is just as bad as NS_SEEK_SET).

We could just use the OS values if they exist, couldn't we? People shouldn't be hardcoding constants, and since c+ modules would have to be recompiled on a new os, theres no compat issue, is there? Although that may be expsing more of the internals than we want to.

bbaetz: you're trusting that the NSPR constants map directly to the OS constants. i don't know if that is a safe thing to assume. cc'ing wtc for advice. we could also just #undef SEEK_SET, etc. above the interface decl. that would only cause minor pain for those who need to #include both our header file and the stdc header file(s). this might be worth the hassle.

Well, thats what I meant about perhaps exposing the interface too much. This is just a wrapper arround fseek though, and so the constants' meanings are fairly standard. Then you'd only have an issue where an os defined SEEK_SET and made it mean something else But yeah, it probably isn't a good idea to reuse those constants. We could #undef them first, I suppose, but then people couldn't use themin teh same file, and so on. |static const int|, perhaps?

You can't assume that the NSPR constants map directly to the OS constants.

Why the setEOF()? I mean the rest of the interface (and the name) implies read-only access to the stream.. would there be any difference between calling close() on the actual stream? It wouldn't surprise me if there was.. I just find it odd to see it in this interface, it seems like it belongs somewhere else.

alec: SetEOF means truncate at the current offset. This follows the windows convention of seek then set eof to truncate at a location in the stream. SetEOF is not at all equivalent to Close.

I think the issue is, that nsISeekableStream denotes read only access to the file. So finding a method that would actually modify the file seems a little out of place. I could have sworn I saw this same discussion somewhere, but wasn't able to locate it.

well, i guess we can move it to a different interface. maybe one that inherits from nsISeekableStream? the issue is that we don't want to make it so awkward to seek and truncate.

however, i just realized (and confirmed via LXR) that after my changes for bug 176919, nsISeekableStream::setEOF is never called. (NOTE: nsFileTransport.cpp is the only caller, but that code is dead and needs to be CVS removed.) so, perhaps the solution is simple. perhaps we might want to just eliminate SetEOF. now, it is of course a useful function in many cases, but if we aren't using it now, then maybe there is not good reason to freeze an interface which provides it.

This is the start of one discussion on setEOF http://bugzilla.mozilla.org/show_bug.cgi?id=86474#c32 I don't think it's the one I was thinking of, though.

darin agreed to own xpcom.io.

seek and truncate seems fine, I just know there are lots of consumers and implementors of input streams where you don't want to be modifying the underlying stream.. i.e. whereas seeking and reading are commonly paired and require two distinct interfaces, seeking and truncating is (I would assume) used much less often so I see no reason to make a seeking interface overly complex to make that particular pairing..

mailnews needs at least the ability to use the full 32 bits of file size, so we can have files up to 4GB, instead of being limited to 2GB. (Seek taking a signed instead of an unsigned is the issue there). Having 64 bit files in mailnews would be a whole new chunk of work...one possibility is having a new SEEK type which treats the offset as an unsigned int, instead of treating negative numbers as a reverse seek, something like NS_SEEK_CUR_UNSIGNED. Obviously, this is ugly and non-standard, but on the plus side, it might allow us to go from 2GB->4GB pretty easily.

why not use PRInt64 for seek types? mailnews wouldn't need to switch entirely to 64 bit functions, it could just convert an unsigned long to a PRInt64 when it wants to call Seek..

well, hows about this. We have a seekForward() and seekBackwards() (or seek() and rewind()), each of which take a long long - then we can still support 4G. It also syntacically defines defines two semantically different actions. I doubt there is ever a time that someone is going to want to seek an arbitrary amount forward or backward without knowing if they want to go forward or backward in the first place. Furthermore, we could make a nsIRewindableStream that contains the rewind() call, so that streams could indicate this capability to rewind via QI response.

converting mailnews is not a big issue, since we're not using nsISeekableStream much, if at all. We'd like to get rid of nsFileSpec and use nsISeekableStream and nsFileStream, but we'd like to get around the 2GB limit. I was more worried about the uses of nsISeekableStream in the rest of the tree and converting those to a 64 bit interface.

Verbose naming and a "rewindable" interface just makes the API story ulgier. I vote for either adding a nsISekableStream64 or fixing up the nsISeekableStream to take 64 bit value. I prefer the former.

A new 64 bit interface would seem the easiest way to introduce it to the code base without affecting the existing code. If we're unsure of the impact of 64 bit ints in the existing interfac,e going that route would be most prudent. If "we're" comfortable with the impact it would be great to convert the existing interface, and open up the ability to handle large files/streams throughout the app.

Darin and I were talking about this a bit - it seems like the least intrusive fix in terms of the rest of the code would be to add seek64 and tell64 methods to nsISeekableStream, since it's not a frozen interface, and Darin wants to remove setEOF anyway. Adding a new nsISeekableStream64 interface would add a bit of vtable bloat and make the client code QI for the new interface. Converting nsISeekableStream to 64 bit methods is probably not prohibitively difficult. From lxr, it looks like there are around 15-20 different files that use nsISeekableStream::seek, and many of them are seeking to the beginning of the stream anyway. I can try either of these. I just need some sort of consensus and buyin...

if there are that few callers, I say just update the interface and them - I would hope the pattern would be pretty straight forward.. we've done much more extensive changes in the past :)

I'd probably be more concerned about tell than seek, since it's likely to have more of ripple effect. I only see a handle full of those, but they're doing things like storing the result out into data members, which would have to be changed, and anything that touches those. That still may not be a big deal given the small number of references to tell I saw on lxr, but I didn't walk more than the top level So there could be one instance that fans out to many things that in turn fan out into many more things.

This patch builds and runs with 64 bit seek and tell - it's crude, and for the most part, just treats the 64 bit result of tell as a 32 bit value. It also doesn't use our 64 bit int macros, which Darin says might still be used in embedding environments. I'm going to look at nsInt64 and see if I should be using that...but, this patch does allow mail files to be > 2GB. I think most of the users of nsISeekableStream will still end up just using the 32 bits, like fastLoad, which isn't going to need files > 2GB, and is storing offsets in the data file as 32 bit numbers anyway. And the StringStream class probably doesn't need to worry about strings > 2GB...Anyway, this is the work in progress.

Does anyone else have input about the 64bit math? Does nsInt64 do the right thing on platforms that don't support 64 bit ints directly? I'd like to see what progress I can make on driving this forward.

> Does nsInt64 do the right thing on platforms that don't support 64 bit ints directly? nsInt64 should work correctly on all platforms, even those that do not natively support 64-bit integer types.

this patch addresses Darin's comment about wrapping 64 bit operations with nsInt64. I've separated out the mailnews diffs into a different patch.

Mailnews part of patch. I had to stop using nsMsgKeySet for the new msg set because it doesn't like id's > 0x7fffffff. nsMsgKeyArray will take up more memory but it does simplify the code a bit. In normal situations, there won't be a lot of new messages, so I don't think it's a big deal.

Comment on attachment 143825 [details] [diff] [review] non-mail part of patch, using nsInt64 >Index: xpcom/io/nsISeekableStream.idl >+ void seek(in long whence, in long long offset); >+ unsigned long long tell(); hmm... i think it might make more sense for tell() to return a signed long long instead. it seems to me that you should be able to seek to any position indicated by tell. also, the lseek system call uses signed integers for both the current stream position and the new stream position. PR_Seek64 behaves just like lseek. if you make tell return signed long long then you avoid the casting from PRUint64 to PRInt64. that casting worried me anyways since it means we really don't support the full range of PRUint64. i think this change might simplify the patch considerably. nit: i noticed some off-by-one character indentation problems, especially in member variable declarations.

sure, I can do that - I was just following the existing behaviour of making tell return an unsigned value. I wasn't happy about the casts either...

(In reply to comment #46) > sure, I can do that - I was just following the existing behaviour of making tell > return an unsigned value. I wasn't happy about the casts either... yeah, i figured that was the case. i hadn't given this much thought until looking at your patch and seeing what a pain all those casts are.

this does remove all the casts - I can't say it makes the patch much simpler, but there aren't any more casts to PRInt64. I've also made nsStorageStream just do 32 bit streams - if we want to make that support larger streams, we can do that orthogonally...

Comment on attachment 144054 [details] [diff] [review] patch addressing Darin's comments - non-mail diffs only >Index: netwerk/base/src/nsInputStreamPump.cpp >+ nsInt64 offsetBefore64 = offsetBefore; >+ nsInt64 offsetAfter64 = offsetAfter; >+ if (offsetAfter64 > offsetBefore64) >+ mStreamOffset += (PRInt32) (offsetAfter64 - offsetBefore64); nit: maybe add an assertion that the difference fits in a PRInt32? not that that isn't going to always be the case ... ;-) >Index: xpcom/io/nsFastLoadFile.cpp >+ NS_ASSERTION(fileSize < 0xFFFFFFFF, "fileSize must fit in 32 bits"); >+ // todo - assert fileSize fits in 32 bits; maybe this "todo" is bogus now? also, do you need to use LL_ macros of nsInt64 inside this NS_ASSERTION? >+ PRInt64 footerOffset; >+ rv = seekable->Tell(&footerOffset); >+ >+ mHeader.mFooterOffset = (PRUint32) footerOffset; this cast needs to use LL_ macros? >+ PRInt64 fileSize; >+ rv = seekable->Tell(&fileSize); >+ mHeader.mFileSize = (PRUint32) fileSize; same here. >Index: xpcom/io/nsStringStream.cpp >+ NS_ASSERTION(offset < 0xFFFFFFFF, "string streams only support 32 bit offsets"); use LL_ macros to compare or use nsInt64? >Index: xpcom/obsolete/nsFileStream.h >+ PRInt64 result = -1; use LL_ macros? >Index: xpcom/obsolete/nsIFileStream.cpp >+ case NS_SEEK_SET: ; break; semicolon before break should not be needed. >+ nsInt64 zero = LL_Zero(); maybe just do: nsInt64 zero = 0; r=darin with these nits picked.

this addresses Darin's comments, assuming I haven't missed anything :-)

Comment on attachment 143826 [details] [diff] [review] handle local folders > 2GB and < 4GB (checked in) corresponding mailnews changes

Minor nit if you think of it before you check it in, the minus1 declarations could benefit from adding "const". There were a few other places (fileSize64,maxUint32, etc.) where, I assume for conversion reasons, you assign and never change the value, those could use const as well. Most smart compilers would probably figure it out.

One more thing that needs to change: The IID of nsISeekableStream must be changed. See bug 239038, and a comment I posted to super-reviewers@mozilla.org

I went ahead and bumped the interface IID: Checking in nsISeekableStream.idl; /cvsroot/mozilla/xpcom/io/nsISeekableStream.idl,v <-- nsISeekableStream.idl new revision: 3.9; previous revision: 3.8 done It appears that the latest patch landed earlier today.

We no longer freeze interfaces.