[PATCH 1/2] net: linkwatch: send change uevent on link changes

[PATCH 1/2] net: linkwatch: send change uevent on link changes

[Jouke Witteveen]

Make it easy for userspace to respond to acquisition/loss of carrier.
The uevent is picked up by udev and, on systems with systemd, the
device unit of the interface announces a configuration reload.

Signed-off-by: Jouke Witteveen <j.witteveen@gmail.com>
---
I did not want to change the commit message into a systemd-howto, but
subscribing to udev events can be done through a line like
    ReloadPropagatedFrom=sys-subsystem-net-devices-%i.device
in a systemd unit file.

 net/core/link_watch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 7f51efb2b..66aeb88f8 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -162,6 +162,8 @@ static void linkwatch_do_dev(struct net_device *dev)
 			dev_deactivate(dev);
 
 		netdev_state_change(dev);
+
+		kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
 	}
 	dev_put(dev);
 }
-- 
2.19.2

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[Stephen Hemminger]

On Sat, 1 Dec 2018 17:00:21 +0100
Jouke Witteveen <j.witteveen@gmail.com> wrote:

> Make it easy for userspace to respond to acquisition/loss of carrier.
> The uevent is picked up by udev and, on systems with systemd, the
> device unit of the interface announces a configuration reload.
> 
> Signed-off-by: Jouke Witteveen <j.witteveen@gmail.com>

Makes sense. I am also working on tracepoints for netdev changes.

Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[David Miller]

From: Jouke Witteveen <j.witteveen@gmail.com>
Date: Sat, 1 Dec 2018 17:00:21 +0100

> Make it easy for userspace to respond to acquisition/loss of carrier.
> The uevent is picked up by udev and, on systems with systemd, the
> device unit of the interface announces a configuration reload.
> 
> Signed-off-by: Jouke Witteveen <j.witteveen@gmail.com>
> ---
> I did not want to change the commit message into a systemd-howto, but
> subscribing to udev events can be done through a line like
>     ReloadPropagatedFrom=sys-subsystem-net-devices-%i.device
> in a systemd unit file.

I want to hear more about "why".

If we have the rtnetlink message that can be listened for, userspace
ought to use that.  That's what it is there for.

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[Jouke Witteveen]

On Wed, Dec 5, 2018 at 5:50 AM David Miller <davem@davemloft.net> wrote:
>
> From: Jouke Witteveen <j.witteveen@gmail.com>
> Date: Sat, 1 Dec 2018 17:00:21 +0100
>
> > Make it easy for userspace to respond to acquisition/loss of carrier.
> > The uevent is picked up by udev and, on systems with systemd, the
> > device unit of the interface announces a configuration reload.
> >
> > Signed-off-by: Jouke Witteveen <j.witteveen@gmail.com>
> > ---
> > I did not want to change the commit message into a systemd-howto, but
> > subscribing to udev events can be done through a line like
> >     ReloadPropagatedFrom=sys-subsystem-net-devices-%i.device
> > in a systemd unit file.
>
> I want to hear more about "why".

Let me give a conceptual and a practical reason.

Conceptual.
>From the net-subsystem point of view, it may not be clear why we
should add uevents. However, from the uevent point of view, it is odd
that networking devices do not send out "change" events when their
medium (=link) state changes. For consistency, it would be better to
also send out uevents. I would go as far as to say that uevents should
have been used from the start, but they were not yet available back
then (I believe rtnetlink predates uevents by at least five years).
A good summary of this argument in favor of the patch is given by
Stephen Hemminger: it makes sense.

> If we have the rtnetlink message that can be listened for, userspace
> ought to use that.  That's what it is there for.

Practical.
If userspace refers to programs with fewer privileges than the kernel,
than, indeed, it is not hard to set up a listener for rtnetlink
messages in C. However, if it refers to system administration, then
uevents are far more convenient to listen to, since they are
propagated by udev and its likes.
For example, I maintain a network manager that delegates the actual
networking work to specialized programs. Basically, it is an
implementation of network manager logic in shell script. For such a
shell script, it is easy to respond to uevents (via udev, or
alternatives), but responding to rtnetlink messages would require a
separate program.

The way I see it is that rtnetlink and uevents represent two different
views of the system and link state changes should be visible in both.

Regards,
- Jouke

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[David Miller]

From: Jouke Witteveen <j.witteveen@gmail.com>
Date: Wed, 5 Dec 2018 14:50:31 +0100

> For example, I maintain a network manager that delegates the actual
> networking work to specialized programs.

Basically "I've implemented things using separate programs"

> Basically, it is an implementation of network manager logic in shell
> script. For such a shell script, it is easy to respond to uevents
> (via udev, or alternatives), but responding to rtnetlink messages
> would require a separate program.

And "In order to use rtnetlink I'll need a separate program!"

