Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Herbert Xu]

Please add netdev to the CC list since this discussion pertains to
the networking subsystem.

Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> 
> User should not know about low-level transport - 
> it is like socket layer -  write only data and do not care about
> how it will be delivered.

The delineation between transport and upper layer is fuzzy.
In one situation the protocol might be transport and in another
it could be above the transport.

So I don't buy this argument.
 
> In the previous versions netlink group was assigned as incremented
> counter, 
> that was not convenient, but now we have 2-way ID, which is better
> from users point of view - idx is supposed to be major id, val - 
> some subsystem of that set.

Actually netlink does let you bind to a specific ID.

Of course you may argue that a single u32 is not enough.  However,
nothing is stopping you from introducing netlink v2 that extends
this.

In fact this is my main gripe with your patch: Why aren't you
extending netlink instead of hacking something on top of the existing
netlink? If the extensions require breaking compatibility: Fine,
you just need to call it netlink v2.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

On Sun, 10 Apr 2005 19:52:54 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Please add netdev to the CC list since this discussion pertains to
> the networking subsystem.
> 
> Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> > 
> > User should not know about low-level transport - 
> > it is like socket layer -  write only data and do not care about
> > how it will be delivered.
> 
> The delineation between transport and upper layer is fuzzy.
> In one situation the protocol might be transport and in another
> it could be above the transport.
> 
> So I don't buy this argument.

Connector allows to hide transport layer completely - 
concider acrypto or superio - that subsystems do not know
about network, author of the temperature driver for superio
should not know how skb is allocated and processed, 
and acrypto is not network related system, so why
they should know about network API?
Why should they know what is skb, how it is allocated, 
why shared skb can not be used in netlink and so on?
Users of the connector are only cared about
destination ID and how to send/receive message.
And what to do if we do not want something like connector
to be used for userspace control - we need to create
each time new netlink socket unit, like kobject's one,
or netlink_ulog, we need to register it in netlink.h, 
where already there too many units.

> > In the previous versions netlink group was assigned as incremented
> > counter, 
> > that was not convenient, but now we have 2-way ID, which is better
> > from users point of view - idx is supposed to be major id, val - 
> > some subsystem of that set.
> 
> Actually netlink does let you bind to a specific ID.
> 
> Of course you may argue that a single u32 is not enough.  However,
> nothing is stopping you from introducing netlink v2 that extends
> this.
> 
> In fact this is my main gripe with your patch: Why aren't you
> extending netlink instead of hacking something on top of the existing
> netlink? If the extensions require breaking compatibility: Fine,
> you just need to call it netlink v2.

When connector was created in the middle of 2004, it's main aim
was allowing easy userspace control over netlink.
Creating it's own skb receive parser was acceptible for
one project, for two, but it is definitely not the solution
for general use. And also I think it is not so easy to include new
netlink family unit for some completely unrelated to network subsystem.

So I decided to create only one skb parser, and put all skb processing
in one place, so any user has to do only registration with it's 
own receive callback, and sending function.

Thus, transport layer was completely hidden from connector users,
there are no complex netlink macros there, no network API
and all complex related issues, no huge code duplication in each device,
which does not want to mess with 32/64 ioctl issues, but 
want easy userspace control.

Later connector was changed a bit to allow easy notification
ability and new bus was added.
Connector is the solution for d-bus related projects, since
all control is in one place, there are destination ID,
there is no complex API.

Concider the latest xfrm event changes - good that we already
have netlink socket there, but in each sending function 
[there are at least three new]
we have all those skb_alloc, skb_put, NLMSG_* and so on which are
absolutely identical.
According to names and structures itself, they are too close
to what connector is for, and how it is implemented, so we already
have several connectors in the tree, and do we really want 
to proceed with it all over the place?


> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


	Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Kay Sievers]

