Closed Bug 76968 Opened 23 years ago Closed 22 years ago

Need to ~expand paths in nsLocalFileUnix

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: frb, Assigned: shaver)

References

Details

(Keywords: helpwanted)

Attachments

(1 file, 2 obsolete files)

Now with GetNativePath (thanks!)
1.17 KB, patch
bzbarsky
: review+
blizzard
: superreview+
jud
: approval+
Details | Diff | Splinter Review 
One cannot set browser.download.dir to "~/.downloads" or anything resembling it
in the prefs such as all.js

Mozilla/5.0 (X11; U; Linux 2.4.2-ac21 i686; en-US; 0.8.1) Gecko/20010420
It's long past time that nsLocalFileUnix supported "~/foo" to mean $HOME/foo. 
I've got a patch that does it. I 0WNZ J00!
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
I require a leading "~/" at the beginning, so simply calling
  myFile.initWithPath("~");
will not get you the home directory.  When I was writing the patch, that seemed
like the right behaviour, but now I'm not so sure.

(frb@ is using this version of the patch in his build, so we'll hear soon if it
causes problems.)

People who really want a relative path beginning with ~/ should use "./~/".  I
don't think anyone actually does, though.

check for a null on nsMemory::Alloc, and r/sr=dougt
Blizzard writes:
> What about when prefs are dumped back out to the filesystem?  The ~/foo is lost?

Yeah, I expect so.  I could round-trip it by testing the path with contains()
for the home dir, or I could just live with it.  (The round-tripping would cause
other paths to change from /home/shaver/dir/file.txt to ~/dir/file.txt, which is
also perhaps undesirable.)

What behaviour do we want here?  Bash expands the ~ before passing it to other
programs, and we expand it before passing it to other code.  I think it's fine.
Summary: relative directories are not supported in prefs → Need to ~expand paths in nsLocalFileUnix
Yeah, putting ~ back into the prefs file would make older versions of mozilla
that don't support this option not work and that's enough reason not to do it
right there.  So this would be useful for default prefs and not much else. 
Isn't that how Ximian is using this?
It's also useful for specifying file-save locations, I've found.

I think that Ximian's main use is for default prefs, though, yes.

I crave review so that I can land it in 0.9.1.
Keywords: patch, review
+    if (filePath[0] == '~' && filePath[1] == '/') {

Do we know for a fact that filePath is at least two bytes long?

+        strcpy(name, homePath);
+        strcat(name, filePath + 1);

Use nsCRT for great justice!

Fix that and you have an r/sr=blizzard.
+    if (filePath[0] == '~' && filePath[1] == '/') {

We know if filePath[0] == '~' that filePath has length >= 1, and that
filePath[1] must be valid to address (it's either a '\0' or another char,
perhaps a '/').  Thanks to NUL termination, one need not fuss with length.

/be
Linux glibc has the wordexp function that does shell expansions. I would
argue that mozilla should use system functions, where available, for
this kind of stuff rather than rolling its own. The system libraries
have presumable "correct" implementations.
> Use nsCRT for great justice!

I think you mean ``use nsCRT for great waste and noise''.  What's the value in
using nsCRT::strcpy or nsCRT::strcat, other than defeating compiler
optimizations and incurring additional function-call overhead?  Any platform
without a working strcat or strcpy is doomed from the start, and will have a
hard time calling itself ``Unix'' enough to be using nsLocalFileUnix!

I don't want to switch to nsCRT.  I think it's evil, and I don't want it in my
code.  There are cases, such as Unicode string manipulations and
case-insensitive string comparisons, where we need a portability layer, and,
though nsCRT is wildly overkill for it, I will use it in those situations.  This
is not one of those situations.

Blizzard: can you reconsider and r/sr without the changes?  brendan, can you do
the other?

I don't think I want to deal with the star-expansion and the like from wordexp,
but I'll look at it (quite a bit) later.  Also, I think that using the Mozilla
settings for home directory is actually the right thing, so that embedders get
the expected effect when they set it to a custom value.

+    if (filePath[0] == '~' && filePath[1] == '/') {

NB: filePath + 1 points to "/...".

+        // XXX support ~user/place?
+        nsCOMPtr<nsIFile> homeDir;
+        nsXPIDLCString homePath;
+        if (NS_FAILED(NS_GetSpecialDirectory(NS_OS_HOME_DIR,
getter_AddRefs(homeDir))) ||
+            NS_FAILED(homeDir->GetPath(getter_Copies(homePath))))
+            return NS_ERROR_FAILURE;

Style rule #37 (brace multi-line-in-toto if-then stmts)?

+        len = strlen(filePath) + strlen(homePath);

Why not order these left-to-right as they are in memory?  strlen(homePath) +
strlen(filePath)?  But wait, you must mean strlen(filePath+1)...

+        name = (char *) nsMemory::Alloc(len + 1);

...else you overallocate by 1 byte here, and (more important) len is off by 1.

+        strcpy(name, homePath);
+        strcat(name, filePath + 1);

Proof: these copy strlen(homePath) + strlen(filePath+1) or len-1 non-NUL bytes,
then blat a NUL after at name[len-2].

/be
If you expose something that looks like a tilde expandsion to the user
then it should work the way the user expects tilde expansions to work or
you leave mozilla open to ridicule as being to stupid to get even a
simple thing like tilde expansions right. That mean supporting ~/ as
well as ~user/, not to mention getting the interaction with $HOME right.

It's all a royal pain which is why I suggested letting the system do it.
Brendan: thanks.  I think I also want to make "~" work by itself, and I shall! 
New patch coming up, but not until after the 0.9.1 freeze (I'm travelling until
Thursday).  Drivers can take it on the branch or not.

Ten: I have trouble reconciling "such a simple thing" with "a royal pain"; which
is it?  If you'd like to produce a patch (including configure tests) for
wordexp, which does tilde-only expansions, then I will gratefully accept it.

I'm thinking this can wait.  we can move it back when shaver returns
if anyone thinks we really need it in 0.9.1
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Asa just told me that we shipped 0.9.2!  Who knew?
Target Milestone: mozilla0.9.2 → mozilla0.9.3
taking this bug, with shaver's persmission.

The most important use I see for ~-expansion is in default prefs.  That's what I
need it for, myself.  Adding 51246 as a dependency.

I'll update shaver's patch per brendan's latest comments.  My thoughts on the
various things said in this bug:

1)  ~user may not be reasonably available or defined at all times (eg it's an
    AFS mount that's only mounted at user foo's login by the system login
    scripts on a system using kerberos and thus not having users in
    /etc/password at all times).  I'm not sure we want to deal with ~user...

2)  I have a linux machine with glibc 2.1.3.  'man wordexp' tells me nothing.
    wordexp.h does exist, but is not exactly documented.... Makes it hard to
    use, no?  Furthermore, this is likely unavailable on other Unix OSes....

3)  At the same time, we could really use expansion of ~/foo for the obvious
    purpose: default values in unix.js.  Things like ~/.mailcap and so