(╯°□°）╯︵ ┻━┻

So it's ok to use the separate program paradigm for dividing up
the tasks, but not for processing events?

I'm not convinced.

Either use the facility we have or extend it to fill a valid missing
need.

I'm not applying these patches, your logic doesn't add up and it's
inconsistent with our clear goals of not duplicating functionality.

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[Jouke Witteveen]

On Wed, Dec 5, 2018 at 8:45 PM David Miller <davem@davemloft.net> wrote:
>
> From: Jouke Witteveen <j.witteveen@gmail.com>
> Date: Wed, 5 Dec 2018 14:50:31 +0100
>
> > For example, I maintain a network manager that delegates the actual
> > networking work to specialized programs.
>
> Basically "I've implemented things using separate programs"

I was trying to give an example of a typical system administrator task.

> > Basically, it is an implementation of network manager logic in shell
> > script. For such a shell script, it is easy to respond to uevents
> > (via udev, or alternatives), but responding to rtnetlink messages
> > would require a separate program.
>
> And "In order to use rtnetlink I'll need a separate program!"
>
> (╯°□°）╯︵ ┻━┻
>
> So it's ok to use the separate program paradigm for dividing up
> the tasks, but not for processing events?
>
> I'm not convinced.

This is about automation. When you want to take some action in
response to an event, you want the event system to be accommodating.
In that sense, it is indeed more ok for actions to require separate
programs than for event listening.

> Either use the facility we have or extend it to fill a valid missing
> need.
>
> I'm not applying these patches, your logic doesn't add up and it's
> inconsistent with our clear goals of not duplicating functionality.

Can you elaborate a bit? I may not be aware of the policy you have in
mind. Furthermore, I fail to see how these "change" events in response
to link state changes are not to be expected. To me, it looks like
uevents are there precisely to inform userspace about this kind of
changes.
As I asked in my conceptual argument, please consider this patch an
extension of uevent coverage, more than an attack on rtnetlink (which
it is definitely not).

Regards,
- Jouke

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[David Miller]

From: Jouke Witteveen <j.witteveen@gmail.com>
Date: Wed, 5 Dec 2018 23:38:17 +0100

> Can you elaborate a bit? I may not be aware of the policy you have in
> mind.

When we have a user facing interface to do something, we don't create
another one unless it is absolutely, positively, unavoidable.

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[Jouke Witteveen]

On Thu, Dec 6, 2018 at 1:34 AM David Miller <davem@davemloft.net> wrote:
>
> From: Jouke Witteveen <j.witteveen@gmail.com>
> Date: Wed, 5 Dec 2018 23:38:17 +0100
>
> > Can you elaborate a bit? I may not be aware of the policy you have in
> > mind.
>
> When we have a user facing interface to do something, we don't create
> another one unless it is absolutely, positively, unavoidable.

Obviously, if I would have known this I would not have gone through
the trouble of investigating and proposing this patch. It was an
honest attempt at making the kernel better.
Where could I have found this policy? I have looked on kernel.org/doc,
but couldn't find it.

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[David Miller]

From: Jouke Witteveen <j.witteveen@gmail.com>
Date: Thu, 6 Dec 2018 09:59:20 +0100

> On Thu, Dec 6, 2018 at 1:34 AM David Miller <davem@davemloft.net> wrote:
>>
>> From: Jouke Witteveen <j.witteveen@gmail.com>
>> Date: Wed, 5 Dec 2018 23:38:17 +0100
>>
>> > Can you elaborate a bit? I may not be aware of the policy you have in
>> > mind.
>>
>> When we have a user facing interface to do something, we don't create
>> another one unless it is absolutely, positively, unavoidable.
> 
> Obviously, if I would have known this I would not have gone through
> the trouble of investigating and proposing this patch. It was an
> honest attempt at making the kernel better.
> Where could I have found this policy? I have looked on kernel.org/doc,
> but couldn't find it.

It is not formally documented but it is a concern we raise every time
a duplicate piece of user facing functionality is proposed.

Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes

[Jouke Witteveen]

On Thu, Dec 6, 2018 at 9:10 PM David Miller <davem@davemloft.net> wrote:
>
> From: Jouke Witteveen <j.witteveen@gmail.com>
> Date: Thu, 6 Dec 2018 09:59:20 +0100
>
> > On Thu, Dec 6, 2018 at 1:34 AM David Miller <davem@davemloft.net> wrote:
> >>
> >> From: Jouke Witteveen <j.witteveen@gmail.com>
> >> Date: Wed, 5 Dec 2018 23:38:17 +0100
> >>
> >> > Can you elaborate a bit? I may not be aware of the policy you have in
> >> > mind.
> >>
> >> When we have a user facing interface to do something, we don't create
> >> another one unless it is absolutely, positively, unavoidable.
> >
> > Obviously, if I would have known this I would not have gone through
> > the trouble of investigating and proposing this patch. It was an
> > honest attempt at making the kernel better.
> > Where could I have found this policy? I have looked on kernel.org/doc,
> > but couldn't find it.
>
> It is not formally documented but it is a concern we raise every time
> a duplicate piece of user facing functionality is proposed.

Ok, thanks for getting back to me! Now I know.

That said, when looking into udev I became more convinced that the
kernel should send uevents on link state changes anyway. An example of
another kernel interface that has two ways of sending out state
changes is rfkill. It informs userspace of state changes via
/dev/rfkill and via uevents. For the latter it also sets some
informative environment variables, which my patch currently does not
do.

What would be needed to get you (or anyone else) to reconsider this
patch (or a revision)? I can definitely see your point and am willing
to accept your rejection. However, I also think there are substantial
ergonomic benefits to a unified and consistent interface for device
state changes and would like to give it one more shot, if possible.

Thanks for your time,
- Jouke