Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 11:15:04PM +0100, Dimitry Andric wrote: D> > S> > Why don't just declare the buffer as: D> > S> > D> > S> > struct if_msghdr buf; D> > S> > D> > S> > and then do: D> > S> > D> > S> > nread = read(s, &buf, sizeof buf); D> > S> > D> > S> > ? You are never reading more than one if_msghdr anyway, and then there D> > S> > is no need to cast anything. D> > S> D> > S> My inspiration: route socket can return other messages (man 4 route) D> > D> > Yes, exactly. We don't know what size next datagram is going to be. D> D> Oh, in that case this code seems completely wrong. How do you know the D> full datagram will be delivered with one read() call? D> D> If it always is, then there is no need to read more than the size of D> struct if_msghdr, since you are not using any data beyond it. So in D> that case, you can suffice with read(..., sizeof(if_msghdr)). D> D> If the read() call will return partial results, you must repeatedly call D> it in a loop, until you either reach EOF, or have enough data. In that D> case, a buffer with the size of if_msghdr is also enough, since you D> never need to read beyond that. Sorry for delayed answer. The routing socket is a datagram socket, it isn't like TCP, it can't deliver partial datagrams. If we don't supply enough space for datagram that arrived, it won't be delivered. So the right solution is suppling plenty of space, but parse only part we are interested in. -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On 5 Mar 2020, at 22:01, Gleb Smirnoff wrote: > > On Thu, Mar 05, 2020 at 09:30:41PM +0300, Slawa Olhovchenkov wrote: > S> > > On Thu, Mar 05, 2020 at 08:35:15PM +0300, Slawa Olhovchenkov wrote: > S> > > S> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' > to 'struct > S> > > S> > > D> if_msghdr *' increases required alignment from 1 to 4 > S> > > S> > > D> [-Werror,-Wcast-align] > S> > > S> > > D> ifm = (struct if_msghdr *)buf; > S> > > S> > > D> ^~~ > S> > > S> > > D> 1 error generated. > S> > > S> > > D> > S> > > S> > > D> In practice I don't think the buffer can ever get > misaligned, so can you > S> > > S> > > D> please add a NO_WCAST_ALIGN= to the Makefile? > S> > > S> > > > S> > > S> > > route(8) handles the same problem via intermediate (void *) > cast. What is > S> > > S> > > preferred way to solve the problem? Change compiler flags file > wide, or > S> > > S> > > just through (void *) cast? > S> > > S> > > S> > > S> > Copy to aligned buffer or got SIGBUS on some architectures? > S> > > S> > S> > > S> char buf[2048] __aligned(__alignof(struct if_msghdr)); > S> > > S> > S> > > S> resolve this watning. > S> > > > S> > > Thanks, Slawa! I think this is the most elegant solution. > S> > > S> > Why don't just declare the buffer as: > S> > > S> > struct if_msghdr buf; > S> > > S> > and then do: > S> > > S> > nread = read(s, &buf, sizeof buf); > S> > > S> > ? You are never reading more than one if_msghdr anyway, and then there > S> > is no need to cast anything. > S> > S> My inspiration: route socket can return other messages (man 4 route) > > Yes, exactly. We don't know what size next datagram is going to be. Oh, in that case this code seems completely wrong. How do you know the full datagram will be delivered with one read() call? If it always is, then there is no need to read more than the size of struct if_msghdr, since you are not using any data beyond it. So in that case, you can suffice with read(..., sizeof(if_msghdr)). If the read() call will return partial results, you must repeatedly call it in a loop, until you either reach EOF, or have enough data. In that case, a buffer with the size of if_msghdr is also enough, since you never need to read beyond that. -Dimitry signature.asc Description: Message signed with OpenPGP
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 09:30:41PM +0300, Slawa Olhovchenkov wrote: S> > > On Thu, Mar 05, 2020 at 08:35:15PM +0300, Slawa Olhovchenkov wrote: S> > > S> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct S> > > S> > > D> if_msghdr *' increases required alignment from 1 to 4 S> > > S> > > D> [-Werror,-Wcast-align] S> > > S> > > D> ifm = (struct if_msghdr *)buf; S> > > S> > > D> ^~~ S> > > S> > > D> 1 error generated. S> > > S> > > D> S> > > S> > > D> In practice I don't think the buffer can ever get misaligned, so can you S> > > S> > > D> please add a NO_WCAST_ALIGN= to the Makefile? S> > > S> > > S> > > S> > > route(8) handles the same problem via intermediate (void *) cast. What is S> > > S> > > preferred way to solve the problem? Change compiler flags file wide, or S> > > S> > > just through (void *) cast? S> > > S> > S> > > S> > Copy to aligned buffer or got SIGBUS on some architectures? S> > > S> S> > > S> char buf[2048] __aligned(__alignof(struct if_msghdr)); S> > > S> S> > > S> resolve this watning. S> > > S> > > Thanks, Slawa! I think this is the most elegant solution. S> > S> > Why don't just declare the buffer as: S> > S> > struct if_msghdr buf; S> > S> > and then do: S> > S> > nread = read(s, &buf, sizeof buf); S> > S> > ? You are never reading more than one if_msghdr anyway, and then there S> > is no need to cast anything. S> S> My inspiration: route socket can return other messages (man 4 route) Yes, exactly. We don't know what size next datagram is going to be. -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 07:19:59PM +0100, Dimitry Andric wrote: > On 5 Mar 2020, at 18:44, Gleb Smirnoff wrote: > > > > On Thu, Mar 05, 2020 at 08:35:15PM +0300, Slawa Olhovchenkov wrote: > > S> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to > > 'struct > > S> > > D> if_msghdr *' increases required alignment from 1 to 4 > > S> > > D> [-Werror,-Wcast-align] > > S> > > D> ifm = (struct if_msghdr *)buf; > > S> > > D> ^~~ > > S> > > D> 1 error generated. > > S> > > D> > > S> > > D> In practice I don't think the buffer can ever get misaligned, so > > can you > > S> > > D> please add a NO_WCAST_ALIGN= to the Makefile? > > S> > > > > S> > > route(8) handles the same problem via intermediate (void *) cast. > > What is > > S> > > preferred way to solve the problem? Change compiler flags file wide, > > or > > S> > > just through (void *) cast? > > S> > > > S> > Copy to aligned buffer or got SIGBUS on some architectures? > > S> > > S> char buf[2048] __aligned(__alignof(struct if_msghdr)); > > S> > > S> resolve this watning. > > > > Thanks, Slawa! I think this is the most elegant solution. > > Why don't just declare the buffer as: > > struct if_msghdr buf; > > and then do: > > nread = read(s, &buf, sizeof buf); > > ? You are never reading more than one if_msghdr anyway, and then there > is no need to cast anything. My inspiration: route socket can return other messages (man 4 route) ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On 5 Mar 2020, at 18:44, Gleb Smirnoff wrote: > > On Thu, Mar 05, 2020 at 08:35:15PM +0300, Slawa Olhovchenkov wrote: > S> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to > 'struct > S> > > D> if_msghdr *' increases required alignment from 1 to 4 > S> > > D> [-Werror,-Wcast-align] > S> > > D> ifm = (struct if_msghdr *)buf; > S> > > D> ^~~ > S> > > D> 1 error generated. > S> > > D> > S> > > D> In practice I don't think the buffer can ever get misaligned, so > can you > S> > > D> please add a NO_WCAST_ALIGN= to the Makefile? > S> > > > S> > > route(8) handles the same problem via intermediate (void *) cast. What > is > S> > > preferred way to solve the problem? Change compiler flags file wide, or > S> > > just through (void *) cast? > S> > > S> > Copy to aligned buffer or got SIGBUS on some architectures? > S> > S> char buf[2048] __aligned(__alignof(struct if_msghdr)); > S> > S> resolve this watning. > > Thanks, Slawa! I think this is the most elegant solution. Why don't just declare the buffer as: struct if_msghdr buf; and then do: nread = read(s, &buf, sizeof buf); ? You are never reading more than one if_msghdr anyway, and then there is no need to cast anything. -Dimitry signature.asc Description: Message signed with OpenPGP
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 08:35:15PM +0300, Slawa Olhovchenkov wrote: S> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct S> > > D> if_msghdr *' increases required alignment from 1 to 4 S> > > D> [-Werror,-Wcast-align] S> > > D> ifm = (struct if_msghdr *)buf; S> > > D> ^~~ S> > > D> 1 error generated. S> > > D> S> > > D> In practice I don't think the buffer can ever get misaligned, so can you S> > > D> please add a NO_WCAST_ALIGN= to the Makefile? S> > > S> > > route(8) handles the same problem via intermediate (void *) cast. What is S> > > preferred way to solve the problem? Change compiler flags file wide, or S> > > just through (void *) cast? S> > S> > Copy to aligned buffer or got SIGBUS on some architectures? S> S> char buf[2048] __aligned(__alignof(struct if_msghdr)); S> S> resolve this watning. Thanks, Slawa! I think this is the most elegant solution. -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 08:24:54PM +0300, Slawa Olhovchenkov wrote: > On Thu, Mar 05, 2020 at 08:33:50AM -0800, Gleb Smirnoff wrote: > > > On Thu, Mar 05, 2020 at 03:29:23PM +0100, Dimitry Andric wrote: > > D> On 2020-03-04 23:27, Gleb Smirnoff wrote: > > D> > Author: glebius > > D> > Date: Wed Mar 4 22:27:16 2020 > > D> > New Revision: 358655 > > D> > URL: https://svnweb.freebsd.org/changeset/base/358655 > > D> > > > D> > Log: > > D> >When a machine boots the NFS mounting script is executed after > > D> >interfaces are configured, but for many interfaces (e.g. all Intel) > > D> >ifconfig causes link renegotiation, so the first attempt to mount > > D> >NFS always fails. After that mount_nfs sleeps for 30 seconds, while > > D> >only a couple seconds are actually required for interface to get up. > > D> >Instead of sleeping, do select(2) on routing socket and check if > > D> >some interface became UP and in this case retry immediately. > > D> > > D> At least on i386, this causes a -Werror warning: > > D> > > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct > > D> if_msghdr *' increases required alignment from 1 to 4 > > D> [-Werror,-Wcast-align] > > D> ifm = (struct if_msghdr *)buf; > > D> ^~~ > > D> 1 error generated. > > D> > > D> In practice I don't think the buffer can ever get misaligned, so can you > > D> please add a NO_WCAST_ALIGN= to the Makefile? > > > > route(8) handles the same problem via intermediate (void *) cast. What is > > preferred way to solve the problem? Change compiler flags file wide, or > > just through (void *) cast? > > Copy to aligned buffer or got SIGBUS on some architectures? char buf[2048] __aligned(__alignof(struct if_msghdr)); resolve this watning. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 08:33:50AM -0800, Gleb Smirnoff wrote: > On Thu, Mar 05, 2020 at 03:29:23PM +0100, Dimitry Andric wrote: > D> On 2020-03-04 23:27, Gleb Smirnoff wrote: > D> > Author: glebius > D> > Date: Wed Mar 4 22:27:16 2020 > D> > New Revision: 358655 > D> > URL: https://svnweb.freebsd.org/changeset/base/358655 > D> > > D> > Log: > D> >When a machine boots the NFS mounting script is executed after > D> >interfaces are configured, but for many interfaces (e.g. all Intel) > D> >ifconfig causes link renegotiation, so the first attempt to mount > D> >NFS always fails. After that mount_nfs sleeps for 30 seconds, while > D> >only a couple seconds are actually required for interface to get up. > D> >Instead of sleeping, do select(2) on routing socket and check if > D> >some interface became UP and in this case retry immediately. > D> > D> At least on i386, this causes a -Werror warning: > D> > D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct > D> if_msghdr *' increases required alignment from 1 to 4 > D> [-Werror,-Wcast-align] > D> ifm = (struct if_msghdr *)buf; > D> ^~~ > D> 1 error generated. > D> > D> In practice I don't think the buffer can ever get misaligned, so can you > D> please add a NO_WCAST_ALIGN= to the Makefile? > > route(8) handles the same problem via intermediate (void *) cast. What is > preferred way to solve the problem? Change compiler flags file wide, or > just through (void *) cast? Copy to aligned buffer or got SIGBUS on some architectures? ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On Thu, Mar 05, 2020 at 03:29:23PM +0100, Dimitry Andric wrote: D> On 2020-03-04 23:27, Gleb Smirnoff wrote: D> > Author: glebius D> > Date: Wed Mar 4 22:27:16 2020 D> > New Revision: 358655 D> > URL: https://svnweb.freebsd.org/changeset/base/358655 D> > D> > Log: D> >When a machine boots the NFS mounting script is executed after D> >interfaces are configured, but for many interfaces (e.g. all Intel) D> >ifconfig causes link renegotiation, so the first attempt to mount D> >NFS always fails. After that mount_nfs sleeps for 30 seconds, while D> >only a couple seconds are actually required for interface to get up. D> >Instead of sleeping, do select(2) on routing socket and check if D> >some interface became UP and in this case retry immediately. D> D> At least on i386, this causes a -Werror warning: D> D> sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct D> if_msghdr *' increases required alignment from 1 to 4 D> [-Werror,-Wcast-align] D> ifm = (struct if_msghdr *)buf; D> ^~~ D> 1 error generated. D> D> In practice I don't think the buffer can ever get misaligned, so can you D> please add a NO_WCAST_ALIGN= to the Makefile? route(8) handles the same problem via intermediate (void *) cast. What is preferred way to solve the problem? Change compiler flags file wide, or just through (void *) cast? -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r358655 - head/sbin/mount_nfs
On 2020-03-04 23:27, Gleb Smirnoff wrote: Author: glebius Date: Wed Mar 4 22:27:16 2020 New Revision: 358655 URL: https://svnweb.freebsd.org/changeset/base/358655 Log: When a machine boots the NFS mounting script is executed after interfaces are configured, but for many interfaces (e.g. all Intel) ifconfig causes link renegotiation, so the first attempt to mount NFS always fails. After that mount_nfs sleeps for 30 seconds, while only a couple seconds are actually required for interface to get up. Instead of sleeping, do select(2) on routing socket and check if some interface became UP and in this case retry immediately. At least on i386, this causes a -Werror warning: sbin/mount_nfs/mount_nfs.c:549:10: error: cast from 'char *' to 'struct if_msghdr *' increases required alignment from 1 to 4 [-Werror,-Wcast-align] ifm = (struct if_msghdr *)buf; ^~~ 1 error generated. In practice I don't think the buffer can ever get misaligned, so can you please add a NO_WCAST_ALIGN= to the Makefile? -Dimitry ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r358655 - head/sbin/mount_nfs
Author: glebius
Date: Wed Mar 4 22:27:16 2020
New Revision: 358655
URL: https://svnweb.freebsd.org/changeset/base/358655
Log:
When a machine boots the NFS mounting script is executed after
interfaces are configured, but for many interfaces (e.g. all Intel)
ifconfig causes link renegotiation, so the first attempt to mount
NFS always fails. After that mount_nfs sleeps for 30 seconds, while
only a couple seconds are actually required for interface to get up.
Instead of sleeping, do select(2) on routing socket and check if
some interface became UP and in this case retry immediately.
Reviewed by: rmacklem
Differential Revision:https://reviews.freebsd.org/D23934
Modified:
head/sbin/mount_nfs/mount_nfs.c
Modified: head/sbin/mount_nfs/mount_nfs.c
==
--- head/sbin/mount_nfs/mount_nfs.c Wed Mar 4 22:23:24 2020
(r358654)
+++ head/sbin/mount_nfs/mount_nfs.c Wed Mar 4 22:27:16 2020
(r358655)
@@ -65,6 +65,8 @@ __FBSDID("$FreeBSD$");
#include
#include
+#include
+#include
#include
#include
@@ -505,6 +507,59 @@ sec_num_to_name(int flavor)
return (NULL);
}
+/*
+ * Wait for RTM_IFINFO message with interface that is IFF_UP and with
+ * link on, or until timeout expires. Returns seconds left.
+ */
+static time_t
+rtm_ifinfo_sleep(time_t sec)
+{
+ char buf[2048];
+ fd_set rfds;
+ struct timeval tv, start;
+ ssize_t nread;
+ int n, s;
+
+ s = socket(PF_ROUTE, SOCK_RAW, 0);
+ if (s < 0)
+ err(EX_OSERR, "socket");
+ (void)gettimeofday(&start, NULL);
+
+ for (tv.tv_sec = sec, tv.tv_usec = 0;
+ tv.tv_sec > 0;
+ (void)gettimeofday(&tv, NULL),
+ tv.tv_sec = sec - (tv.tv_sec - start.tv_sec)) {
+ FD_ZERO(&rfds);
+ FD_SET(s, &rfds);
+ n = select(s + 1, &rfds, NULL, NULL, &tv);
+ if (n == 0)
+ continue;
+ if (n == -1) {
+ if (errno == EINTR)
+ continue;
+ else
+ err(EX_SOFTWARE, "select");
+ }
+ nread = read(s, buf, 2048);
+ if (nread < 0)
+ err(EX_OSERR, "read");
+ if ((size_t)nread >= sizeof(struct if_msghdr)) {
+ struct if_msghdr *ifm;
+
+ ifm = (struct if_msghdr *)buf;
+ if (ifm->ifm_version == RTM_VERSION &&
+ ifm->ifm_type == RTM_IFINFO &&
+ (ifm->ifm_flags & IFF_UP) &&
+ ifm->ifm_data.ifi_link_state != LINK_STATE_DOWN)
+ break;
+ }
+ }
+
+ close(s);
+
+ return (tv.tv_sec);
+}
+
static int
getnfsargs(char *spec, struct iovec **iov, int *iovlen)
{
@@ -638,7 +693,12 @@ getnfsargs(char *spec, struct iovec **iov, int *iovlen
if (daemon(0, 0) != 0)
err(1, "daemon");
}
- sleep(60);
+ /*
+* If rtm_ifinfo_sleep() returns non-zero, don't count
+* that as a retry attempt.
+*/
+ if (rtm_ifinfo_sleep(60) && retrycnt != 0)
+ retrycnt++;
}
freeaddrinfo(ai_nfs);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