On Sun, 2005-04-10 at 14:32 +0400, Evgeniy Polyakov wrote:
> On Sun, 10 Apr 2005 19:52:54 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> > > 
> > > User should not know about low-level transport - 
> > > it is like socket layer -  write only data and do not care about
> > > how it will be delivered.
> > 
> > The delineation between transport and upper layer is fuzzy.
> > In one situation the protocol might be transport and in another
> > it could be above the transport.
> > 
> > So I don't buy this argument.
> 
> Connector allows to hide transport layer completely - 
> concider acrypto or superio - that subsystems do not know
> about network, author of the temperature driver for superio
> should not know how skb is allocated and processed, 
> and acrypto is not network related system, so why
> they should know about network API?
> Why should they know what is skb, how it is allocated, 
> why shared skb can not be used in netlink and so on?
> Users of the connector are only cared about
> destination ID and how to send/receive message.
> And what to do if we do not want something like connector
> to be used for userspace control - we need to create
> each time new netlink socket unit, like kobject's one,
> or netlink_ulog, we need to register it in netlink.h, 
> where already there too many units.
> 
> > > In the previous versions netlink group was assigned as incremented
> > > counter, 
> > > that was not convenient, but now we have 2-way ID, which is better
> > > from users point of view - idx is supposed to be major id, val - 
> > > some subsystem of that set.
> > 
> > Actually netlink does let you bind to a specific ID.
> > 
> > Of course you may argue that a single u32 is not enough.  However,
> > nothing is stopping you from introducing netlink v2 that extends
> > this.
> > 
> > In fact this is my main gripe with your patch: Why aren't you
> > extending netlink instead of hacking something on top of the existing
> > netlink? If the extensions require breaking compatibility: Fine,
> > you just need to call it netlink v2.
> 
> When connector was created in the middle of 2004, it's main aim
> was allowing easy userspace control over netlink.
> Creating it's own skb receive parser was acceptible for
> one project, for two, but it is definitely not the solution
> for general use. And also I think it is not so easy to include new
> netlink family unit for some completely unrelated to network subsystem.
> 
> So I decided to create only one skb parser, and put all skb processing
> in one place, so any user has to do only registration with it's 
> own receive callback, and sending function.

Sure, but that would apply the a generic netlink extension too, right?

> Thus, transport layer was completely hidden from connector users,
> there are no complex netlink macros there, no network API
> and all complex related issues, no huge code duplication in each device,
> which does not want to mess with 32/64 ioctl issues, but 
> want easy userspace control.

I don't see the need for unimplemented abstractions here. You can
replace a generic netlink-use just the same way as you can replace the
connector's own netlink code below the connector API. There is not much
difference.

> Later connector was changed a bit to allow easy notification
> ability and new bus was added.
> Connector is the solution for d-bus related projects, since
> all control is in one place, there are destination ID,
> there is no complex API.

Sorry, what does this have to do with DBUS?

> Concider the latest xfrm event changes - good that we already
> have netlink socket there, but in each sending function 
> [there are at least three new]
> we have all those skb_alloc, skb_put, NLMSG_* and so on which are
> absolutely identical.
> According to names and structures itself, they are too close
> to what connector is for, and how it is implemented, so we already
> have several connectors in the tree, and do we really want 
> to proceed with it all over the place?

That's not the point. Nobody asks to replace the whole connector by raw
netlink. But the message subscription and multicasting could be part of
the generic netlink framework. The connector API would  still be on-top
if it and nothing changes besides that we would have a nice low-level
api to use for other systems too.
The basic idea behind the connector as a nice generic framework for
netlink, as it fills the missing multicast case, while we already can do
singlecast and broadcast with netlink.
It is just the same way the kernel plays with other core functionality
too. If something is not represented in the vfs-layer your are asked to
add it to the generic part and not implement it privately for your
filesystem only.

Thanks,
Kay

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

On Sun, 10 Apr 2005 13:08:44 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:

