connector.c

connector.c

[Andrew Morton]

Some belated comments...


> 
> module_param(unit, int, 0);
> module_param(cn_idx, uint, 0);
> module_param(cn_val, uint, 0);

MODULE_PARM_DESC needed, please.

> static DEFINE_SPINLOCK(notify_lock);
> static LIST_HEAD(notify_list);
> 
> static struct cn_dev cdev;
> 
> int cn_already_initialized = 0;
> 
> /*
>  * msg->seq and msg->ack are used to determine message genealogy.
>  * When someone sends message it puts there locally unique sequence 
>  * and random acknowledge numbers.
>  * Sequence number may be copied into nlmsghdr->nlmsg_seq too.
>  *
>  * Sequence number is incremented with each message to be sent.
>  *
>  * If we expect reply to our message, 
>  * then sequence number in received message MUST be the same as in original message,
>  * and acknowledge number MUST be the same + 1.
>  *
>  * If we receive message and it's sequence number is not equal to one we are expecting, 
>  * then it is new message.
>  * If we receive message and it's sequence number is the same as one we are expecting,
>  * but it's acknowledge is not equal acknowledge number in original message + 1,
>  * then it is new message.
>  *
>  */

This comment looks crappy in an 80-col xterm.

What happens if we expect a reply to our message but userspace never sends
one?  Does the kernel leak memory?  Do other processes hang?

> void cn_netlink_send(struct cn_msg *msg, u32 __groups)
> {
> 	struct cn_callback_entry *n, *__cbq;
> 	unsigned int size;
> 	struct sk_buff *skb, *uskb;
> 	struct nlmsghdr *nlh;
> 	struct cn_msg *data;
> 	struct cn_dev *dev = &cdev;
> 	u32 groups = 0;
> 	int found = 0;
> 
> 	if (!__groups)
> 	{

Wrong indenting.

> 		spin_lock_bh(&dev->cbdev->queue_lock);
> 		list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 			if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> 				found = 1;
> 				groups = __cbq->group;
> 			}
> 		}
> 		spin_unlock_bh(&dev->cbdev->queue_lock);
> 
> 		if (!found) {
> 			printk(KERN_ERR "Failed to find multicast netlink group for callback[0x%x.0x%x]. seq=%u\n",
> 			       msg->id.idx, msg->id.val, msg->seq);

Needs wrapping.

> 			return;
> 		}
> 	}
> 	else
> 		groups = __groups;
> 
> 	size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> 
> 	skb = alloc_skb(size, GFP_ATOMIC);

GFP_ATOMIC is quite unreliable.  Better to find a way to use GFP_KERNEL here.

> 	if (!skb) {
> 		printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> 		return;
> 	}

Surely we should return -ENOMEM here?  How is the caller to know that the
send attempt worked?

> 	nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> 
> 	data = (struct cn_msg *)NLMSG_DATA(nlh);

Unneeded typecast.

> 	memcpy(data, msg, sizeof(*data) + msg->len);
> #if 0
> 	printk("%s: len=%u, seq=%u, ack=%u, group=%u.\n",
> 	       __func__, msg->len, msg->seq, msg->ack, groups);
> #endif
> 	
> 	NETLINK_CB(skb).dst_groups = groups;
> 
> 	uskb = skb_clone(skb, GFP_ATOMIC);
> 	if (uskb) {
> 		netlink_unicast(dev->nls, uskb, 0, 0);
> 	}

Unneeded {}

> 	netlink_broadcast(dev->nls, skb, 0, groups, GFP_ATOMIC);

GFP_ATOMIC is quite undesirable.

> 
> static int cn_call_callback(struct cn_msg *msg, void (*destruct_data) (void *), void *data)

80 cols

> {
> 	struct cn_callback_entry *n, *__cbq;
> 	struct cn_dev *dev = &cdev;
> 	int found = 0;
> 
> 	spin_lock_bh(&dev->cbdev->queue_lock);
> 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {

80 cols

> 		if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> 			__cbq->cb->priv = msg;
> 
> 			__cbq->ddata = data;
> 			__cbq->destruct_data = destruct_data;
> 
> 			queue_work(dev->cbdev->cn_queue, &__cbq->work);
> 			found = 1;
> 			break;
> 		}
> 	}
> 	spin_unlock_bh(&dev->cbdev->queue_lock);
> 
> 	return found;
> }

