[systemd-devel] The enum udev_monitor_netlink_group contains bit masks

[Kay Sievers]

On Wed, Mar 13, 2013 at 6:09 AM, Andrey Wagin <avagin at gmail.com> wrote:
> 2013/3/13 Kay Sievers <kay at vrfy.org>
>>
>> On Tue, Mar 12, 2013 at 9:35 PM, Andrey Wagin <avagin at gmail.com> wrote:
>> > According to netlink(7) nl_groups is a bit mask with every bit
>> > representing
>> > a netlink group number.
>>
>> Netlink uses "numbers" not "a mask" since many years, we have just a
>> 32bit number, not 32 groups today. The man page should really be
>> updated:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d629b836d151d43332492651dd841d32e57ebe3b
>>
>> There might be magic for group 1-32, which is still in place, but
>> there is generally no mask anymore for netlink groups.
>
> I'm afraid, you are wrong. This patch is about kernel internals.
> User-interfaces have not been changed.

They surely did, I had code that time which was affected. We used to
send messages to multiple groups at the same time.

> E.g.
> netlink_connect(...)
> {
> ...
>  nlk->dst_group  = ffs(nladdr->nl_groups);
> ...
> }

Which is the "magic" I mentioned above. Only the first bit is found.
We would use 4 as the next entry in the enum, and end up in group 3.

> Here is another patch, which expands user interface to set more than 32
> groups.
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=9a4595bc7e67962f13232ee55a64e063062c3a99

Right, which is the access to the "number" instead of using the
"mask", which isn't a mask anymore, but just a plain number from 1 to
32 encoded as one bit in a 32 bit value.

>> > I found that constants from udev_monitor_netlink_group are set directly
>> > to
>> > nl_groups. It's dangerous. Currently this enum contains only three
>> > constant
>> > and all is ok, but the next constant will be incorrect.
>>
>> There will probably never be another netlink group inside libudev. If
>> we ever change that low-level stuff here, we would move entirely to a
>> kernel dbus facility, and not (mis-)use netlink anymore.
>
> It's not a reason to not fix this code. People use code of system tools as
> examples.

Nobody will get new raw netlink socket numbers, it's all only generic
netlink today, and there are no masks at all in today's commonly
available api, just numbers.

> I usually do that. It's the most probable place where kernel
> interfaces are used by the right way.

1 and 2 are distinctive bits in an enum; they are correct, and there
will not be more of them added. The enum specifies UDEV_MONITOR_UDEV
== 2, your patch defines it as 1 << 1, which is a purely cosmetic
difference to express the very same number. Adding the next line as
UDEV_MONITOR_FOO=4 would just work fine.

If you like, send a patch with a proper changelog, which changes the
enum to directly carry the explicit assignments. That would *explain*
in a more obvious way what's going on here, but it's still the very
same behaviour, and nothing needs to be *fixed* here, or is not
correct at the moment.

Kay