forth.Given all that and given that Mozilla is not a shell and that we're
implementing this for our internal use, I would suggest sticking with shaver's
original approach.
Assignee: shaver → bzbarsky
Blocks: 51246
Status: ASSIGNED → NEW
Keywords: mozilla0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Internal use things have a way of creeping out into the world,
particularly in an open source project. I see no reason to believe that
users will not use these features once they exist. Once that happens
you must deal with the fact that the proposals here overload the "~/"
syntax with somewhat different semantics than what the users are
accustomed to. Mozilla may not be a shell (although there have been
proposals to make it one) but Unix users are exposed to shells all the
time and that's where they get their understanding of tilde expansion.
It's just bad ui design to possibly confuse, and potentially irritate,
users.

Now Unix vendors have never been very consistent so it is not at all
clear that tilde expansion is done the same way on all platforms. That's
why I argue that mozilla should use a system library call for this kind
of thing is a call is available. Obviously, mozilla would have to have
its own default version just in case. On a GNU system, the call is
wordexp.

Finally, glibc comes with a reasonably complete texinfo manual. Wordexp
is documented there, including examples. In particular, the tilde
expansion algorithm is defined. Bash also has a manual and an equivalent
algorithm is defined there.

BTW, if you're going to do this, wouldn't it also be a good idea to 
introduce something like "@/" to indicate the current profile dir?
> Once that happens you must deal with the fact that the proposals here overload
> the "~/" syntax with somewhat different semantics than what the users are
> accustomed to.

Really?  ~/filename means ${HOME}/filename in all shells I have tried.

~username is a totally different syntax.  We would explicitly only be expanding
~/.  So I'm not sure that statement you make is true.

Tying mozilla to a system call that does not exist on all Unix systems just
introduces more code paths.  Since ~/filename is expanded the same way on all
Unix platforms I'm aware of and since we need to roll our own for non-GNU
systems anyway, I see no good reason to use the GNU system call.

I feel that the complexity of having to read and parse /etc/passwd is not worth
it.  I would propose that the patch to make ~/ work be checked in and the bug be
left open for all the other fun (an not-as-likely-to-be-used) ~-expansions


> Really?  ~/filename means ${HOME}/filename in all shells I have tried.

You're designing for what you know now not for what may exist in the 
future. 