> On Sun, 2005-04-10 at 14:32 +0400, Evgeniy Polyakov wrote:
> > On Sun, 10 Apr 2005 19:52:54 +1000
> > Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> > > > 
> > > > User should not know about low-level transport - 
> > > > it is like socket layer -  write only data and do not care about
> > > > how it will be delivered.
> > > 
> > > The delineation between transport and upper layer is fuzzy.
> > > In one situation the protocol might be transport and in another
> > > it could be above the transport.
> > > 
> > > So I don't buy this argument.
> > 
> > Connector allows to hide transport layer completely - 
> > concider acrypto or superio - that subsystems do not know
> > about network, author of the temperature driver for superio
> > should not know how skb is allocated and processed, 
> > and acrypto is not network related system, so why
> > they should know about network API?
> > Why should they know what is skb, how it is allocated, 
> > why shared skb can not be used in netlink and so on?
> > Users of the connector are only cared about
> > destination ID and how to send/receive message.
> > And what to do if we do not want something like connector
> > to be used for userspace control - we need to create
> > each time new netlink socket unit, like kobject's one,
> > or netlink_ulog, we need to register it in netlink.h, 
> > where already there too many units.
> > 
> > > > In the previous versions netlink group was assigned as incremented
> > > > counter, 
> > > > that was not convenient, but now we have 2-way ID, which is better
> > > > from users point of view - idx is supposed to be major id, val - 
> > > > some subsystem of that set.
> > > 
> > > Actually netlink does let you bind to a specific ID.
> > > 
> > > Of course you may argue that a single u32 is not enough.  However,
> > > nothing is stopping you from introducing netlink v2 that extends
> > > this.
> > > 
> > > In fact this is my main gripe with your patch: Why aren't you
> > > extending netlink instead of hacking something on top of the existing
> > > netlink? If the extensions require breaking compatibility: Fine,
> > > you just need to call it netlink v2.
> > 
> > When connector was created in the middle of 2004, it's main aim
> > was allowing easy userspace control over netlink.
> > Creating it's own skb receive parser was acceptible for
> > one project, for two, but it is definitely not the solution
> > for general use. And also I think it is not so easy to include new
> > netlink family unit for some completely unrelated to network subsystem.
> > 
> > So I decided to create only one skb parser, and put all skb processing
> > in one place, so any user has to do only registration with it's 
> > own receive callback, and sending function.
> 
> Sure, but that would apply the a generic netlink extension too, right?

Sending is the only and not the most significant part of the connector.
Of course cn_netlink_send() can be split into prepare_send() and commit_send(),
where commit_send() will live in af_netlink.c and do 
allocation and so on.
But it will not change the fact, that kernel users still need
to allocate a netlink socket, register it's own family unit, and so on...

Direct netlink usage is like using raw socket or even tun/tap device for TCP/IP, 
noone use it in that way, although we can, but connector is like
using socket interface.

> > Thus, transport layer was completely hidden from connector users,
> > there are no complex netlink macros there, no network API
> > and all complex related issues, no huge code duplication in each device,
> > which does not want to mess with 32/64 ioctl issues, but 
> > want easy userspace control.
> 
> I don't see the need for unimplemented abstractions here. You can
> replace a generic netlink-use just the same way as you can replace the
> connector's own netlink code below the connector API. There is not much
> difference.

Kernel users do not want to implement it's own transport over netlink.
Socket allocation was changed in 2.5 - we could need to change
each driver that use it instead of changing only one place in connector.c
Abstration over netlink already implemented in connector and is used
in acrypto, superio and even kobject_uevent was changed to do it.

> > Later connector was changed a bit to allow easy notification
> > ability and new bus was added.
> > Connector is the solution for d-bus related projects, since
> > all control is in one place, there are destination ID,
> > there is no complex API.
> 
> Sorry, what does this have to do with DBUS?

Kernel now has only two ways to inform to userspace about it's things - 
kobject [uevent] and hotplug. There is inotify/dnotify too, but it is 
diferent in some way.

The second one is a huge monster that can not be used in embedded
systems, calling userspace process from inside the kernel is 
now very flexible way.
The first one has it's own socket, it's own protocol and infrastructure
based on strings.

What if we want to inform about some new event?
Should we use kobject_uevent mechanism? Not anything can be 
described as kobject strings.
Should we create new socket/proto/infratructure? We do not
have another way.

That is why connector is usefull here - only one set of 
methods, known proto, just change "val" in ID and 
drop it in the userspace if it is not updated to use
new events.


