lkml.org 
[lkml]   [2005]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: connector.c
From
Date
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2005-04-06 13:31    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site