Re: exec server and /dev/fd/N

[Emilio Pozuelo Monfort]

Hi Carl,

Thanks for the thorough review!

On 25/05/10 21:10, Carl Fredrik Hammar wrote:
> Some of the changes that need to be made have already been discussed on
> IRC: you should use string_t instead of data_t, empty string instead of
> NULL for RPCs, and update copyright years,

OK

> The first three patches are pretty useless on their own, they all affect
> the same program, and most changes are pretty mechanical, so I think
> you might as well merge them.

OK. I thought about going step by step to not end with one single huge patch,
but maybe that was too much ;)

> You also haven't changed some calls to exec_exec and _hurd_exec() in
> Hurd and glib to _hurd_exec_file_name(), which would be appropriate if
> we're deprecating _hurd_exec().

Will do.

> It would also be good if you always include a ChangeLog so I can catch
> early errors there too, which hopefully leads to less round-trips.

OK

> Are you familiar with TopGit?  It is pretty useful when developing
> patch series since you can keep history of the development of a patch.
> It is also nice since it helps you maintain the ChangeLog if you keep
> it in .topmsg, which will later become the commit message for the patch.

I've never used it. I'm familiar with quilt, which has no history but maybe its
usage is somewhat similar? (I once saw a comparison between git, topgit and 
quilt)

> Now to the details.  I've commented on every hunk to make sure I looked
> through it all, which makes it a bit long but hopefully easy to follow
> (complain if it isn't).

I think that's perfect.

All your comments look fine and I'll address the problems in the next patch.
Just one comment:

>> diff --git a/hurd/Versions b/hurd/Versions
>> index 83c8ab1..81cd904 100644
>> --- a/hurd/Versions
>> +++ b/hurd/Versions
>> @@ -73,6 +73,7 @@ libc {
>>      _hurd_critical_section_unlock;
>>      _hurd_exception2signal;
>>      _hurd_exec;
>> +    _hurd_exec_file_name;
>>      _hurd_fd_get;
>>      _hurd_init;
>>      _hurd_intern_fd;
> 
> Oh, I haven't noticed this file before.  :-)
> 
> I don't really know how this file works but this seems to be a section
> devoted to compatibility to an older version of glibc so I doubt this is
> the correct place to put a new function.  You might want to ask Samuel
> or Thomas about it, or if that fails, libc-alpha and Roland.

It's the list of symbols libc.so should export. Since _hurd_exec is a public
API, I guessed _file_name should be too. But if we don't want it to be public, I
can probably put the prototype in hurd/hurdexec.h and include "hurdexec.h" from
execve.c.

Cheers,
Emilio