> > Concider the latest xfrm event changes - good that we already
> > have netlink socket there, but in each sending function 
> > [there are at least three new]
> > we have all those skb_alloc, skb_put, NLMSG_* and so on which are
> > absolutely identical.
> > According to names and structures itself, they are too close
> > to what connector is for, and how it is implemented, so we already
> > have several connectors in the tree, and do we really want 
> > to proceed with it all over the place?
> 
> That's not the point. Nobody asks to replace the whole connector by raw
> netlink. But the message subscription and multicasting could be part of
> the generic netlink framework. The connector API would  still be on-top
> if it and nothing changes besides that we would have a nice low-level
> api to use for other systems too.

Netlink already has it's groups - why doesn't it meet your needs?
If someone "subscribed" [bound to that group], so message
sent to it will be delivered, otherwise - not.

One thing may be added to raw netlink - ability to send
to the group but not in a "that" multicast way,
i.e. send message to all subscribed _exactly_ to that group,
so one bound to -1 group will not receive message to
0x123 group when that group was specified in netlink header.

I can prepare a patch it is something like following:
--- ./net/netlink/af_netlink.c.orig     2005-04-10 15:46:48.000000000 +0400
+++ ./net/netlink/af_netlink.c  2005-04-10 15:47:04.000000000 +0400
@@ -747,7 +747,7 @@
        if (p->exclude_sk == sk)
                goto out;
 
-       if (nlk->pid == p->pid || !(nlk->groups & p->group))
+       if (nlk->pid == p->pid || (nlk->groups != p->group))
                goto out;
 
        if (p->failure) {

and requires new name...

> The basic idea behind the connector as a nice generic framework for
> netlink, as it fills the missing multicast case, while we already can do
> singlecast and broadcast with netlink.
> It is just the same way the kernel plays with other core functionality
> too. If something is not represented in the vfs-layer your are asked to
> add it to the generic part and not implement it privately for your
> filesystem only.

Exactly for that reason connector was created - so noone
needs to create it's private sockets/netlink API wrappers/skb handlers
and so on, just register callback and have a ->send() call.
 
> Thanks,
> Kay


	Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

On Sun, 10 Apr 2005 15:37:57 +0400
Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:

> The second one is a huge monster that can not be used in embedded
> systems, calling userspace process from inside the kernel is 
> now very flexible way.

is NOT very flexible way...


	Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Thomas Graf]

* Evgeniy Polyakov <20050410153757.104fe611@zanzibar.2ka.mipt.ru> 2005-04-10 15:37
> --- ./net/netlink/af_netlink.c.orig     2005-04-10 15:46:48.000000000 +0400
> +++ ./net/netlink/af_netlink.c  2005-04-10 15:47:04.000000000 +0400
> @@ -747,7 +747,7 @@
>         if (p->exclude_sk == sk)
>                 goto out;
>  
> -       if (nlk->pid == p->pid || !(nlk->groups & p->group))
> +       if (nlk->pid == p->pid || (nlk->groups != p->group))
>                 goto out;
>  
>         if (p->failure) {

Not valid, would break RTMGRP_*.

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

On Sun, 10 Apr 2005 14:10:05 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> * Evgeniy Polyakov <20050410153757.104fe611@zanzibar.2ka.mipt.ru> 2005-04-10 15:37
> > --- ./net/netlink/af_netlink.c.orig     2005-04-10 15:46:48.000000000 +0400
> > +++ ./net/netlink/af_netlink.c  2005-04-10 15:47:04.000000000 +0400
> > @@ -747,7 +747,7 @@
> >         if (p->exclude_sk == sk)
> >                 goto out;
> >  
> > -       if (nlk->pid == p->pid || !(nlk->groups & p->group))
> > +       if (nlk->pid == p->pid || (nlk->groups != p->group))
> >                 goto out;
> >  
> >         if (p->failure) {
> 
> Not valid, would break RTMGRP_*.

Yes, it will break too many existing application, 
it would be new API, like do_one_broadcast_direct().
If I understand Kay right, it is what he needs
for the new multicast.

	Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[jamal]

Evgeniy,

Please crosspost on netdev - you should know that by now;->

I actually disagreee with Herbert on this. Theres definetely good
need to have a more usable messaging system that rides on top of
netlink. It is not that netlink cant be extended (I actually think thats
a separate topic) - its just that its usability curve is too high.
Thats what the original motivation for konnector was. To make it easy
for joe dumbass. And i think if konnector sticks to just doing that it
will end up being valuable.

I do think (and ive said this before) that Evgeniy is pushing it
by going beyond this simple agenda/focus. Unfortunately, I actually dont
think he is listening _at all_ on this specific issue. 

Evgeniy, just stick to the original focus and if it is accepted and
understood then lets move on to adding new features. Otherwise the
result of you adding yet one more feature for CBUS or whatever is
clearly to question what the original motivation was. And i dont think
you are able to add any other points to justify the existence of any new
konnector feature other than describe the original goals. At least thats
what i saw reading this thread. 
Otherwise if you really dont know what you want yet lets just pull this
whole thing out IMO.

cheers,
jamal

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[James Morris]

On 10 Apr 2005, jamal wrote:

> Thats what the original motivation for konnector was. To make it easy
> for joe dumbass.

Who you really want writing kernel code :-)


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[jamal]

On Sun, 2005-04-10 at 10:56, James Morris wrote:
> On 10 Apr 2005, jamal wrote:
> 
> > Thats what the original motivation for konnector was. To make it easy
> > for joe dumbass.
> 
> Who you really want writing kernel code :-)

