Closed Bug 76968 Opened 24 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.)
Attachment #87789 - Attachment is obsolete: true
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+
Thanks. Done.
Keywords: mozilla1.0.1+ → fixed1.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: