Date: Sat, 29 Jun 2013 00:53:19 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Davide Italiano <davide@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r252356 - in head: contrib/smbfs/mount_smbfs etc/defaults etc/mtree include lib lib/libprocstat rescue/rescue sbin/mount share/examples share/examples/etc share/mk sys/conf sys/kern sys... Message-ID: <20130628225318.GA16971@stack.nl> In-Reply-To: <201306282100.r5SL08kx093999@svn.freebsd.org> References: <201306282100.r5SL08kx093999@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 28, 2013 at 09:00:08PM +0000, Davide Italiano wrote: > Author: davide > Date: Fri Jun 28 21:00:08 2013 > New Revision: 252356 > URL: http://svnweb.freebsd.org/changeset/base/252356 > Log: > - Trim an unused and bogus Makefile for mount_smbfs. > - Reconnect with some minor modifications, in particular now selsocket() > internals are adapted to use sbintime units after recent'ish calloutng > switch. > Modified: head/sys/kern/sys_generic.c > ============================================================================== > --- head/sys/kern/sys_generic.c Fri Jun 28 20:32:48 2013 (r252355) > +++ head/sys/kern/sys_generic.c Fri Jun 28 21:00:08 2013 (r252356) > @@ -1498,6 +1498,61 @@ sys_openbsd_poll(td, uap) > } > > /* > + * XXX This was created specifically to support netncp and netsmb. This > + * allows the caller to specify a socket to wait for events on. It returns > + * 0 if any events matched and an error otherwise. There is no way to > + * determine which events fired. > + */ > +int > +selsocket(struct socket *so, int events, struct timeval *tvp, struct thread *td) > +{ > + struct timeval rtv; > + sbintime_t asbt, precision, rsbt; > + int error; > + > + if (tvp != NULL) { > + rtv = *tvp; > + if (rtv.tv_sec < 0 || rtv.tv_usec < 0 || > + rtv.tv_usec >= 1000000) > + return (EINVAL); > + if (!timevalisset(&rtv)) > + asbt = 0; > + else if (rtv.tv_sec <= INT32_MAX) { > + rsbt = tvtosbt(rtv); > + precision = rsbt; > + precision >>= tc_precexp; > + if (TIMESEL(&asbt, rsbt)) > + asbt += tc_tick_sbt; > + if (asbt <= INT64_MAX - rsbt) > + asbt += rsbt; > + else > + asbt = -1; > + } else > + asbt = -1; > + } else > + asbt = -1; > + seltdinit(td); > + /* > + * Iterate until the timeout expires or the socket becomes ready. > + */ > + for (;;) { > + selfdalloc(td, NULL); > + error = sopoll(so, events, NULL, td); > + /* error here is actually the ready events. */ > + if (error) > + return (0); > + error = seltdwait(td, asbt, precision); > + if (error) > + break; > + } > + seltdclear(td); > + /* XXX Duplicates ncp/smb behavior. */ > + if (error == ERESTART) > + error = 0; This if looks like it may introduce unexpected differences based on whether SA_RESTART is set for an interrupting signal. When "ignoring" ERESTART or EINTR like this, there is a danger of busy-waiting because once a sleep with PCATCH has returned ERESTART or EINTR, all further sleeps with PCATCH will immediately return the same until you return to the user boundary so the signal can be handled. In quick inspection, the (single) caller looks like it can handle ERESTART like EINTR. The file sys/netsmb/smb_trantcp.c does appear to contain incorrect ERESTART/EINTR handling in nbssn_recv(), which can busy-wait for the rest of the data if a signal occurs. You could mask some signals temporarily if proper ERESTART/EINTR handling would be too hard or would confuse applications that do not expect partial success or [EINTR] on regular files. The NFS client does some of these things although it contains bugs as well (such as losing the distinction between ERESTART and EINTR). > + return (error); > +} > + > +/* > * Preallocate two selfds associated with 'cookie'. Some fo_poll routines > * have two select sets, one for read and another for write. > */ -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130628225318.GA16971>