For example, access() POSIX function returns always 0, inspite real status/mode. (used in IsExecutable, IsWritable, IsReadable) This is partly bugginess, partly specifics of BeOS POSIX layer implementation. For example, http://bugzilla.mozilla.org/show_bug.cgi?id=145860 depends on this bugginess. I think that right idea is to fork at last nsLocalFileBeOS from nsLocalFileUnix and starting step-by-step rewrite.

feel free to reasign to a beos owner.

setting dependency

Using stat() instead access()

Comment on attachment 100758 [details] [diff] [review] Patch for IsXRW group of methods sr=scc. I think this requires module owner review from dougt with an analysis (or at least an explanation) from dougt as to why |stat| instead of |access|.

or rather, why not |stat| instead of |access| universally, so that separate implementations aren't needed...

Comment on attachment 100758 [details] [diff] [review] Patch for IsXRW group of methods according to sergei_d@fi.tartu.ee, access is buggy on BeOS. r=dougt if you put a comment under the #ifdef XP_BEOS stating this. btw, do you require me to check this in?

"btw, do you require me to check this in?" Yes, i do, because i haven't permissions.

Same as previous, but has comment after #ifdef BEOS

I can check this in tonight, as I've been told that I am the port lead for BeOS and have CVS access.

great. over to you...

Comment on attachment 101730 [details] [diff] [review] Patch with comment Opps. Additionaly removed warning (sometimes even compile error) about assigning const char * Path() to char * resolved_path_ptr in XP_BEOS part of NS_IMETHODIMP nsLocalFile::Normalize()(get tired to fix such things every time manually - explicit casting is Good Thing (TM)).

Doug, does it need r= again? Scott allowed me to forward sr from previous patch.

Comment on attachment 101730 [details] [diff] [review] Patch with comment r=arougthopher sr=scc (as per sergei's comments) I will check this in as soon as the tree re-opens

This has been checked in.

Reopening, the patch commited does not take into account that ENOENT should not return an error.

Patch to fix ENOENT, also includes correct behaviour for Reveal, Exists is also moved away from the buggy access implementation.

Reassigning to tqh. I'm looking at it right now.

1. It doesn't apply cleanly on the MAIN branch (your name is rejected) Please note that the formatting of the header has changed (make sure you check it) 2. Your second hunk deletes a newline. Why? 3. In the third hunk you have two newlines between Exists() and IsWritable(). 4. Could you explain why ENOENT is a good condition? If you have a path that doesn't exist, how can you say it is writable? Remember, I'm just nagging. This is shared code and it should look as nice as possible. One small question though. Exactly what kind of bugs does this fix?

(In reply to comment #18) > 1. It doesn't apply cleanly on the MAIN branch (your name is rejected) > Please note that the formatting of the header has changed (make sure you > check it) > 2. Your second hunk deletes a newline. Why? > 3. In the third hunk you have two newlines between Exists() and IsWritable(). > 4. Could you explain why ENOENT is a good condition? If you have a path that > doesn't exist, how can you say it is writable? > > Remember, I'm just nagging. This is shared code and it should look as nice as > possible. One small question though. Exactly what kind of bugs does this fix? 1. Ah well I'm still on AVIARY unfortunatly so I have to get the new tree. 2. 3. Because I've done major work and checking and those slipped thru. 4. These functions are defined as 'do not fail if not existant'. Just return false. At least that's how they are used under Windows / Unix and the access documentation says the same so I assume that's correct.

It fixes downloading, Otherwise you will get a lot of JS-crashes in some JS file if you don't specify 'Ask me where to save'. Probably Downloads.js. It may not show without bug 276378 applied. Also it probably fixes a lot under the hood.

Comment on attachment 170048 [details] [diff] [review] Updated to HEAD - still the same patch by tqh >-#ifdef XP_BEOS >-// access() is buggy in BeOS POSIX implementation, at least for BFS, using stat() instead >+ > NS_IMETHODIMP > nsLocalFile::IsWritable(PRBool *_retval) > { > CHECK_mPath(); > NS_ENSURE_ARG_POINTER(_retval); > struct stat buf; > *_retval = (stat(mPath.get(), &buf) == 0); > *_retval = *_retval && (buf.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH )); >- if (*_retval || errno == EACCES) >+ if (*_retval || errno == EACCES || errno == ENOENT) 'errno' should be read only if a library function call has failed. If the stat() call succeeds (returns 0) but the file is not writable, the above code will set *_retval to 0, and proceed to read errno (for comparison with EACCES and ENOENT). In this case, the value of errno is garbage.

Ah, good point, I was going to say that we have the 'stat(..) == 0' in there but then on the next line we may set *_retval to false anyway. I'll fix that during the day.

tqh, could you fix this patch? I'd really like to include it (working) in a next build. Thanks.

Yes, in fact I thought I already had, but I see now that I never uploaded it here. Thanks for reminding me.

This should be the latest version. I am having some troubles with my computer, so not 100% sure. Also tried to improve formatting and added a reference to this bug so that we know why this was done.

Comment on attachment 171405 [details] [diff] [review] Updated patch I believe this one to be ok, r?

Comment on attachment 171405 [details] [diff] [review] Updated patch r=sergei_d Hope no more buggy access() cases for BeOS

Comment on attachment 171405 [details] [diff] [review] Updated patch sr?

Comment on attachment 171405 [details] [diff] [review] Updated patch looks fine.

Checked in by timeless