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: > Hello Ketan, > thanks for your patch and your interest in HelenOS. > > Dne 2. dubna 2012 21:24 Ketan Singh <[email protected]> napsal(a): > > Hello everyone, > > > > I am a GSOC 2012 aspirant and would like to contribute to HelenOS > project. I > > would like to submit a very simple patch to allow jump to previous > working > > directory in HelenOS using 'cd -' command. Please have a look at it. Any > There are few issues with your patch. Sorry, it is a bit longer but > today I have somehow problems with compressing the text ;-). > That sure helped a lot. :-) > > 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. > > 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. > > 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. > 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 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 to create a new file named '...' just like it does for '..' ( Very naive solution. ). I am happy to receive all the feedback and open to any suggestions/improvements further to my patches. Working with HelenOS seems 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 >
unistd.patch
Description: Binary data
vfs.patch
Description: Binary data
cd.patch
Description: Binary data
_______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
