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...

[Jilles Tjoelker]

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