[PATCH] agetty: Reprompt once the network addresses change if address displayed

[PATCH] agetty: Reprompt once the network addresses change if address displayed

[Stef Walter]

Several of the /etc/issue escape codes such as \4 and \6 depend on
the current addresses of the system that can change after the agetty
prompt is displayed.  This can cause stale data to be displayed
when a user looks at a VT, especially in cases of DHCP racing with
system start up.

Similar to the --reload mechanism, if we're displaying an address
in the issue output, and the user hasn't typed anything yet: then
redisplay the prompt with the new address.

We use netlink to watch for address changes. We only open the netlink
socket if we display an address in the issue file.
---
 term-utils/agetty.c | 113 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 95 insertions(+), 18 deletions(-)

diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 122959f..fc36488 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -129,9 +129,12 @@
  */
 #ifdef AGETTY_RELOAD
 # include <sys/inotify.h>
+# include <linux/netlink.h>
+# include <linux/rtnetlink.h>
 # define AGETTY_RELOAD_FILENAME "/run/agetty.reload"	/* trigger file */
 # define AGETTY_RELOAD_FDNONE	-2			/* uninitialized fd */
 static int inotify_fd = AGETTY_RELOAD_FDNONE;
+static int netlink_fd = AGETTY_RELOAD_FDNONE;
 #endif
 
 /*
@@ -1516,6 +1519,68 @@ done:
 }
 
 #ifdef AGETTY_RELOAD
+static void open_netlink(void)
+{
+	struct sockaddr_nl addr = { 0, };
+	int sock;
+
+	if (netlink_fd != AGETTY_RELOAD_FDNONE)
+		return;
+
+	sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
+	if (sock >= 0) {
+		addr.nl_family = AF_NETLINK;
+		addr.nl_pid = getpid();
+		addr.nl_groups = RTMGRP_IPV4_IFADDR | RTMGRP_IPV6_IFADDR;
+		if (bind(sock,(struct sockaddr *)&addr, sizeof(addr)) < 0)
+			close (sock);
+		else
+			netlink_fd = sock;
+	}
+}
+
+static int process_netlink_msg(int *changed)
+{
+	char buf[4096];
+	struct sockaddr_nl snl;
+	struct iovec iov = { buf, sizeof buf };
+	struct msghdr msg = { (void*)&snl, sizeof (snl), &iov, 1, NULL, 0, 0 };
+	struct nlmsghdr *h;
+	int rc;
+
+	rc = recvmsg(netlink_fd, &msg, MSG_DONTWAIT);
+	if (rc < 0) {
+		if (errno == EWOULDBLOCK || errno == EAGAIN)
+			return 0;
+
+		/* Failure, just stop listening for changes */
+		close (netlink_fd);
+		netlink_fd = AGETTY_RELOAD_FDNONE;
+		return 0;
+	}
+
+	for (h = (struct nlmsghdr *)buf; NLMSG_OK(h, (unsigned int)rc); h = NLMSG_NEXT(h, rc)) {
+		if (h->nlmsg_type == NLMSG_DONE ||
+		    h->nlmsg_type == NLMSG_ERROR) {
+			close (netlink_fd);
+			netlink_fd = AGETTY_RELOAD_FDNONE;
+			return 0;
+		}
+
+		*changed = 1;
+		break;
+	}
+
+	return 1;
+}
+
+static int process_netlink(void)
+{
+	int changed = 0;
+	while (process_netlink_msg (&changed));
+	return changed;
+}
+
 static int wait_for_term_input(int fd)
 {
 	char buffer[sizeof(struct inotify_event) + NAME_MAX + 1];
@@ -1556,33 +1621,41 @@ static int wait_for_term_input(int fd)
 					AGETTY_RELOAD_FILENAME);
 	}
 