Ok, let me take that back then ;->
The value is in allowing people who are kernel hackers that are trying
to focus on an entirely different problem to not really know much about
the internals of the messaging subsystem (if you wanna call netlink
that). A good example will be the fork patches which were posted
recently.

cheers,
jamal

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Thomas Graf]

* jamal <1113143959.1089.316.camel@jzny.localdomain> 2005-04-10 10:39
> Please crosspost on netdev - you should know that by now;->
> 
> I actually disagreee with Herbert on this. Theres definetely good
> need to have a more usable messaging system that rides on top of
> netlink. It is not that netlink cant be extended (I actually think thats
> a separate topic)

I find it quite easy already but I guess a few macros would improve
it even more. The routing attribute macros could be made generic to
so can benefit from the advanages of TLVs.

Evgeniy, Sorry for not having time earlier to give your patch a
review. I'm not yet through completely and won't comment on the
overall architecture until I have understood it all.

diff -Nru /tmp/empty/cn_queue.c linux-2.6/drivers/connector/cn_queue.c
--- /tmp/empty/cn_queue.c       1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/connector/cn_queue.c      2004-09-24 00:01:00.000000000 
+int cn_queue_add_callback(struct cn_queue_dev *dev, struct cn_callback *cb)
+{
+       struct cn_callback_entry *cbq, *n, *__cbq;
+       int found = 0;
+
+       cbq = cn_queue_alloc_callback_entry(cb);
+       if (!cbq)
+               return -ENOMEM;
+
+       atomic_inc(&dev->refcnt);
+       cbq->pdev = dev;
+
+       spin_lock(&dev->queue_lock);
+       list_for_each_entry_safe(__cbq, n, &dev->queue_list, callback_entry) {

Why _safe? There is no way a entry can be removed here.

+               if (cn_cb_equal(&__cbq->cb->id, &cb->id)) {
+                       found = 1;
+                       break;
+               }
+       }
diff -Nru /tmp/empty/connector.c linux-2.6/drivers/connector/connector.c
--- /tmp/empty/connector.c      1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/connector/connector.c     2004-09-24 00:01:00.000000000 
+void cn_netlink_send(struct cn_msg *msg, u32 __groups)
+{
+       struct cn_callback_entry *n, *__cbq;
+       unsigned int size;
+       struct sk_buff *skb;
+       struct nlmsghdr *nlh;
+       struct cn_msg *data;
+       struct cn_dev *dev = &cdev;
+       u32 groups = 0;
+       int found = 0;
+
+       if (!__groups)
+       {
+               spin_lock(&dev->cbdev->queue_lock);
+               list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {

Same here

+                       if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
+                               found = 1;
+                               groups = __cbq->group;
+                       }
+               }
+               spin_unlock(&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);
+                       return;
+               }
+       }
+       else
+               groups = __groups;
+
+       size = NLMSG_SPACE(sizeof(*msg) + msg->len);
+
+       skb = alloc_skb(size, GFP_ATOMIC);
+       if (!skb) {
+               printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
+               return;
+       }
+
+       nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));