> ~username is a totally different syntax.  We would explicitly only be 
> expanding ~/.  So I'm not sure that statement you make is true.

Well, all the references I examined treat ~/ and ~user/ as part of the 
same algorithm. This is exactly one of my points: you're overloading 
your semantics on a previously existing syntax. I think it's a big 
mistake.

> I feel that the complexity of having to read and parse /etc/passwd is 
> not worth it.  I would propose that the patch to make ~/ work be 
> checked in and the bug be left open for all the other fun
> (an not-as-likely-to-be-used) ~-expansions

You're quite correct that reading /etc/passwd is a pain but you really 
have to if you're going to handle bogus or non-existent $HOME. Since 
this is also highly platform specific you have almost the same problems. 
That's one reason I propose using already existing library calls.

As for just doing part of the job, we all know it will never be finished 
then.
> You're quite correct that reading /etc/passwd is a pain but you really 
> have to if you're going to handle bogus or non-existent $HOME

Just tried tcsh, sh, and bash.  Their use /etc/password to correct a bogus value
of $HOME is nonexistent -- they all fail to 'cd ~'.  If $HOME is not set, sh
does not deal at all, the other two seem to read /etc/password.

Not exactly consistent behavior....

> That's one reason I propose using already existing library calls

Which don't exist on most systems, so we still have to write our own
implementation.... which we may as well use for everything after that.

By the way, there _are_ library calls to read a passwd file (getpwent(), eg). 
Too bad they're not reentrant on Linux (Solaris has getwpent_r(), so things
would be OK there).  So we have to roll our own parsing too if we decide to do
this ~user thing.

> As for just doing part of the job, we all know it will never be finished 
> then.

Sure it will.  Assuming anyone cares about it.  Want to do it? :)
Blocks: 95504
> Just tried tcsh, sh, and bash.  Their use /etc/password to correct a
> bogus value of $HOME is nonexistent -- they all fail to 'cd ~'.  If
> $HOME is not set, sh does not deal at all, the other two seem to read
> /etc/password.

> Not exactly consistent behavior....

You missed, zsh, ksh, ash, and probably others. Ah , the wonders of
Unix. :-)

> By the way, there _are_ library calls to read a passwd file
> (getpwent(), eg). Too bad they're not reentrant on Linux (Solaris has
> getwpent_r(), so things would be OK there).  So we have to roll our
> own parsing too if we decide to do this ~user thing.

Hmm..., mozilla tries to find the user's home dir at startup where 
reentrancy isn't an issue. I've always assumed you were going to hook 
into that code.

I looked at that code and it seems just a fragile as anything proposed
here. There's no reason to assume that HOME will be set in the
environment so mozilla would definitely need a reliable robust method of
finding the user's home dir and that, in turn, requires the ability to
examine /etc/passwd or its equivalent. Once that's done, the ~user/
thing becomes a trivial addition.

The problem, as always, is inconsistency among Unix-like systems. You 
can't assume getpwent or even that /etc/passwd has what you want and is 
readable, etc.
No longer blocks: 95504
Blocks: 95594
No longer blocks: 95594
Blocks: 95504
No longer blocks: 95504
Blocks: 95504
Man how much misinformation:

- the wordexp() function is available on all Unix systems.  It's defined in POSIX.2
  and has to be implemented for an implementation to get the Unix brand.

- getpwnam_r() etc of course exists on Linux.  But since wordexp() is clearly
preferred
  go with it, it does the right thing.

The entire libc is thread safe.  Believe me, I wrote it.

pushing out since there is no way to have this make 0.9.4

shaver, blizzard would it be OK to use wordexp?  That is, would a patch doing so
pass your sr?  If so, I'll try to whip one up.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
More investigating:

1) getpwnam_r seems to take different numbers of arguments on Linux and Solaris
   (tried taking some code and compiling on both, so this is not a theoretical
   consideration).
2) If we implement this, it should _not_ be done on VMS (per mail from
   colin@theblakes.com that I can attach here if people care).

What other platforms define XP_UNIX that are not actually unix?  OS/2?  BeOS?
> getpwnam_r seems to take different numbers of arguments on Linux and Solaris
> (tried taking some code and compiling on both, so this is not a theoretical
> consideration).

What version of Solaris did use looked at and which flags?  Solaris
unfortunately used an incompatible interface for all the _r functions before
they were standardized.  But to comply with the standard the new functions must
be there, too.  If you look at <pwd.h> you'll see that there is a function
getpwnam_r with the same interface glibc has.  Depending on what compiler you
use, either a #pragma trick or a local wrapper function around
__posix_getpwnam_r is used.  So, there shouldn't really be a problem.

Besides, what was to outcome of the proposal to use wordexp()?  Using this
function would hide to gory details.
From sysinfo:

