Closed Bug 72892 Opened 19 years ago Closed 11 years ago

nsLocalFile::GetDiskSpaceAvailable ignores quotas for unix

Categories

(Core :: XPCOM, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: bbaetz, Assigned: stransky)

References

()

Details

(Keywords: dataloss)

Attachments

(1 file, 2 obsolete files)

This is split off from bug 55814, which deals with mail lossage when the disk is
full.

nsLocalFile::GetDiskSpaceAvailable does not take quotas into account, so the
checks before downloading mail which are done by mail don't work properly.
*** Bug 98076 has been marked as a duplicate of this bug. ***
taking some of the bugs that I have been working on from scc.
Assignee: scc → dougt
thought i had a patch for this, but I don't.
Keywords: helpwanted
Target Milestone: --- → Future
QA Contact: kandrot → nobody
Assignee: dougt → nobody
QA Contact: nobody → xpcom
Marking as dataloss, as if I understand it correctly, the client can download mail which it has no way to store - depending on the type of server and the clientside and serverside configuration, this could lose mail.
Keywords: dataloss
Attached patch trunk patch (obsolete) — Splinter Review
Comment on attachment 376242 [details] [diff] [review]
trunk patch

Can you please review this one?
Attachment #376242 - Flags: review?(benjamin)
Comment on attachment 376242 [details] [diff] [review]
trunk patch

>diff -up src/configure.in.old src/configure.in
>--- src/configure.in.old	2009-05-07 14:55:16.000000000 +0200
>+++ src/configure.in	2009-05-07 14:58:32.000000000 +0200
>@@ -3096,6 +3096,9 @@ AC_CHECK_HEADERS(io.h)
> dnl These are all the places some variant of statfs can be hiding.
> AC_CHECK_HEADERS(sys/statvfs.h sys/statfs.h sys/vfs.h sys/mount.h)
> 
>+dnl Quota support
>+AC_CHECK_HEADERS(sys/quota.h)

What systems might *not* have these functions? Does Mozilla need to worry about binary compatibility with older systems or kernels when introducing this test?

>diff -up src/xpcom/io/nsLocalFileUnix.cpp.old src/xpcom/io/nsLocalFileUnix.cpp

>+#if defined(HAVE_SYS_QUOTA_H)
>+    #include <sys/sysmacros.h>
>+    #pragma GCC visibility push(default)
>+    #include <sys/quota.h>
>+    #pragma GCC visibility pop

Instead of putting the visibility macros here, please add sys/quota.h (and sys/sysmacros.h?) to config/system-headers, which automatically wraps them for default visibility.

Also, please don't indent the #includes


>+static nsresult
>+GetDeviceName(nsACString &deviceName, int deviceMajor, int deviceMinor)

* Please order paramaters so that inparams are first and outparams are last: (int deviceMajor, int deviceMinor, nsACString &deviceName);
* No need for nsresult failure codes here, just use PRBool true==success false==failure
* This function should probably have a short doccomment explaining failure conditions