This is not correct, what happens is:
size = NLMSG_SPACE(sizeof(*msg) + msg->len);
 --> align(hdr)+align(data)
size - sizeof(*nlh)
 --> (align(hdr)-hdr)+align(data)
NLMSG_PUT pads again to get to the end of the data block (NLMSG_LENGTH)
 --> align(hdr)+(align(hdr)-hdr)+align(data)

At the moment align(hdr) == hdr since nlmsghdr is already aligned
but this might change and your code will break.

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

On Sun, Apr 10, 2005 at 09:27:27PM +0200, Thomas Graf (tgraf@suug.ch) wrote:
> * jamal <1113143959.1089.316.camel@jzny.localdomain> 2005-04-10 10:39
> > Please crosspost on netdev - you should know that by now;->
> > 
> > I actually disagreee with Herbert on this. Theres definetely good
> > need to have a more usable messaging system that rides on top of
> > netlink. It is not that netlink cant be extended (I actually think thats
> > a separate topic)
> 
> I find it quite easy already but I guess a few macros would improve
> it even more. The routing attribute macros could be made generic to
> so can benefit from the advanages of TLVs.
> 
> Evgeniy, Sorry for not having time earlier to give your patch a
> review. I'm not yet through completely and won't comment on the
> overall architecture until I have understood it all.
> 
> diff -Nru /tmp/empty/cn_queue.c linux-2.6/drivers/connector/cn_queue.c
> --- /tmp/empty/cn_queue.c       1970-01-01 03:00:00.000000000 +0300
> +++ linux-2.6/drivers/connector/cn_queue.c      2004-09-24 00:01:00.000000000 
> +int cn_queue_add_callback(struct cn_queue_dev *dev, struct cn_callback *cb)
> +{
> +       struct cn_callback_entry *cbq, *n, *__cbq;
> +       int found = 0;
> +
> +       cbq = cn_queue_alloc_callback_entry(cb);
> +       if (!cbq)
> +               return -ENOMEM;
> +
> +       atomic_inc(&dev->refcnt);
> +       cbq->pdev = dev;
> +
> +       spin_lock(&dev->queue_lock);
> +       list_for_each_entry_safe(__cbq, n, &dev->queue_list, callback_entry) {
> 
> Why _safe? There is no way a entry can be removed here.

No particular reason - it easier to copy-paste it from other line :)

> +               if (cn_cb_equal(&__cbq->cb->id, &cb->id)) {
> +                       found = 1;
> +                       break;
> +               }
> +       }
> diff -Nru /tmp/empty/connector.c linux-2.6/drivers/connector/connector.c
> --- /tmp/empty/connector.c      1970-01-01 03:00:00.000000000 +0300
> +++ linux-2.6/drivers/connector/connector.c     2004-09-24 00:01:00.000000000 
> +void cn_netlink_send(struct cn_msg *msg, u32 __groups)
> +{
> +       struct cn_callback_entry *n, *__cbq;
> +       unsigned int size;
> +       struct sk_buff *skb;
> +       struct nlmsghdr *nlh;
> +       struct cn_msg *data;
> +       struct cn_dev *dev = &cdev;
> +       u32 groups = 0;
> +       int found = 0;
> +
> +       if (!__groups)
> +       {
> +               spin_lock(&dev->cbdev->queue_lock);
> +               list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 
> Same here
> 
> +                       if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> +                               found = 1;
> +                               groups = __cbq->group;
> +                       }
> +               }
> +               spin_unlock(&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);
> +                       return;
> +               }
> +       }
> +       else
> +               groups = __groups;
> +
> +       size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> +
> +       skb = alloc_skb(size, GFP_ATOMIC);
> +       if (!skb) {
> +               printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> +               return;
> +       }
> +
> +       nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> 
> This is not correct, what happens is:
> size = NLMSG_SPACE(sizeof(*msg) + msg->len);
>  --> align(hdr)+align(data)
> size - sizeof(*nlh)
>  --> (align(hdr)-hdr)+align(data)
> NLMSG_PUT pads again to get to the end of the data block (NLMSG_LENGTH)
>  --> align(hdr)+(align(hdr)-hdr)+align(data)
> 
> At the moment align(hdr) == hdr since nlmsghdr is already aligned
> but this might change and your code will break.

