[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Making use of string items in the capability system.
From: |
Marcus Brinkmann |
Subject: |
Re: Making use of string items in the capability system. |
Date: |
Wed, 16 Feb 2005 03:05:08 +0100 |
User-agent: |
Wanderlust/2.10.1 (Watching The Wheels) SEMI/1.14.6 (Maruoka) FLIM/1.14.6 (Marutamachi) APEL/10.6 Emacs/21.3 (i386-pc-linux-gnu) MULE/5.0 (SAKAKI) |
Hi,
How about if you rework this patch a bit according to the comments
below, and resubmit it together with the bug fixes you sent in that
other mail (the assertion fixes)?
* You use "string item" and "string item buffer" in a lot of places
where it should be "string buffer".
* libhurd-cap-server/bucket-create.c
You write:
while (*size++) {string_item_sizes_length++;}
First: Always follow the coding style guidelines. This should be:
while (*size++)
string_item_sizes_length++;
But in fact, I suggest that you change the signature of the function to:
error_t hurd_cap_bucket_create (hurd_cap_bucket_t *r_bucket,
int string_buf_count, size_t *string_buf_sizes);
So that the number is already known at invocation time, and no
trailing NULL is necessary as terminator. Also store this number in
the bucket and use it in the manager code. It's just too useful.
Furthermore, you go to a lot of trouble, including l4.h, multiplying
the number of size_t's with sizeof l4_word_t to allocate the buffer,
etc. That's an unnecessary L4 dependency and conversion. You can
just use size_t as the type, and then the whole thing simplifies to
a malloc and memcpy.
Now, this will not round up to a page multiple, but that can also be
done later. I think I said at some time that different allocation
strategies are possible (on the stack, with malloc, or with mmap),
depending on how large string items you want to support. This can
all be hidden deep down in the implementation. In any case we can
always impose the user-provided limit exactly as given (even if we
allocate a bit more with mmap).
So, this function should just store the unmodified size_t array in
the bucket structure.
You do not check the return value of malloc. It may be NULL.
* The limit MAX_STRING_ITEMS should be defined as
#define HURD_CAP_MAX_STRING_BUF (L4_NUM_BRS - 1) / 2
or so. I think we should not define it in l4 headers, as this
calculation assumes that you don't use compound strings, so it is
not generally useful.
This limit should be enforced in bucket_create (return EINVAL if it is
violated)
* libhurd-cap-server/bucket-manage-mt.c
allocate_string_item_buffers(): You return an "error_t" but also a -1.
We do not return -1 and the error number in errno in our functions.
Instead, we just return the error directly. In this case, you can do:
error_t saved_errno = errno;
free_string_items...
return saved_errno;
You do not check the return value of allocate_string_item_buffers
when you call it.
free_string_item_buffers: It's not very useful for deallocators to
fail. It would indicate a severe bug. If you are really paranoid,
you could assert on the munmap return value (which btw is also not
an error value but -1, you'd have to return errno), but in fact I
would just ignore it (and change free_string_item_buffers to return
void).
Can you add a function flush_string_item_buffers() that would do
munmap/mmap for now? (and will later use our own pager feature to
just flush the underlying memory)
* We use NULL for null pointers, like in
void *foo = NULL;
or in:
bar (NULL);
(NULL is defined in stddef.h).
With some exceptions: If you allocate statically (or in a global),
we don't use any initializer at all:
static void *foo;
Or when doing comparisons, you can safely assume that NULL == false == 0:
if (foo)
if (!foo)
because that is just a common idiom.
* You free bucket->string_item_sizes at the end of the manager. This
seems to be wrong, as it is allocated in bucket-create. You can
deallocate it in bucket-free.c.
* The msg translation thingie violates strict aliasing rules, you may
want to look at how I fixed that, but don't worry, if you don't, I
will do it.
More importantly, I now believe that this should be done in one
operation with _L4_msg_store for efficiency. So, basically, have a
new function _L4_msg_store_parsed that does the translation.
Now that I see how you implemented the do-not-keep-structure version
it is apparent that the keep-structure version is not actually
needed. My confusion used to be that I thought the map and grant
items need to be in the same message registers as in the original
message, but that's just bollocks. We can happily move them around
as long as the order is preserved - the message stubs need to be
able to deal with arbitrary locations anyway in case gather is used
(if you mix map and grant items with string items).
Note that in case we _ever_ want to allow map/grant items in
messages, we may impose a policy in the Hurd that a message must
first contain all string items and then all map/grant items or
whatever. This also makes it simpler to deal with IPC errors (as
the kernel processes the elements in order).
So, again, we only need the version that simplifies the structure,
and we can just add it to a powerful version of the msg store
operation. This also solves the problem of using BRs directly (if
you can use MRs, you certainly can use BRs, or the user is doing
something stupid).
* There is the one or other odd coding style violation, like this:
err = hurd_cap_bucket_create (&bucket,0);
which lacks a space before the 0 that should be NULL (or "0, NULL"
with the two-argument version).
Also, as I said in my other mail, keep the lines short.
That's it! Great job. I am looking forward to incuding this patch.
Thanks,
Marcus