On Thu, 2005-03-31 at 23:42 -0800, Andrew Morton wrote: > Evgeniy Polyakov <[email protected]> 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
- Follow-Ups:
- Re: connector.c
- From: Andrew Morton <[email protected]>
- Re: connector.c
- References:
- Re: connector.c
- From: Evgeniy Polyakov <[email protected]>
- Re: connector.c
- From: Andrew Morton <[email protected]>
- Re: connector.c
- Prev by Date: Re: [patch 2.6.12-rc1-mm4] fork_connector: add a fork connector
- Next by Date: Re: cn_queue.c
- Previous by thread: Re: connector.c
- Next by thread: Re: connector.c
- Index(es):
 |