Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes
On Thu, Dec 6, 2018 at 9:10 PM David Miller wrote: > > From: Jouke Witteveen > Date: Thu, 6 Dec 2018 09:59:20 +0100 > > > On Thu, Dec 6, 2018 at 1:34 AM David Miller wrote: > >> > >> From: Jouke Witteveen > >> 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
Re: [PATCH 1/2] net: linkwatch: send change uevent on link changes
From: Jouke Witteveen Date: Thu, 6 Dec 2018 09:59:20 +0100 > On Thu, Dec 6, 2018 at 1:34 AM David Miller wrote: >> >> From: Jouke Witteveen >> 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
On Thu, Dec 6, 2018 at 1:34 AM David Miller wrote: > > From: Jouke Witteveen > 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
From: Jouke Witteveen 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
On Wed, Dec 5, 2018 at 8:45 PM David Miller wrote: > > From: Jouke Witteveen > 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
From: Jouke Witteveen 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
On Wed, Dec 5, 2018 at 5:50 AM David Miller wrote: > > From: Jouke Witteveen > 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 > > --- > > 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
From: Jouke Witteveen 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 > --- > 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
On Sat, 1 Dec 2018 17:00:21 +0100 Jouke Witteveen 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 Makes sense. I am also working on tracepoints for netdev changes. Reviewed-by: Stephen Hemminger
[PATCH 1/2] net: linkwatch: send change uevent on link changes
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 --- 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.kobj, KOBJ_CHANGE); } dev_put(dev); } -- 2.19.2