OS Distribution is        Solaris 8 10/00 s28s_u2wos_11b SPARC

No flags.  Just straight 'gcc foo.c'

As for wordexp() (and probably getpwnam_r, for that matter), so far the story
is: At least one platform (VMS) that uses nsLocalFileUnix does _not_ have
wordexp (or /etc/passwd, or ~-expansion, for that matter).  So we can't use this
until we figure out what platforms _do_ have it and whether that set of
platforms is the same as the set of platforms we want ~-expansion on.
> No flags.  Just straight 'gcc foo.c'

This isn't guaranteed to work anywhere.  You should read up on feature select
macros (e.g., in the beginning of the glibc manual).  Try again with

  gcc foo.c -D_POSIX_C_SOURCE=199506L

or better in this case (as the man page tells you)

  gcc foo.c -D_POSIX_PTHREAD_SEMANTICS

> As for wordexp() (and probably getpwnam_r, for that matter), so far the story
> is: At least one platform (VMS) that uses nsLocalFileUnix does _not_ have
> wordexp (or /etc/passwd, or ~-expansion, for that matter).

You are thinking solely about XP_UNIX when distinguishing the platforms. 
Instead feature tests in configure should be used.  Test for wordexp() and
getpwnam_r() and use the functions, fall back on what exists in the moment. 
Yes, this means a bit of code is implemented in more than one way but this is
already the case today.  It also means that some platforms are (initially)
better supported but this is nothing new either.
beos is currently not multiuser and is kinda not likely to change much.  i've 
recently commented in a bug describing how to check for apis, i can check for 
this too if people care.

fwiw, some shells appear to convert ~envvar/ to ~(eval[envar])/ so you could 
probably implement that for things like BeOS...

QNX and OS/2 (in one variant) probably also follow the XP_UNIX flag
pushing out since there seems to be no clean way to do this and I really don't
have the time.  If someone wants this, please take.
Keywords: helpwanted
Target Milestone: mozilla0.9.5 → mozilla0.9.7
Pushing out some more.  I don't have access to enough different unix systems to
test the portability of the more ambitious solutions.  Life is making too many
time demands.  People seem to be unhappy with the solution that provides the
maximal utility to time expended ratio...

Hopefully preferences will be able to use resource: urls for things like
~/.mailcap and then this bug will simply become a non-issue.
Keywords: mozilla0.9.4, review
Target Milestone: mozilla0.9.7 → mozilla1.0
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
Priority: -- → P4
Will this patch fix bug 93141?
Keywords: mozilla1.0.1
Alright.  I see that I'll never have the time to implement a solution that would
make people happy.  I'll just stop wasting my time and that of the people who
obviously have better ideas than me of how to implement this and how it should
work.  Back to shaver (and clearing bogus target milestone).
Assignee: bzbarsky → shaver
Priority: P4 → --
Target Milestone: mozilla1.0.1 → ---
Suppose we require that HOME be set for mozilla-bin to run. That pushes 
the problem into whatever launches mozilla-bin but then you wouldn't 
need a mozilla expert to fix any problems. All mozilla-bin would have to 
do is decide whether to continue or not if HOME is missing.
Blocks: 142012
Blocks: 93141
Keywords: mozilla1.0.1 → mozilla1.0.1-
Of course, the trunk and branch have diverged here, and I only have a branch
tree around, but I'll remedy that shortly.  In the meantime, people can at
least check my string-math.

(I'm still amused that people actually talked about reading /etc/passwd, in the
year 2002, BTW.)
Attachment #31683 - Attachment is obsolete: true
So... what was the reason not to use homeDir->GetNativePath()? Then you can
avoid monkeying with convert_ucs2_to_native yourself.

Apart from that the patch looks fine...
The simple reason is that I was rushing for dinner, and didn't notice that API
feature.  Review wins again!

(New patch later, sleep now.)

Comment on attachment 87936 [details] [diff] [review]
Now with GetNativePath (thanks!)

This patch works on the trunk too, BTW.
Comment on attachment 87936 [details] [diff] [review]
Now with GetNativePath (thanks!)

r=bzbarsky; looks good, works great.
Attachment #87936 - Flags: review+
Comment on attachment 87936 [details] [diff] [review]
Now with GetNativePath (thanks!)

sr=blizzard
Attachment #87936 - Flags: superreview+
Fixed.  I'll might ask for branch permission when I get back on Thursday.

Or I might not!  You never know with crazy old shaver!
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Attachment #87936 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1- → mozilla1.0.1+
Blocks: 134143
No longer blocks: 134143
*** Bug 134143 has been marked as a duplicate of this bug. ***
Blocks: 241850
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: