Date: Mon, 8 Jun 2009 12:14:42 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Hiroki Sato <hrs@FreeBSD.org> Cc: amd64@FreeBSD.org, current@FreeBSD.org Subject: Re: [head tinderbox] failure on amd64/amd64 Message-ID: <yU9M2qm9rH%2BoeIVIgSzCrCakobk@j4OYE6OL8eALCd4BvSxIfwgoxSc> In-Reply-To: <20090608.165325.225640915.hrs@allbsd.org> References: <20090608025715.499087302F@freebsd-current.sentex.ca> <8LPG99US2/4EsGlonyfMSkDb40o@XX1fo6zQUfC4h0jjRC6IBz3oNH4> <sqWp8ZLqcJhvIOZAWr3MfqIXJ9k@XX1fo6zQUfC4h0jjRC6IBz3oNH4> <20090608.165325.225640915.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hiroki, good day.
Mon, Jun 08, 2009 at 04:53:25PM +0900, Hiroki Sato wrote:
> Gr, certainly this looks strange. I meant the attached patch.
> Thanks for pointing out it.
No problems.
But still:
> Index: if_gif.c
> ===================================================================
> --- if_gif.c (revision 193673)
> +++ if_gif.c (working copy)
> @@ -914,10 +914,10 @@
> case GIFSOPTS:
> if ((error = priv_check(curthread, PRIV_NET_GIF)) != 0)
> break;
> - if ((error = copyin(&options, &sc->gif_options,
> - sizeof(sc->gif_options)))) {
> + if ((error = copyin(ifr->ifr_data, &options,
> + sizeof(options)))) {
> if ((options | GIF_FULLOPTS) == GIF_FULLOPTS)
> - ifr->ifr_data = (caddr_t)options;
> + sc->gif_options = options;
> else
> error = EINVAL;
> }
Do you intend to set sc->gif_options only for the case of failed
copyin()? This looks a bit strange to me too, because
1. in this case 'options' will have undeterminate contents;
2. I thought that 'set options' should set options if it is
permitted.
Though there could be some logics behing this -- don't know, but
may be the negation operator was lost before '(error = copyin(...))' --
this is most adequate description of check for GIF_FULLOPTS.
By the way, it will be great if new sysctls and their options will be
documented somewhere, perhaps in the gif(4) itself.
--
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?yU9M2qm9rH%2BoeIVIgSzCrCakobk>