>+{
>+    nsresult ret = NS_ERROR_FAILURE;
>+    
>+    #define MOUNTINFO_LINE_LENGHT   200
>+    #define MOUNTINFO_DEV_POSITION  6

* Plese make these static const int instead of defines (kMountInfoLineLength)
* LENGHT is misspelled LENGTH?

>+    while(fgets(mountinfo_line,MOUNTINFO_LINE_LENGHT,f)) {
>+        char *p_dev = strstr(mountinfo_line,device_num);

I could use a comment before this loop explaining the file format and what is being looked-for.

> NS_IMETHODIMP
> nsLocalFile::GetDiskSpaceAvailable(PRInt64 *aDiskSpaceAvailable)
> {
>@@ -1167,6 +1216,30 @@ nsLocalFile::GetDiskSpaceAvailable(PRInt
>      */
>     *aDiskSpaceAvailable = (PRInt64)fs_buf.f_bsize * (fs_buf.f_bavail - 1);
> 
>+#if defined(HAVE_SYS_STAT_H) || defined(HAVE_SYS_QUOTA_H)
>+
>+    struct stat stat_buf;
>+    if(stat(mPath.get(), &stat_buf) < 0) {

Use FillStatCache/mCachedStat.

>+        PRInt64 QuotaSpaceAvailable =  (PRInt64)fs_buf.f_bsize * dq.dqb_bhardlimit;

Use a constructor-style cast: PRInt64(fs_buf.f_bsize * ...)

Overall, pretty good, just needs a little tightening.
Attachment #376242 - Flags: review?(benjamin) → review-
Attached patch v2 (obsolete) — Splinter Review
Thanks for the review, there's an updated one.
Attachment #376242 - Attachment is obsolete: true
Attachment #376718 - Flags: review?(benjamin)
Comment on attachment 376718 [details] [diff] [review]
v2

I need the answer to the question about sys/quota.h and the binary availability of quotactl. A couple people on IRC say that the BSDs don't have that header. Since nsLocalFileUnix.cpp is shared code, we'd at least need the configure checks from the first patch.

Is the format of /proc/self/mountinfo stable?
Attachment #376718 - Flags: review?(benjamin) → review-
sys/quota.h is exported by glibc so I presume it safe to expect appropriate kernel support when the header is there. So I vote for the original AC_CHECK_HEADERS(sys/quota.h) in configure file.

/proc/self/mountinfo should be stable since 2.6.26 kernels, see
http://www.mjmwired.net/kernel/Documentation/filesystems/proc.txt
Attached patch v2+quota.h checkSplinter Review
There's the v2 patch + quota.h check so it should be portable enough.
Attachment #376718 - Attachment is obsolete: true
Attachment #379547 - Flags: review?(benjamin)
Assignee: nobody → stransky
Attachment #379547 - Flags: review?(benjamin) → review+
Comment on attachment 379547 [details] [diff] [review]
v2+quota.h check

This needs a tryserver run before pushed to -central.
Priority: -- → P2
Target Milestone: Future → mozilla1.9.3
http://hg.mozilla.org/mozilla-central/rev/85bf327a368d
and followup http://hg.mozilla.org/mozilla-central/rev/6bf2a4703c9a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
This broke Darwin.  First it fails w/ sys/sysmacros.h not being found and I'm able to get around that by patching configure.in to look for it and #ifdef'ing it in nsLocalFileUnix.cpp, but then the build further fails with:

g++-4.2 -arch i386 -o nsLocalFileUnix.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -DOSTYPE=\"Darwin\" -DOSARCH=Darwin -D_IMPL_NS_COM -I.. -I/src/mozilla-central/xpcom/io -I. -I../../dist/include -I../../dist/include/nsprpub  -I/src/mozilla-central/obj-i386-apple-darwin9.8.0-browser/dist/include/nspr -I/src/mozilla-central/obj-i386-apple-darwin9.8.0-browser/dist/include/nss -I/sw/include      -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -isysroot /Developer/SDKs/MacOSX10.5.sdk -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread  -DNDEBUG -DTRIMMED -O3   -DMOZILLA_CLIENT -include ../../mozilla-config.h -Wp,-MD,.deps/nsLocalFileUnix.pp /src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp
/src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp: In member function ‘virtual nsresult nsLocalFile::GetDiskSpaceAvailable(PRInt64*)’:
/src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp:1238: error: invalid conversion from ‘int’ to ‘const char*’
/src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp:1238: error:   initializing argument 1 of ‘int quotactl(const char*, int, int, char*)’
/src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp:1238: error: invalid conversion from ‘const char*’ to ‘int’
/src/mozilla-central/xpcom/io/nsLocalFileUnix.cpp:1238: error:   initializing argument 2 of ‘int quotactl(const char*, int, int, char*)’
make[1]: *** [nsLocalFileUnix.o] Error 1
make[1]: Leaving directory `/src/mozilla-central/obj-i386-apple-darwin9.8.0-browser/xpcom/io'


I've filed Bug 520098 to deal with this, which has a possible solution, but I don't know the ramifications.
Is this bug really still 'helpwanted'?
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1
Helpwanted is the Bug 520098 - build this patch on Darwin.
Keywords: helpwanted
You need to log in before you can comment on or make changes to this bug.