Why is spin_lock_bh() being used here?

> static int __cn_rx_skb(struct sk_buff *skb, struct nlmsghdr *nlh)
> {
> 	u32 pid, uid, seq, group;
> 	struct cn_msg *msg;
> 
> 	pid = NETLINK_CREDS(skb)->pid;
> 	uid = NETLINK_CREDS(skb)->uid;
> 	seq = nlh->nlmsg_seq;
> 	group = NETLINK_CB((skb)).groups;
> 	msg = (struct cn_msg *)NLMSG_DATA(nlh);
> 
> 	if (NLMSG_SPACE(msg->len + sizeof(*msg)) != nlh->nlmsg_len) {
> 		printk(KERN_ERR "skb does not have enough length: "
> 				"requested msg->len=%u[%u], nlh->nlmsg_len=%u, skb->len=%u.\n",

80 cols (all over the place)

> static void cn_notify(struct cb_id *id, u32 notify_event)
> {
> 	struct cn_ctl_entry *ent;
> 
> 	spin_lock_bh(&notify_lock);
> 	list_for_each_entry(ent, &notify_list, notify_entry) {
> 		int i;
> 		struct cn_notify_req *req;
> 		struct cn_ctl_msg *ctl = ent->msg;
> 		int a, b;
> 
> 		a = b = 0;
> 		
> 		req = (struct cn_notify_req *)ctl->data;
> 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {

		for (i = 0; i < ctl->idx_notify_num; i++, req++) {

> 			if (id->idx >= req->first && id->idx < req->first + req->range) {
> 				a = 1;
> 				break;
> 			}
> 		}
> 		
> 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> 			if (id->val >= req->first && id->val < req->first + req->range) {
> 				b = 1;
> 				break;
> 			}
> 		}
> 
> 		if (a && b) {
> 			struct cn_msg m;
> 			
> 			printk(KERN_INFO "Notifying group %x with event %u about %x.%x.\n", 
> 					ctl->group, notify_event, 
> 					id->idx, id->val);
> 
> 			memset(&m, 0, sizeof(m));
> 			m.ack = notify_event;
> 
> 			memcpy(&m.id, id, sizeof(m.id));
> 			cn_netlink_send(&m, ctl->group);
> 		}

What's all the above code doing?  What do `a' and `b' mean?  Needs
commentary and better-chosen identifiers.

> 	}
> 	spin_unlock_bh(&notify_lock);
> }
> 
> int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *))
> {
> 	int err;
> 	struct cn_dev *dev = &cdev;
> 	struct cn_callback *cb;
> 
> 	cb = kmalloc(sizeof(*cb), GFP_KERNEL);
> 	if (!cb) {
> 		printk(KERN_INFO "%s: Failed to allocate new struct cn_callback.\n",
> 		       dev->cbdev->name);
> 		return -ENOMEM;
> 	}
> 
> 	memset(cb, 0, sizeof(*cb));
> 
> 	snprintf(cb->name, sizeof(cb->name), "%s", name);

scnprintf?

> 	memcpy(&cb->id, id, sizeof(cb->id));
> 	cb->callback = callback;
> 
> 	atomic_set(&cb->refcnt, 0);
> 
> 	err = cn_queue_add_callback(dev->cbdev, cb);
> 	if (err) {
> 		kfree(cb);
> 		return err;
> 	}
> 			
> 	cn_notify(id, 0);
> 
> 	return 0;
> }
> 
> void cn_del_callback(struct cb_id *id)
> {
> 	struct cn_dev *dev = &cdev;
> 	struct cn_callback_entry *n, *__cbq;
> 
> 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 		if (cn_cb_equal(&__cbq->cb->id, id)) {
> 			cn_queue_del_callback(dev->cbdev, __cbq->cb);
> 			cn_notify(id, 1);
> 			break;
> 		}
> 	}
> }

Doesn't this list walk need locking?

Please document all functions with comments.  Functions which constitute
part of the external API should be commented using the kernel-doc format.

> static void cn_callback(void * data)
> {
> 	struct cn_msg *msg = (struct cn_msg *)data;
> 	struct cn_ctl_msg *ctl;
> 	struct cn_ctl_entry *ent;
> 	u32 size;
>  
> 	if (msg->len < sizeof(*ctl)) {
> 		printk(KERN_ERR "Wrong connector request size %u, must be >= %u.\n", 
> 				msg->len, sizeof(*ctl));
> 		return;
> 	}
> 	
> 	ctl = (struct cn_ctl_msg *)msg->data;
> 
> 	size = sizeof(*ctl) + (ctl->idx_notify_num + ctl->val_notify_num)*sizeof(struct cn_notify_req);
> 
> 	if (msg->len != size) {
> 		printk(KERN_ERR "Wrong connector request size %u, must be == %u.\n", 
> 				msg->len, size);
> 		return;
> 	}
> 
> 	if (ctl->len + sizeof(*ctl) != msg->len) {
> 		printk(KERN_ERR "Wrong message: msg->len=%u must be equal to inner_len=%u [+%u].\n", 
> 				msg->len, ctl->len, sizeof(*ctl));
> 		return;
> 	}
> 
> 	/*
> 	 * Remove notification.
> 	 */
> 	if (ctl->group == 0) {
> 		struct cn_ctl_entry *n;
> 		
> 		spin_lock_bh(&notify_lock);
> 		list_for_each_entry_safe(ent, n, &notify_list, notify_entry) {
> 			if (cn_ctl_msg_equals(ent->msg, ctl)) {
> 				list_del(&ent->notify_entry);
> 				kfree(ent);
> 			}
> 		}
> 		spin_unlock_bh(&notify_lock);
> 
> 		return;
> 	}
> 
> 	size += sizeof(*ent);
> 
> 	ent = kmalloc(size, GFP_ATOMIC);

Another GFP_ATOMIC :(  In what context can this function be called?

> 	{
> 		int i;
> 		struct cn_notify_req *req;

May as well move these up to the top of the function, save the ugly indent.

> 		printk("Notify group %x for idx: ", ctl->group);
> 
> 		req = (struct cn_notify_req *)ctl->data;
> 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {
> 			printk("%u-%u ", req->first, req->first+req->range-1);
> 		}

Unneeded braces.

This file is full of trailing whitespace, btw.  Please fix that up, then
find a new editor.

> 		printk("\nNotify group %x for val: ", ctl->group);
> 
> 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> 			printk("%u-%u ", req->first, req->first+req->range-1);
> 		}

Braces.

Re: connector.c

[Tommy Reynolds]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

Uttered Andrew Morton <akpm@osdl.org>, spake thus:

> > 	if (uskb) {
> > 		netlink_unicast(dev->nls, uskb, 0, 0);
> > 	}
> 
> Unneeded {}

Speaking strictly as a language lawyer, they are not needed.

However, for maintainability (and best practices) they are essential.
They make the scope of the test explicit and ensure that a one-line-
change really doesn't affect more lines that needed.  Think context
diffs here.

Cheers!

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: connector.c

[Matthias Urlichs]

Hi,   Tommy Reynolds schrub am Thu, 31 Mar 2005 20:41:35 -0600:

> Uttered Andrew Morton <akpm@osdl.org>, spake thus:
> 
>> > 	if (uskb) {
>> > 		netlink_unicast(dev->nls, uskb, 0, 0);
>> > 	}
>> 
>> Unneeded {}
> 
> However, for maintainability (and best practices) they are essential.

They do add visual clutter, though, so they make the code as-is less
readable.

I don't think it's entirely accidental that Python is so much more
readable. (For me, anyway -- YMMV and all that.)

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de

Re: connector.c

[Paul Jackson]

> I don't think it's entirely accidental that Python

Hmmm ... cutting off a brace-war at the pass
by escalating to a Python war -- good work ;).

Please suppress any urge to reply ...

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: connector.c

[Evgeniy Polyakov]

[-- Attachment #1: Type: text/plain, Size: 11512 bytes --]

On Thu, 2005-03-31 at 17:30 -0800, Andrew Morton wrote:
> Some belated comments...
> 
> 
> > 
> > module_param(unit, int, 0);
> > module_param(cn_idx, uint, 0);
> > module_param(cn_val, uint, 0);
> 
> MODULE_PARM_DESC needed, please.

Yep.

> > static DEFINE_SPINLOCK(notify_lock);
> > static LIST_HEAD(notify_list);
> > 
> > static struct cn_dev cdev;
> > 
> > int cn_already_initialized = 0;
> > 
> > /*
> >  * msg->seq and msg->ack are used to determine message genealogy.
> >  * When someone sends message it puts there locally unique sequence 
> >  * and random acknowledge numbers.
> >  * Sequence number may be copied into nlmsghdr->nlmsg_seq too.
> >  *
> >  * Sequence number is incremented with each message to be sent.
> >  *
> >  * If we expect reply to our message, 
> >  * then sequence number in received message MUST be the same as in original message,
> >  * and acknowledge number MUST be the same + 1.
> >  *
> >  * If we receive message and it's sequence number is not equal to one we are expecting, 
> >  * then it is new message.
> >  * If we receive message and it's sequence number is the same as one we are expecting,
> >  * but it's acknowledge is not equal acknowledge number in original message + 1,
> >  * then it is new message.
> >  *
> >  */
> 
> This comment looks crappy in an 80-col xterm.

Does anyone still use it? :)

> 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.

> > void cn_netlink_send(struct cn_msg *msg, u32 __groups)
> > {
> > 	struct cn_callback_entry *n, *__cbq;
> > 	unsigned int size;
> > 	struct sk_buff *skb, *uskb;
> > 	struct nlmsghdr *nlh;
> > 	struct cn_msg *data;
> > 	struct cn_dev *dev = &cdev;
> > 	u32 groups = 0;
> > 	int found = 0;
> > 
> > 	if (!__groups)
> > 	{
> 
> Wrong indenting.

Yep, my fault...

> > 		spin_lock_bh(&dev->cbdev->queue_lock);
> > 		list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> > 			if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> > 				found = 1;
> > 				groups = __cbq->group;
> > 			}
> > 		}
> > 		spin_unlock_bh(&dev->cbdev->queue_lock);
> > 
> > 		if (!found) {
> > 			printk(KERN_ERR "Failed to find multicast netlink group for callback[0x%x.0x%x]. seq=%u\n",
> > 			       msg->id.idx, msg->id.val, msg->seq);
> 
> Needs wrapping.
> 
> > 			return;
> > 		}
> > 	}
> > 	else
> > 		groups = __groups;
> > 
> > 	size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> > 
> > 	skb = alloc_skb(size, GFP_ATOMIC);
> 
> GFP_ATOMIC is quite unreliable.  Better to find a way to use GFP_KERNEL here.

It can be used in bh context, my first TODO entryf for connector is
provideing
extended API with GFP flags provided by caller.

> > 	if (!skb) {
> > 		printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> > 		return;
> > 	}
> 
> Surely we should return -ENOMEM here?  How is the caller to know that the
> send attempt worked?

It is notification area, notification may fail and original applicant
can not 
and should not know about it.
If it was direct call, then error must be propagated to the caller, 
but indirect notification is not.

> > 	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?
I try to always dereference void pointer to the given type...

> > 	memcpy(data, msg, sizeof(*data) + msg->len);
> > #if 0
> > 	printk("%s: len=%u, seq=%u, ack=%u, group=%u.\n",
> > 	       __func__, msg->len, msg->seq, msg->ack, groups);
> > #endif
> > 	
> > 	NETLINK_CB(skb).dst_groups = groups;
> > 
> > 	uskb = skb_clone(skb, GFP_ATOMIC);
> > 	if (uskb) {
> > 		netlink_unicast(dev->nls, uskb, 0, 0);
> > 	}
> 
> Unneeded {}

Ok.

> > 	netlink_broadcast(dev->nls, skb, 0, groups, GFP_ATOMIC);
> 
> GFP_ATOMIC is quite undesirable.

It will be changed to provided GFP flags by caller soon.

> > 
> > static int cn_call_callback(struct cn_msg *msg, void (*destruct_data) (void *), void *data)
> 
> 80 cols
> 
> > {
> > 	struct cn_callback_entry *n, *__cbq;
> > 	struct cn_dev *dev = &cdev;
> > 	int found = 0;
> > 
> > 	spin_lock_bh(&dev->cbdev->queue_lock);
> > 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 
> 80 cols

Grrr....

> > 		if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> > 			__cbq->cb->priv = msg;
> > 
> > 			__cbq->ddata = data;
> > 			__cbq->destruct_data = destruct_data;
> > 
> > 			queue_work(dev->cbdev->cn_queue, &__cbq->work);
> > 			found = 1;
> > 			break;
> > 		}
> > 	}
> > 	spin_unlock_bh(&dev->cbdev->queue_lock);
> > 
> > 	return found;
> > }
> 
> 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.

> > static int __cn_rx_skb(struct sk_buff *skb, struct nlmsghdr *nlh)
> > {
> > 	u32 pid, uid, seq, group;
> > 	struct cn_msg *msg;
> > 
> > 	pid = NETLINK_CREDS(skb)->pid;
> > 	uid = NETLINK_CREDS(skb)->uid;
> > 	seq = nlh->nlmsg_seq;
> > 	group = NETLINK_CB((skb)).groups;
> > 	msg = (struct cn_msg *)NLMSG_DATA(nlh);
> > 
> > 	if (NLMSG_SPACE(msg->len + sizeof(*msg)) != nlh->nlmsg_len) {
> > 		printk(KERN_ERR "skb does not have enough length: "
> > 				"requested msg->len=%u[%u], nlh->nlmsg_len=%u, skb->len=%u.\n",
> 
> 80 cols (all over the place)

Ok...

> > static void cn_notify(struct cb_id *id, u32 notify_event)
> > {
> > 	struct cn_ctl_entry *ent;
> > 
> > 	spin_lock_bh(&notify_lock);
> > 	list_for_each_entry(ent, &notify_list, notify_entry) {
> > 		int i;
> > 		struct cn_notify_req *req;
> > 		struct cn_ctl_msg *ctl = ent->msg;
> > 		int a, b;
> > 
> > 		a = b = 0;
> > 		
> > 		req = (struct cn_notify_req *)ctl->data;
> > 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {
> 
> 		for (i = 0; i < ctl->idx_notify_num; i++, req++) {
> 
> > 			if (id->idx >= req->first && id->idx < req->first + req->range) {
> > 				a = 1;
> > 				break;
> > 			}
> > 		}
> > 		
> > 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> > 			if (id->val >= req->first && id->val < req->first + req->range) {
> > 				b = 1;
> > 				break;
> > 			}
> > 		}
> > 
> > 		if (a && b) {
> > 			struct cn_msg m;
> > 			
> > 			printk(KERN_INFO "Notifying group %x with event %u about %x.%x.\n", 
> > 					ctl->group, notify_event, 
> > 					id->idx, id->val);
> > 
> > 			memset(&m, 0, sizeof(m));
> > 			m.ack = notify_event;
> > 
> > 			memcpy(&m.id, id, sizeof(m.id));
> > 			cn_netlink_send(&m, ctl->group);
> > 		}
> 
> 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.

> > 	}
> > 	spin_unlock_bh(&notify_lock);
> > }
> > 
> > int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *))
> > {
> > 	int err;
> > 	struct cn_dev *dev = &cdev;
> > 	struct cn_callback *cb;
> > 
> > 	cb = kmalloc(sizeof(*cb), GFP_KERNEL);
> > 	if (!cb) {
> > 		printk(KERN_INFO "%s: Failed to allocate new struct cn_callback.\n",
> > 		       dev->cbdev->name);
> > 		return -ENOMEM;
> > 	}
> > 
> > 	memset(cb, 0, sizeof(*cb));
> > 
> > 	snprintf(cb->name, sizeof(cb->name), "%s", name);
> 
> scnprintf?

Returned value is ignored here, so they do not differ.
Will change it though.

> > 	memcpy(&cb->id, id, sizeof(cb->id));
> > 	cb->callback = callback;
> > 
> > 	atomic_set(&cb->refcnt, 0);
> > 
> > 	err = cn_queue_add_callback(dev->cbdev, cb);
> > 	if (err) {
> > 		kfree(cb);
> > 		return err;
> > 	}
> > 			
> > 	cn_notify(id, 0);
> > 
> > 	return 0;
> > }
> > 
> > void cn_del_callback(struct cb_id *id)
> > {
> > 	struct cn_dev *dev = &cdev;
> > 	struct cn_callback_entry *n, *__cbq;
> > 
> > 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> > 		if (cn_cb_equal(&__cbq->cb->id, id)) {
> > 			cn_queue_del_callback(dev->cbdev, __cbq->cb);
> > 			cn_notify(id, 1);
> > 			break;
> > 		}
> > 	}
> > }
> 
> Doesn't this list walk need locking?

Ugh, you are tight, I totally wrong here.
It requires dev->queue_lock to be hold.

> 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?

> > static void cn_callback(void * data)
> > {
> > 	struct cn_msg *msg = (struct cn_msg *)data;
> > 	struct cn_ctl_msg *ctl;
> > 	struct cn_ctl_entry *ent;
> > 	u32 size;
> >  
> > 	if (msg->len < sizeof(*ctl)) {
> > 		printk(KERN_ERR "Wrong connector request size %u, must be >= %u.\n", 
> > 				msg->len, sizeof(*ctl));
> > 		return;
> > 	}
> > 	
> > 	ctl = (struct cn_ctl_msg *)msg->data;
> > 
> > 	size = sizeof(*ctl) + (ctl->idx_notify_num + ctl->val_notify_num)*sizeof(struct cn_notify_req);
> > 
> > 	if (msg->len != size) {
> > 		printk(KERN_ERR "Wrong connector request size %u, must be == %u.\n", 
> > 				msg->len, size);
> > 		return;
> > 	}
> > 
> > 	if (ctl->len + sizeof(*ctl) != msg->len) {
> > 		printk(KERN_ERR "Wrong message: msg->len=%u must be equal to inner_len=%u [+%u].\n", 
> > 				msg->len, ctl->len, sizeof(*ctl));
> > 		return;
> > 	}
> > 
> > 	/*
> > 	 * Remove notification.
> > 	 */
> > 	if (ctl->group == 0) {
> > 		struct cn_ctl_entry *n;
> > 		
> > 		spin_lock_bh(&notify_lock);
> > 		list_for_each_entry_safe(ent, n, &notify_list, notify_entry) {
> > 			if (cn_ctl_msg_equals(ent->msg, ctl)) {
> > 				list_del(&ent->notify_entry);
> > 				kfree(ent);
> > 			}
> > 		}
> > 		spin_unlock_bh(&notify_lock);
> > 
> > 		return;
> > 	}
> > 
> > 	size += sizeof(*ent);
> > 
> > 	ent = kmalloc(size, GFP_ATOMIC);
> 
> Another GFP_ATOMIC :(  In what context can this function be called?

It is called from BH context.
It is only one place that can not be moved to the different GFP
allocation,
but it is notification area, that is quite rarely used as fas as I can
see...

> > 	{
> > 		int i;
> > 		struct cn_notify_req *req;
> 
> May as well move these up to the top of the function, save the ugly indent.
> 
> > 		printk("Notify group %x for idx: ", ctl->group);
> > 
> > 		req = (struct cn_notify_req *)ctl->data;
> > 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {
> > 			printk("%u-%u ", req->first, req->first+req->range-1);
> > 		}
> 
> Unneeded braces.
> 
> This file is full of trailing whitespace, btw.  Please fix that up, then
> find a new editor.
> 
> > 		printk("\nNotify group %x for val: ", ctl->group);
> > 
> > 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> > 			printk("%u-%u ", req->first, req->first+req->range-1);
> > 		}
> 
> Braces.

It is debug code and I will remove it.

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: connector.c

[Andrew Morton]

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.

> > > 	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().

> > 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.

> > 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.  

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. 

Re: connector.c

[Evgeniy Polyakov]

[-- Attachment #1: Type: text/plain, Size: 3486 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: connector.c

[Andrew Morton]

Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
>
> 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.

Back to my original question.  If the kernel expects a reply from userspace
to a particular message, and that reply never comes, what happens?

Re: connector.c

[Evgeniy Polyakov]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

On Fri, 2005-04-01 at 00:02 -0800, Andrew Morton wrote:
> Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> >
> > 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.
> 
> Back to my original question.  If the kernel expects a reply from userspace
> to a particular message, and that reply never comes, what happens?

Nothing.
If reply message will be recived, it will be delivered to the requested 
connector user, if reply will not be received just nothing happens.

Not connector, but it's users who may expect reply to theirs messages.

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]