As far as I remember, header is always supposed to be aligned properly
"by design", so it even could be nonaligned here.

-- 
	Evgeniy Polyakov ( s0mbre )

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Thomas Graf]

* Evgeniy Polyakov <20050411092228.A32699@2ka.mipt.ru> 2005-04-11 09:22
> On Sun, Apr 10, 2005 at 09:27:27PM +0200, Thomas Graf (tgraf@suug.ch) wrote:
> > +       size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> > +
> > +       skb = alloc_skb(size, GFP_ATOMIC);
> > +       if (!skb) {
> > +               printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> > +               return;
> > +       }
> > +
> > +       nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> > 
> > This is not correct, what happens is:
> > size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> >  --> align(hdr)+align(data)
> > size - sizeof(*nlh)
> >  --> (align(hdr)-hdr)+align(data)
> > NLMSG_PUT pads again to get to the end of the data block (NLMSG_LENGTH)
> >  --> align(hdr)+(align(hdr)-hdr)+align(data)
> > 
> > At the moment align(hdr) == hdr since nlmsghdr is already aligned
> > but this might change and your code will break.
> 
> As far as I remember, header is always supposed to be aligned properly
> "by design", so it even could be nonaligned here.

No, have a look at the macros:

#define NLMSG_LENGTH(len) ((len)+NLMSG_ALIGN(sizeof(struct nlmsghdr)))
#define NLMSG_SPACE(len) NLMSG_ALIGN(NLMSG_LENGTH(len))

NLMSG_LENGTH points to the end of the payload in the message, NLMSG_SPACE
represents the total size aligned properly for a possible next multipart
message.

It is unlikely that nlmsghdr will ever be unaligned but there can be no
reason to introduce code that can break with perfectly legal changes just
because of that.

Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov]

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

On Mon, 2005-04-11 at 12:45 +0200, Thomas Graf wrote:
> * Evgeniy Polyakov <20050411092228.A32699@2ka.mipt.ru> 2005-04-11 09:22
> > On Sun, Apr 10, 2005 at 09:27:27PM +0200, Thomas Graf (tgraf@suug.ch) wrote:
> > > +       size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> > > +
> > > +       skb = alloc_skb(size, GFP_ATOMIC);
> > > +       if (!skb) {
> > > +               printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> > > +               return;
> > > +       }
> > > +
> > > +       nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> > > 
> > > This is not correct, what happens is:
> > > size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> > >  --> align(hdr)+align(data)
> > > size - sizeof(*nlh)
> > >  --> (align(hdr)-hdr)+align(data)
> > > NLMSG_PUT pads again to get to the end of the data block (NLMSG_LENGTH)
> > >  --> align(hdr)+(align(hdr)-hdr)+align(data)
> > > 
> > > At the moment align(hdr) == hdr since nlmsghdr is already aligned
> > > but this might change and your code will break.
> > 
> > As far as I remember, header is always supposed to be aligned properly
> > "by design", so it even could be nonaligned here.
> 
> No, have a look at the macros:
> 
> #define NLMSG_LENGTH(len) ((len)+NLMSG_ALIGN(sizeof(struct nlmsghdr)))
> #define NLMSG_SPACE(len) NLMSG_ALIGN(NLMSG_LENGTH(len))
> 
> NLMSG_LENGTH points to the end of the payload in the message, NLMSG_SPACE
> represents the total size aligned properly for a possible next multipart
> message.
> 
> It is unlikely that nlmsghdr will ever be unaligned but there can be no
> reason to introduce code that can break with perfectly legal changes just
> because of that.

But NLMSG_ALIGN(sizeof(hdr)) is always equal to sizeof(hdr), that size
was select not accidentally.

I will change it, no problem, it is good from aesthetic point of view.

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