-	FD_ZERO(&rfds);
-	FD_SET(fd, &rfds);
+	for (;;) {
+		FD_ZERO(&rfds);
+		FD_SET(fd, &rfds);
 
-	if (inotify_fd >= 0)
-		FD_SET(inotify_fd, &rfds);
+		if (inotify_fd >= 0)
+			FD_SET(inotify_fd, &rfds);
+		if (netlink_fd >= 0)
+			FD_SET(netlink_fd, &rfds);
 
-	/* If waiting fails, just fall through, presumably reading input will fail */
-	if (select(max(fd, inotify_fd) + 1, &rfds, NULL, NULL, NULL) < 0)
-		return 1;
+		/* If waiting fails, just fall through, presumably reading input will fail */
+		if (select(max(fd, inotify_fd) + 1, &rfds, NULL, NULL, NULL) < 0)
+			return 1;
 
-	if (FD_ISSET(fd, &rfds)) {
-		count = read(fd, buffer, sizeof (buffer));
+		if (FD_ISSET(fd, &rfds)) {
+			count = read(fd, buffer, sizeof (buffer));
 
-		tcsetattr(fd, TCSANOW, &orig);
+			tcsetattr(fd, TCSANOW, &orig);
 
-		/* Reinject the bytes we read back into the buffer, usually just one byte */
-		for (i = 0; i < count; i++)
-			ioctl(fd, TIOCSTI, buffer + i);
+			/* Reinject the bytes we read back into the buffer, usually just one byte */
+			for (i = 0; i < count; i++)
+				ioctl(fd, TIOCSTI, buffer + i);
 
-		/* Have terminal input */
-		return 1;
+			/* Have terminal input */
+			return 1;
 
-	} else {
-		tcsetattr(fd, TCSANOW, &orig);
+		} else if (netlink_fd >= 0 && FD_ISSET (netlink_fd, &rfds)) {
+			if (!process_netlink())
+				continue;
 
 		/* Just drain the inotify buffer */
-		while (read(inotify_fd, buffer, sizeof (buffer)) > 0);
+		} else if (inotify_fd >= 0 && FD_ISSET (inotify_fd, &rfds)) {
+			while (read(inotify_fd, buffer, sizeof (buffer)) > 0);
+		}
+
+		tcsetattr(fd, TCSANOW, &orig);
 
 		/* Need to reprompt */
 		return 0;
@@ -2371,6 +2444,10 @@ static void output_special_char(unsigned char c, struct options *op,
 		struct ifaddrs *addrs = NULL;
 		char iface[128];
 
+#ifdef AGETTY_RELOAD
+		open_netlink();
+#endif
+
 		if (getifaddrs(&addrs))
 			break;
 
-- 
2.4.3

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Mike Frysinger]

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

On 03 Jul 2015 14:58, Stef Walter wrote:
  #ifdef AGETTY_RELOAD
>  # include <sys/inotify.h>
> +# include <linux/netlink.h>
> +# include <linux/rtnetlink.h>

why not use libmnl instead ?
	http://netfilter.org/projects/libmnl

> +static void open_netlink(void)
> +{
> ...
> +		if (bind(sock,(struct sockaddr *)&addr, sizeof(addr)) < 0)

need space after the first ,

> +			close (sock);

no space before the (

> +static int process_netlink_msg(int *changed)
> +{
> +	char buf[4096];
> +	struct sockaddr_nl snl;
> +	struct iovec iov = { buf, sizeof buf };
> +	struct msghdr msg = { (void*)&snl, sizeof (snl), &iov, 1, NULL, 0, 0 };

would be nice to use nmed initializers imo.  makes it easier to read, more 
portable, and less error prone.

	struct iovec iov = {
		.iov_base = ...,
		.iov_len = ...,
	};

also your use of parens w/sizeof is inconsistent.  should use them, and not put 
a space between them.

> +		close (netlink_fd);

no space before the (

> +			close (netlink_fd);

same here

> +	for (;;) {

while (1) is more common i think

> +		} else if (inotify_fd >= 0 && FD_ISSET (inotify_fd, &rfds)) {

no space before the (
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Bruce Dubbs]

Mike Frysinger wrote:
> On 03 Jul 2015 14:58, Stef Walter wrote:
>    #ifdef AGETTY_RELOAD
>>   # include <sys/inotify.h>
>> +# include <linux/netlink.h>
>> +# include <linux/rtnetlink.h>
>
> why not use libmnl instead ?
> 	http://netfilter.org/projects/libmnl

That would not work for the LinuxFromScratch project.  At the time 
util-linux is built, libmnl is not available.

   -- Bruce

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Mike Frysinger]

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

On 06 Jul 2015 13:58, Bruce Dubbs wrote:
> Mike Frysinger wrote:
> > On 03 Jul 2015 14:58, Stef Walter wrote:
> >    #ifdef AGETTY_RELOAD
> >>   # include <sys/inotify.h>
> >> +# include <linux/netlink.h>
> >> +# include <linux/rtnetlink.h>
> >
> > why not use libmnl instead ?
> > 	http://netfilter.org/projects/libmnl
> 
> That would not work for the LinuxFromScratch project.  At the time 
> util-linux is built, libmnl is not available.

then update the LFS insns to include libmnl, or build util-linux with the
reload option disabled.  i don't see how LFS's decisions are relevant here.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Bruce Dubbs]

Mike Frysinger wrote:
> On 06 Jul 2015 13:58, Bruce Dubbs wrote:
>> Mike Frysinger wrote:
>>> On 03 Jul 2015 14:58, Stef Walter wrote:
>>>     #ifdef AGETTY_RELOAD
>>>>    # include <sys/inotify.h>
>>>> +# include <linux/netlink.h>
>>>> +# include <linux/rtnetlink.h>
>>>
>>> why not use libmnl instead ?
>>> 	http://netfilter.org/projects/libmnl
>>
>> That would not work for the LinuxFromScratch project.  At the time
>> util-linux is built, libmnl is not available.
>
> then update the LFS insns to include libmnl, or build util-linux with the
> reload option disabled.  i don't see how LFS's decisions are relevant here.
> -mike

Yes, we can do either, but it seems like a lot of overhead to add a new 
package for 95 lines of code that are already written.

We already manage about 1000 packages and are volunteers. Many of my 
users would call adding a new library for one minor function bloat.
None of the packages we support now use that library although that could 
change in the future if others start to use it.

Yes, our decisions are only a minor input to upstream packages, but 
there may be others that have similar concerns that don't monitor this list.

   -- Bruce

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Mike Frysinger]

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

On 07 Jul 2015 00:05, Bruce Dubbs wrote:
> Mike Frysinger wrote:
> > On 06 Jul 2015 13:58, Bruce Dubbs wrote:
> >> Mike Frysinger wrote:
> >>> On 03 Jul 2015 14:58, Stef Walter wrote:
> >>>     #ifdef AGETTY_RELOAD
> >>>>    # include <sys/inotify.h>
> >>>> +# include <linux/netlink.h>
> >>>> +# include <linux/rtnetlink.h>
> >>>
> >>> why not use libmnl instead ?
> >>> 	http://netfilter.org/projects/libmnl
> >>
> >> That would not work for the LinuxFromScratch project.  At the time
> >> util-linux is built, libmnl is not available.
> >
> > then update the LFS insns to include libmnl, or build util-linux with the
> > reload option disabled.  i don't see how LFS's decisions are relevant here.
> 
> Yes, we can do either, but it seems like a lot of overhead to add a new 
> package for 95 lines of code that are already written.
> 
> We already manage about 1000 packages and are volunteers. Many of my 
> users would call adding a new library for one minor function bloat.
> None of the packages we support now use that library although that could 
> change in the future if others start to use it.
> 
> Yes, our decisions are only a minor input to upstream packages, but 
> there may be others that have similar concerns that don't monitor this list.

the whole point of libmnl is to provide a clean userspace library API that is 
completely standalone & minimal (it's in the name) so people don't have to 
learn the low level netlink APIs nor have to include linux/ headers directly 
(which often lead to clashes with the C libraries).  libmnl itself is <20KiB, 
so i don't buy the bloat argument.

even then, if we do use libmnl in util-linux, it'd have configure checks so that 
if the system doesn't have it, it'd be disabled automatically.  i'm surprised 
you're complaining about libmnl but not the other large external libs that 
util-linux utilizes.

another point of information: the latest iproute2 now requires libmnl, so that 
ship has already sailed.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Bruce Dubbs]

Mike Frysinger wrote:

> the whole point of libmnl is to provide a clean userspace library API that is
> completely standalone & minimal (it's in the name) so people don't have to
> learn the low level netlink APIs nor have to include linux/ headers directly
> (which often lead to clashes with the C libraries).  libmnl itself is <20KiB,
> so i don't buy the bloat argument.
>
> even then, if we do use libmnl in util-linux, it'd have configure checks so that
> if the system doesn't have it, it'd be disabled automatically.  i'm surprised
> you're complaining about libmnl but not the other large external libs that
> util-linux utilizes.
>
> another point of information: the latest iproute2 now requires libmnl, so that
> ship has already sailed.

Yes, you're right.  We do disable tipc in iproute2 for the same reason 
as we are discussing here.  That's not a function that many of our users 
would need.  If libmnl is added to util-linux, then we'll deal with it.

   -- Bruce

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Mike Frysinger]

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

On 07 Jul 2015 09:56, Bruce Dubbs wrote:
> Mike Frysinger wrote:
> > the whole point of libmnl is to provide a clean userspace library API that is
> > completely standalone & minimal (it's in the name) so people don't have to
> > learn the low level netlink APIs nor have to include linux/ headers directly
> > (which often lead to clashes with the C libraries).  libmnl itself is <20KiB,
> > so i don't buy the bloat argument.
> >
> > even then, if we do use libmnl in util-linux, it'd have configure checks so that
> > if the system doesn't have it, it'd be disabled automatically.  i'm surprised
> > you're complaining about libmnl but not the other large external libs that
> > util-linux utilizes.
> >
> > another point of information: the latest iproute2 now requires libmnl, so that
> > ship has already sailed.
> 
> Yes, you're right.  We do disable tipc in iproute2 for the same reason 
> as we are discussing here.  That's not a function that many of our users 
> would need.  If libmnl is added to util-linux, then we'll deal with it.

at some point i think it's going to be inevitable.  my (bystander) view is that 
they'll be moving to use it more.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Stef Walter]

On 06.07.2015 16:07, Mike Frysinger wrote:
> On 03 Jul 2015 14:58, Stef Walter wrote:
>   #ifdef AGETTY_RELOAD
>>  # include <sys/inotify.h>
>> +# include <linux/netlink.h>
>> +# include <linux/rtnetlink.h>
> 
> why not use libmnl instead ?
> 	http://netfilter.org/projects/libmnl

I'm only a sporadic contributor to util-linux. I felt that adding a
dependency to util-linux wasn't my place to suggest. In addition this
patch performs very very simple usage of the netlink socket. We don't
even look at message data.

However the mantainers feel this is a better approach, I'd be happy to
implement it this way. Are you a maintainer of util-linux (sorry for the
dumb question)?

>> +static void open_netlink(void)
>> +{
>> ...
>> +		if (bind(sock,(struct sockaddr *)&addr, sizeof(addr)) < 0)
> 
> need space after the first ,

<snip>

Fixed all of these other review items and pushed the patch here:

https://github.com/stefwalter/util-linux/commits/agetty-addrchange

I'd be happy to repost the patch to the mailing list once a maintainer
decides on whether libmnl is necessary or not.

Stef

Re: [PATCH] agetty: Reprompt once the network addresses change if address displayed

[Karel Zak]

On Thu, Jul 09, 2015 at 09:20:09AM +0200, Stef Walter wrote:
> I'm only a sporadic contributor to util-linux. I felt that adding a
> dependency to util-linux wasn't my place to suggest. In addition this
> patch performs very very simple usage of the netlink socket. We don't
> even look at message data.

IMHO it's fine.

> Fixed all of these other review items and pushed the patch here:
> 
> https://github.com/stefwalter/util-linux/commits/agetty-addrchange

 Applied, thanks!

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com