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 ;-).
>
>>
>>
>> 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], "-")?
>
>>
>>
>> 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.
>
>
>>
>> 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.
> 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.
> 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.
>
>
> 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.
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.
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
