Re: connector.c

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

On Thu, 2005-03-31 at 23:42 -0800, Andrew Morton wrote:
> Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> >
> > > What happens if we expect a reply to our message but userspace never sends
> > > one?  Does the kernel leak memory?  Do other processes hang?
> > 
> > It is only advice, one may easily skip seq/ack initialization.
> > I could remove it totally from the header, but decided to 
> > place it to force people to use more reliable protocols over netlink
> > by introducing such overhead.
> 
> hm.  I don't know what that means.

Messages that are passed between agents must have only id,
but I decided to force people to use provided seq/ack fields
to store there some information about message order.
Neither kernel nor userspace requires that fields to be 
somehow initialized.

> > > > 	nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> > > > 
> > > > 	data = (struct cn_msg *)NLMSG_DATA(nlh);
> > > 
> > > Unneeded typecast.
> > 
> > Is it really an issue?
> 
> Well it adds clutter, but more significantly the cast defeats typechecking.
> If someone was to change NLMSG_DATA() to return something other than
> void*, the compiler wouldn't complain.
> 
> > > 
> > > Why is spin_lock_bh() being used here?
> > 
> > skb may be delivered in soft irq context, and may race with sending.
> > And actually it can be sent from irq context, like it is done in test
> > module.
> 
> But spin_lock_bh() in irq context will deadlock if interruptible context is
> also doing spin_lock_bh().

skbs are delivered in softirq context and, yes,
I need to exactly point that sending also must be limited to soft irq
context.

> > > What's all the above code doing?  What do `a' and `b' mean?  Needs
> > > commentary and better-chosen identifiers.
> > 
> > It searches for idx and val to match requested notification, 
> > if "a" is true - idx is found, if b - val is found.
> 
> Let me rephrase: please comment the code and choose identifiers in a manner
> which makes it clearer what's going on.

Sure.

> > > Please document all functions with comments.  Functions which constitute
> > > part of the external API should be commented using the kernel-doc format.
> > 
> > There is Documentation/connector/connector.txt which describes all
> > exported functions and structures.
> > Should it be ported to docbook?
> 
> connector.txt is pitched at about the right level: an in-kernel and
> userspace API description.  It's rather unclear with respect to mesage
> directions though - whether the callback is invoked after kernel->user
> messages, or for user->kernel or what, for example.  Some clarification
> there would help.  

Callback is invoked each time message is received - either 
from userspace [original aim] or from kernelspace
[one can send notification request or just send a message from one 
kernelspace subsystem to another].
Callback can also be called when notification for given idx/val range
was requested and new callback is being registered/unregistered 
which mathces given idx/val range.

> But an API description is a different thing from code commentary which
> explains the internal design - the latter should be coupled to the code
> itself. 

I will begin create in-code documentation after all technical issues 
are resolved. Patches will be ready soon I think.

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

Attachment: signature.asc
Description: This is a digitally signed message part