Subject: Re: COPYIN/COPYOUT macro problems Re: IOCTL implementation and kernel/userland addresses
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
From: Reinoud Zandijk <reinoud@netbsd.org>
List: tech-kern
Date: 08/25/2005 21:09:44
--LwW0XdcUbUexiWVK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Aug 26, 2005 at 03:01:18AM +0900, YAMAMOTO Takashi wrote:
> > +#ifdef _KERNEL
> > +#define COPYIN(ioctlflags, uaddr, kaddr, len)\
> > +               if ((ioctlflags) & FKIOCTL) {\
> > +                       memcpy((kaddr), (uaddr), (len));\
> > +               } else {\
> > +                       copyin((uaddr), (kaddr), (len));\
> > +               };
> > +#define COPYOUT(ioctlflags, kaddr, uaddr, len)\
> > +               if ((ioctlflags) & FKIOCTL) {\
> > +                       memcpy((uaddr), (kaddr), (len));\
> > +               } else {\
> > +                       copyout((kaddr), (uaddr), (len));\
> > +               };
> > +#endif /* KERNEL */
> 
> - don't discard return values.
> - why memcpy rather than kcopy?
> - is there any benefits to use macros?  why not functions?
> - do you have any plan for non-vnode ioctls?

you are 100% right.... i didn't think of return values so functions might 
be handier yes oh and i choose memcpy since kcopy AFAIK is aliased to 
memcpy anyway but it might be changed one day yeah.

We could opt for a more generic kcopyin() function :

int kucopyin(int kernelspace, void *uaddr, void *kaddr, int len) {
	int retval;

	if (!kernelspace) {
		return copyin(uaddr, kaddr, len);
	};

	/*
	 * returning anything other than 0 is meaningless for it would have 
	 * panicked the kernel allready if the address was not valid.
	 */
	kcopy(uaddr, kaddr, len);
	return 0;
}

where the flag `kernelspace' being non-zero means use in-kernel transfers. 
This way checking for the FKIOCTL flag and/or using uio's uio_seg { 
UIO_USERSPACE, UIO_SYSSPACE } would be consistent.

I choose `kucopyin' for its either kernel or user addresses. kcopyin could 
also be used though copyin is allways kernel stuff anyway so kcopyin could 
confuse... 

would this be better?

Reinoud


--LwW0XdcUbUexiWVK
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (NetBSD)

iQEVAwUBQw4XcIKcNwBDyKpoAQJKKggAp761vhoBcidbBPlxPISgb+c5R0xxoSBT
MVUvhZVJaWGd86NZA0h7tLlDsTl2aUDVnX/X25bNu/ZykiH0u34Le2qRkeQBAZIV
X8Cyxsv5Zf4oT5BpKlaFJRjjUQ6lePUG3QzY/sudsgP/cwsN3ap0aVprtvKydUUt
xwKDxtB+7x0X5Cx0cEgHu4ushU3a3/jFBDsJMHJr1hGT4PBs74pRneXKByITtjkc
UeNVGW5E7m0oXVQk8HFNUwzdiIG6Q3B10lQ5zqLvLOq7TFsO1vVAvp5rBpGUPJOT
9aJsG6gjgCmuaK/it/lozZt7DL6HZ1m6IfmPrhil77345Mu11/JwRA==
=JMQ+
-----END PGP SIGNATURE-----

--LwW0XdcUbUexiWVK--