32942 - [RFE] Impliment UNIX nsLocalFile::GetDiskSpaceAvailabl

Status: VERIFIED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Jason Eager]

Currently, we have:

NS_IMETHODIMP
nsLocalFile::GetDiskSpaceAvailable(PRInt64 *aDiskSpaceAvailable)
{
    NS_ENSURE_ARG_POINTER(aDiskSpaceAvailable);
    return NS_ERROR_NOT_IMPLEMENTED;
}


That would tend to explain our crappy error handling around
out of disk space conditions. :P

Is this something that no one ever got around to doing? (as in,
could I just do an obvious implimentation?)

Or is this something that is sadistically difficult to do in
a cross-platform manner?

Comment 1

[Suresh Duddi (gone)]

To Mr. File.

Comment 2

[Doug Turner (:dougt)]

Chris, can you assist?  I believe the code in nsFileSpec just needs to be 
massaged into nsLocalFile.

Comment 3

[Jason Eager]

Attachment: The first draft of my patch to implement this.

Comment 4

[Jason Eager]

Please note my opinion comment at the beginning of the function that this
function should have two parameters, the directory(->Filesystem) to determine
free disk space of should be one of the parameters. Making this change would
simplify this procedure dramatically.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

nsLocalFile represents a path, so it shouldn't be too hard.  mPath holds the
path in question, so you probably just want to use that.

Also, there are some strange indentation things here; the convention in that
file is 4-char indent, and tabs are verboten (as in the rest of the Mozilla
tree).

Comment 6

[Jason Eager]

Attachment: A much better patch to implement this.

Comment 7

[Christopher Blizzard (:blizzard)]

Adding dougt

Comment 8

[Christopher Blizzard (:blizzard)]

Attachment: cleaned up patch

Comment 9

[Christopher Blizzard (:blizzard)]

Thanks so much for doing the work, Jason!  I've cleaned up the patch a little
bit and it seems to work well.  Here's the script that I'm using to test it:

const nsILocalFile = Components.interfaces.nsILocalFile;
const nsILocalFile_PROGID = "component://mozilla/file/local";

var file = Components.classes[nsILocalFile_PROGID].createInstance(nsILocalFile);
var file2 =
Components.classes[nsILocalFile_PROGID].createInstance(nsILocalFile);

file.initWithPath("/tmp/6706.patch");
dump("free space is " + file.diskSpaceAvailable + "\n");

file2.initWithPath("/boot/boot.b");
dump("free space is " + file2.diskSpaceAvailable + "\n");

dougt, can you review and if you don't have any comments approve checking it in
please?  Thanks!

Comment 10

[Doug Turner (:dougt)]

Why are you initializing bytesFree to all the space in the world?  Should this 
be zero instead?

Comment 11

[Christopher Blizzard (:blizzard)]

Good question.  I'll set it to zero.  Any other comments?

Comment 12

[Doug Turner (:dougt)]

nope.  check it in.  :-)

Comment 13

[Jason Eager]

NO NO, DON'T set free space to zero..that hides bugs!

Since we no longer need to worry about passing a dummy value back
that works when this function fails, lets initalize bytesFree to
something like 0xCCCC, something where it becomes obvious that
bytesFree wasn't initalized after it was declared.

Comment 14

[Jason Eager]

It looks like the patch was already checked in...in fact, I got:

"Conflicts found in mozilla/xpcom/io/nsLocalFileUnix.cpp"
and the conflict marks appear to be in my code, so I assume
you checked in my patch.

Should we set this to "FIXED" now that the patch has been checked in?

Anyways, I'm off to fix bug# 32443.

Comment 15

[Doug Turner (:dougt)]

[e-mail], how would setting the space to zero hide bugs?  

Comment 16

[Christopher Blizzard (:blizzard)]

Yeah, initializing to zero is the right way to do this in my opinion.  If
there's a problem then people will definitely know because it will always
showing up as out of disk space.  Your way, there might be a chance that a size
will be returned when there isn't actually any space, causing the program using
it to try and write to disk when there isn't any space.  If there is an error,
and the program does something wrong that it end up doing the safe thing.

Oh, yes.  It's checked in.

Comment 17

[Christopher Blizzard (:blizzard)]

Please ignore the spam.  Changing address.

Comment 18

[Christopher Blizzard (:blizzard)]

busted from my reassign

Comment 19

[Christopher Blizzard (:blizzard)]

clearing status whiteboard while I'm at it

Comment 20

[Tracy Walker [:tracy]]

- Per last comments, age of bug, and no reopen - Marking Verified/Fixed.  Please 
reopen if still a problem.