Re: multiple capabilities in a single RPC

[Marcus Brinkmann]

At Wed, 19 Jan 2005 18:38:30 +0000,
Neal H. Walfield wrote:
> We need to move the declaration of struct _hurd_cap_list_item from
> cap-server-internal.h to cap-server.h.  The whole point of the use
> context is to create the list items on the stack and not allocate them
> internally.

Ok.  I don't know how we will ever maintain ABI compatibility with
this mess.

> Your code gratuitously checks the inhibited state of the client.  We
> know that the capability handle we are looking up belongs to the same
> client as the first capability handle (i.e. the one that we are
> dispatching the RPC on).  Hence, if we are running client->state must
> be either green or yellow.  You assert this yourself at the start of
> the function:

True.  That's weird, of course I knew that, our discussion was still
on my mind, but apparently I was absently minded.

It's either green or yellow, true.  I think we should return an error
if it is yellow.  It's just a quick check and makes sure that we don't
run against the flow.

> Is it worth adding the obj pointer to the use context?  Internally, we
> can trivially find it given the _obj_entry?  Then we can mark the
> whole structure as private.  Of course, we would have to return the
> capability object in one of the arguments but I don't think this is
> bad.

Yes, we can do that.  _obj_entry->cap_obj will be valid as explained
in the comment before the assert that makes sure the obj entry is not
dead.

I was just thinking that maybe it is more convenient for the user to
have it all in the struct (say, for select()).  But then, I don't
think it buys us much, so it can go.

> SAME_CLASS argument is really useless.  All it does is offer another
> assert.  If the argument is that it provides type checking (and we
> need a mechanism to do that),

Affirmative, that was the purpose.

> I think a more useful type check would
> be to have the user pass a pointer to a capability object class
> indicating what class the capability must be in.  This is a clean way
> to provide type checking.

True, I like that.

> The alternative is to add an accessor
> function for capability objects which returns the capability object's
> capability class (or allowing the user to inspect the structure
> directly).

Yeah.  The purpose of having the type check in start_cap_use is to
avoid doing extra work in the error case.  It probably doesn't matter
much, but unless we have a real need, I would go for the simpler
one-class-check only.  I am not sure we really need polymorphic
interfaces, and having the check in start_cap_use is a minor
optimization and also draws attention to the fact that you absolutely
must check the type (with the class arguemnt even more so).

> If we incorporate type checking into the start_cap_use
> function directly, this makes the case where a stub accepts multiple
> types difficult as it must call _start_cap_use multiple times, once
> for each type it accepts, until it succeeds.  We may just want to punt
> this until we see a real need but adding an accessor function to
> return a capability object's class is reasonably elegant.

Yeah, adding such an accessor is a no-brainer.  Should be an
always-inline function I'd say.

> If we require the user to pass the rpc context to _end_cap_use then we
> can elide the _status member.  All values are easily determined
> anyways.

Also true.  In that case I would propose to keep the enum internally
and have an inline function that calculates the status given two
objects - avoids code duplication and is self-documenting.

> Finally, I think the cap field name should be changed to handle.  In
> both the cap use context and the rpc context.  This is confusing.  Why
> is a capability object not a cap but an obj?  I think of a capability
> as the whole she-bang and I think we should call capability handles
> handles to avoid any confusion in that regard.

I will think about it.  It makes sense to me.  But note that a
"capability", that is to say a "hurd_cap_t" as a data-type is also
intended to be used for a specific type, and that is the one that will
end up being used at the highest level in a client.  Ie, what you
actually invoke RPCs on at the client side.

This means that naming the handle just "cap" is misleading and wrong,
but it's not just instance - it is named cap because of other similar
occurences.  I will include fixing up terminology into the next
complete review/overhaul.

> > There are probably some error values which should be changed, because
> > they are more suitable for the demuxer internals than for an actual
> > user.  I am thinking about the ECAP_* error codes.
> 
> Yes, I think so.  ECAP_* are not actually exported and I don't think
> we want to export them.  Maybe ESRCH if the cap is not found and EIEIO
> if it died.

Yeah, something like that.  I don't really care much, whatever does
the job for the stubs using the function will be fine.  If we fail
with this error in the stub, it must be something that the user can
understand (that's why I used EINVAL in some places).

> > You can not lock two objects at the same time, unless you define a
> > locking hierarchy for all the objects in the set, or find some other
> > dead-lock-avoidance strategy.
> 
> In case it wasn't obvious, there is a trivial general solution: we
> lexically sort the capability objects by their location and use that
> as the locking order.

You mean the memory address?  Mmh, quite true.  You should get a
patent on that trick.

Thanks,
Marcus