169506 - nsLocalFileUnix have problems with BeOS. IsExecutable()...

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sergei Dolgov]

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.

Comment 1

[Doug Turner (:dougt)]

feel free to reasign to a beos owner.

Comment 2

[Sergei Dolgov]

setting dependency

Comment 3

[Sergei Dolgov]

Attachment: Patch for IsXRW group of methods

Using stat() instead access()

Comment 4

[Scott Collins]

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|.

Comment 5

[Scott Collins]

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

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 100758 [details] [diff] [review]
Patch for IsXRW group of methods

according to [e-mail], 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?

Comment 7

[Sergei Dolgov]

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

Comment 8

[Sergei Dolgov]

Attachment: Patch with comment

Same as previous, but has comment after #ifdef BEOS

Comment 9

[Paul]

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

Comment 10

[Doug Turner (:dougt)]

great.  over to you...

Comment 11

[Sergei Dolgov]

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)).

Comment 12

[Sergei Dolgov]

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

Comment 13

[Paul]

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

Comment 14

[Paul]

This has been checked in.

Comment 15

[tqh]

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

Comment 16

[tqh]

Attachment: Patch that complements previous patch and fix ENOENT

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

Comment 17

[Niels Reedijk]

Reassigning to tqh. 

I'm looking at it right now.

Comment 18

[Niels Reedijk]

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?

Comment 19

[tqh]

(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. 

Comment 20

[tqh]

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 21

[Niels Reedijk]

Attachment: Updated to HEAD - still the same patch by tqh

Comment 22

[Wan-Teh Chang]

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.

Comment 23

[tqh]

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.

Comment 24

[Niels Reedijk]

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

Comment 25

[tqh]

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

Comment 26

[tqh]

Attachment: Updated patch

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 27

[tqh]

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

I believe this one to be ok, r?

Comment 28

[Sergei Dolgov]

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

r=sergei_d

Hope no more buggy access() cases for BeOS

Comment 29

[tqh]

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

sr?

Comment 30

[Doug Turner (:dougt)]

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

looks fine.

Comment 31

[Niels Reedijk]

Checked in by timeless