[systemd-devel] The enum udev_monitor_netlink_group contains bit masks
kay at vrfy.org
Wed Mar 13 03:23:48 PDT 2013
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
>> 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.
> 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
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
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.
More information about the systemd-devel