Re: Making use of string items in the capability system.

[Marcus Brinkmann]

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