At Fri, 04 Mar 2005 17:33:22 +0000, Neal H. Walfield wrote: > > ***task_create(hurd_cap_handle_t current_task_handle, > > hurd_cap_handle_t *new_task_handle) > > > > This create a new task, with just one thread, and creates an handle > > associated with it so you can act on the task. > > The first argument should be a task server root capability found in > the hurd_startup_data.
I disagree. We need task groups, so we can kill off child tasks which did not register themselves with the proc server. This is not only important for sub-hurds like boot, or similar applications where a process doesn't register child tasks with the proc server, but also to clean up after a partial fork() or exec(), which otherwise could leave behind unregistered tasks in a broken state. The task group ID can just be a protected word that can only be set by a privileged server, for example proc, and which is inherited at task creation. proc could set this word to something as simple as the PID. Killing a task should kill all tasks with the same task group ID, too. This ensures that no stranded unregistered tasks are left behind. So, yes, the task_create RPC should go to the "parent task" capability. Furthermore, we need a privileged "task_set_group_id" RPC that sets the protected word. And task_destroy should kill all tasks in the same group. BTW, one important thing to consider is how suid exec will work. If we want proper accounting, suid tasks should be donated by the starter to the filesystem, but the filesystem needs total control over it. So some revoke-and-reparent operation will be needed: revoke to remove access by the original starter, and reparent to associate the donated task to the filesystem doing the suid exec. So, the task_set_group_id should also be permitted for non-privileged users in the special case that you want to set the group ID to the group ID of your own task (rather than any arbitrary group ID). Ie, you claim a task you have control over (you have the task capability) as belonging to your own task group (there are no security problems with this I think - there _are_ security issues with donating your own tasks to somebody else's task group). BTW, using a protected word as task group ID is just an optimization - you could generalize this and use task group capabilities. But I think that the optimization is worthwhile here - task group as distinct cap objects don't offer us enough to be worth the overhead I think. (IE, I don't see any value in giving another task group control over your own task group, except for proc which has "control" over all task groups, ie can arbitrary change the task group ID). No "root task server" capability seems to be needed at all for what I know. The thing that comes next to it is the "control" or "master" capability (or whatever it will be) that is used by proc. > Second, allocating a task should not allocate a thread. Those are > separate operations there is no need to artificially merge them at > this level. This is easy enough to do at the POSIX level. Well, maybe. But not because they are separate operations, but because there are optimizations possible then. In particular, you can allocate the address space lazily at first thread creation. Consider the suid exec: The starter creates a task object and donates it to the filesystem. The filesystem revokes all access except its own. To be secure, all threads in the task must be trashed, and the address space has to be purged (this means destroying the address space and creating a new one). This is most efficient if you didn't create an address space in the first place and the task is still "pristine". However, if you are not donating the task, I don't see a point in making this two operations. A task without threads is pretty much useless in L4. So, as an optimization, I would not object in allowing to request creation of N initial threads in the task. IE, folding the functionality of thread creation into the task creation RPC. In fact, this could even actually be one and the same RPC (with a flag saying if you want the threads in the same or a new task). > > -By a separate RPC, task_set_pager(hurd_cap_handle_t task_handle, > > l4_thread_id_t pager, > > l4_thread_id_t thread). > > This isn't needed if we set the pager when creating the thread. Then > it automatically starts waiting for a message from the pager. Yes. I think that it should be part of the creation call. > > *** task_thread_alloc(hurd_cap_handle_t task_handle, > > l4_thread_id_t pager, > > l4_word_t number_requested, > > l4_word_t all_or_nothing, > > l4_word_t *number_allocated, > > l4_thread_id_t *thread_ids) > > > > Neal wanted to alloc several threads at one time, so: > > > > This allocates NUMBER threads, or less if the server cannot or does > > not want (unless all_or_nothing is set, in which case no thread is > > allocated). Every created thread has its pager set to PAGER. > > This looks good. But rename all_or_nothing to flags and create a > macro called TASK_THREAD_ALLOW_PARTIAL. This way, we can add more > flags later. Yes. Allocating multiple threads at once and feed them into a local cache is a good optimization. However, I wonder why you need to fail if you can't allocate N threads. You could also just always default to "allow partial" and then deallocate the threads if you didn't get as many as you wanted, in case you really don't want them. So, unless somebody shows me why such a flag is needed, I would just vote for not having such a flag at all. > > *** task_thread_dealloc(hurd_cap_handle_t task_handle, > > l4_word_t number, > > l4_thread_id_t *thread_ids) > > > > This destroy the threads and put the thread ids back in the free > > thread ids list. > > Good. But you need to know how many were successfully deallocated. > If I pass 3 tids and the second is bad (but the first and third are > valid) then only the first should be deallocated and an error, such as > EINVAL, should be returned and the number of deallocations set to 1. I don't think that's a smart interface. If you want to allow to return RPC "out" arguments _and_ an error code, then you should realize that this means that: 1. All our RPC server stubs must return valid return arguments, even on errors. (ie, it must make sure that returned caps are HURD_CAP_NULL, all "nr" types are 0, pointers are 0 or -1 or similar, etc). In particular, for all return arguments there must be a value that indicates that the return argument is not valid. 2. All client RPC stubs must go through all return arguments and clean them up, even on errors. I think that this makes the RPC stub code generation a lot harder and increases interface complexity enormously. And this for a small number of interfaces that potentially work this way. Instead, I would suggest that you change the interface to return the failure reason as part of the return arguments, not as the error code of the whole RPC, and just return success. Then only the caller of this particular RPC must be changed to not only check the return code, but also how many threads were actually deallocated and do something in case this was not the whole list. But, the caller of such an interface must already be aware of the possibility of partial success, so this is not a cost that is imposed by my suggestion. Rather, my suggestion aims at not introducing costs for _all other_ RPCs. There is a semantical issue here. I think for simplicity, it is worthwhile to consider that anything but "0" returned from an IPC indicates complete failure - in particular that no arguments are returned. Otherwise, we will have to make sure that _all_ return arguments are valid whenever we return an error. This is important to make the RPC stub code work properly. So, for example, you could return N error codes, one for each thread for which deallocation was attempted. Or you could just return the number X of the deallocated threads starting from the first one, and an error code that gives a reason for the failure of the X+1 one. > > When there is only one thread left, the thread is not destroyed but > > just inactivated. > > This is an interesting case. I think it might be better to just > implicitly deallocate the address space. I agree with Neal here. I think that there is no need to keep tasks with an address space with mappings around that doesn't contain any threads. Dealing with the need to keep one thread around to keep the address space intact can happily be punted to the user code. > The rest of this patch looks fine as do the other 2 patches. Marcus > can check them in if he is happy with them. I'll look at it later... Marcus _______________________________________________ L4-hurd mailing list [email protected] http://lists.gnu.org/mailman/listinfo/l4-hurd
