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    [W:0.025 / U:98.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site