Hi Vojta, Thank you very much for you feedback. I have applied the changes you asked me to do and have attached the patch with the override.
On Thu, Apr 5, 2012 at 1:38 PM, Vojtech Horky <[email protected]>wrote: > Hi Ketan, > thanks for the revised patches. Please, see my comments below. > > Dne 4. dubna 2012 14:12 Ketan Singh <[email protected]> napsal(a): > > Hi, > > > > Thank you very much for your feedback Vojta. It helped a lot. > > > > On Tue, Apr 3, 2012 at 11:41 AM, Vojtech Horky <[email protected]> > > wrote: > >> First, leave the whole path to the file in the patch. That is because > >> filenames across directories are not unique and vfs.c is in libc as > >> well as in VFS server. Although it is pretty clear which vfs.c you > >> meant, it just makes things easier. Thanks. > > > > > > I have attached a few more patches with the mail that address these > > problems. Now the patches contain the entire path to the file. > Thanks. Just a hint - if you use Bazaar checkout, you can create the > patch by using bzr diff and send it all in one file. That is perfectly > okay. Plus it does not reveal your file system hierarchy ;-). > > Done! Thanks for the pointer. I have started learning bzr. :-) > > > >> > >> > >> Next, the way you test for '-' is not correct because you just test > >> the first character. That means that 'cd -whatever' has the same > >> meaning as 'cd -something-else' and 'cd -' which is something you > >> probably have not intended. > > > > > > Yeah, I forgot to check for null character after the '-'. Now that is > fixed > > with a null check. > Good. But why don't you use str_cmp(argv[1], "-")? > I am using that now. Perhaps, I went too rigorous on the conditions. > > > > >> > >> > >> When you start HelenOS and the first command you type is 'cd -', you > >> will get an error proclaiming that "Destination path is too long". > >> Such message is very misleading. I suspect that the cause is the > >> second hunk where you test pwd_path for NULL. Can you, please, clarify > >> what this code shall do? I probably missed something... > >> > > I fixed the problem by raising a cli_error EFAIL ( I could not find a > better > > error type. Maybe I could define some other error type to be more clear. > ) > > with the message "PWD Parameter not defined". I think this is more clear > > than my earlier attempt. > In my humble opinion, this message is still not very clear for end > user. Something in the line of "No previous directory to switch to" is > more informative. Furthermore, for Unix users, PWD might evoke > *current* working directory because that is what pwd actually prints. > As a matter of fact, even in HelenOS pwd exists. > > I changed the message to "No previous directory to switch to". > > > > > >> > >> And the last thing is that you put this functionality inside libc. > >> Although it might seem to be a nice feature when the library itself > >> remembers previous directory, it is not. First of all, it complicates > >> working with directory named '-'. Although such name is weird, it is > >> perfectly valid and your patch makes it - from some point of view - > >> inaccessible. Next, it makes the function more powerful than it should > >> be. That makes its description (documentation) more complicated and in > >> my humble opinion, functions in base system library shall have as > >> simple (unambiguous) functionality as possible. I think that the > >> correct approach is to implement this functionality inside shell > >> itself. But even there, an option to override the meaning - to allow > >> cding to '-' - has to be present. > > > > > > I now handle 'cd -' in cd.c itself. But, I have added a getpwd function > I have some doubts about the getpwd name. As I stated above, pwd > prints the current directory, not previous and thus the name is a bit > misleading. Especially when no documentation is provided. > I changed the function name to getprevwd. This function name is informative of its nature and is also not very long. > > > inside the libc ( I think that should be its proper place. ). Also the > > changes in chdir are minimal now ( It sets the pwd parameter. ). > > Regarding the second issue ( '-' could be a valid filename ), we have to > > compromise somewhere. This is similar to the case of 'cd ..' . I could > > implement something like 'cd ...' to maintain the format and forbid the > user > Probably I didn't express myself clearly. My point is that if we add > such behavior, we must provide way to override it. For example, 'cd -' > to switch to previous directory and 'cd -- -' to switch to directory > '-' (taking '--' as way to end option specification). Using different > syntax is not a solution. > I have implemented the override feature in the new patch. Please have a look at it. If you find any problems/ suggestions please let me know. > > > to create a new file named '...' just like it does for '..' ( Very naive > > solution. ). > You can forbid user create files that are against specification. But > you cannot do this just to have some "cool" extension. Also consider > situation when the filesystem was created elsewhere and you just want > to mount it under HelenOS. > Yea, multiple file systems will create problems. Thanks! > > > > > > > > I am happy to receive all the feedback and open to any > > suggestions/improvements further to my patches. Working with HelenOS > seems > One more thing applies to your style in general. Looking at your > patches, I see mixing of > if (...) { > xxx > } > versus > if (...) > { > xxx > } > In HelenOS, the former is used. The latter style is used solely for > functions. > > Fixed the format. I will keep that in mind. :-) > Last thing, the line memset(buffer, 0, sizeof(PATH_MAX)) looks odd. > First, you probably did not want to apply sizeof() here. Plus there is > no need to zero the buffer because getpwd is safe - the string is > always zero terminated. Exception is when NULL is returned due to > error and the buffer is untouched. Thus simple buffer[0] = 0 to create > empty string shall be sufficient. > Actually, I borrowed the convention from cmds/modules/ls/ls.c. I have removed that from the current patch. All the issues that you suggested are fixed in the current patch. Please have a look at it. Thank you very much for your feedback. I am always open to any more suggestions. Cheers, Ketan > > Cheers, > - Vojta > > > to be fun! :) > > > > > > Cheers, > > Ketan > > > > > >> > >> Thanks again. I would be glad to apply your patch once you address, or > >> explain, the issues mentioned above. > >> > >> Cheers, > >> - Vojta > >> > >> > >> > suggestions are most welcome. I am a student at Indian Institute of > >> > Technology Guwahati in my senior year pursuing Bachelors in Computer > >> > Science. I will contribute more patches as I delve deeper in the > code. I > >> > am > >> > interested in the following projects order in decreasing order of > >> > preference: > >> > > >> > 1. Support PAE on ia32 > >> > 2. Improve the hash table implementation. > >> > 3. Graceful system shutdown. > >> > > >> > I have also contributed to PintOS ( > >> > http://www.scs.stanford.edu/10wi-cs140/pintos/pintos.html ) as an > >> > academic > >> > project in my university. I think I will do well as a part of HelenOS > >> > team > >> > and will help in improving the system. > >> > > >> > Cheers, > >> > Ketan Singh > >> > Senior Undergraduate Student, > >> > Department of Computer Science, Indian Institute of Technology > Guwahati > >> > India > >> > > >> > _______________________________________________ > >> > HelenOS-devel mailing list > >> > [email protected] > >> > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > >> > > >> > >> _______________________________________________ > >> HelenOS-devel mailing list > >> [email protected] > >> http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > > > > > > > > _______________________________________________ > > HelenOS-devel mailing list > > [email protected] > > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > > > > _______________________________________________ > HelenOS-devel mailing list > [email protected] > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel >
cd_with_override.patch
Description: Binary data
_______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
