Date: Fri, 2 May 2014 17:57:02 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <eadler@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Jung-uk Kim <jkim@freebsd.org> Subject: Re: svn commit: r265132 - in head: share/man/man4 sys/dev/null Message-ID: <20140502165438.D1112@besplex.bde.org> In-Reply-To: <CAF6rxg=3KU99V9ax0THVWJwjF1X6ZXkwZCRnbxYeWYiW4rbQ=Q@mail.gmail.com> References: <201404300620.s3U6Kmn6074492@svn.freebsd.org> <5361512A.7070205@FreeBSD.org> <53627D8C.3030404@FreeBSD.org> <CAF6rxg=3KU99V9ax0THVWJwjF1X6ZXkwZCRnbxYeWYiW4rbQ=Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 1 May 2014, Eitan Adler wrote: > On 1 May 2014 09:59, Jung-uk Kim <jkim@freebsd.org> wrote: >> On 2014-04-30 15:38:18 -0400, ?? wrote: >>> On 2014-04-30 02:20:48 -0400, ?? wrote: Mail clients keep getting worse. Fitst the apostrophes were corrupted, now the names... >>>> Author: eadler Date: Wed Apr 30 06:20:48 2014 New Revision: >>>> 265132 URL: http://svnweb.freebsd.org/changeset/base/265132 >>>> >>>> Log: Add a /dev/full device. >>>> >>>> /dev/full is similar to /dev/zero except it always returns ENOSPC >>>> when you attempt to write to it. >>>> >>>> Reviewed by: jhibbits Discussed with: rpaulo >>> ... Please see lindev(4). >> >> I guess I wasn't loud enough. We already had the exact same feature >> via lindev(4). In fact, now it panics if we load both, i.e., >> "make_dev_credv: bad si_name (error=17, si_name=full)". Please see >> sys/dev/lindev/full.c. Also, the manual page is still symlinked from >> lindev.4. > > Thanks for letting me know about lindev(4). I've brought up /dev/full > to a few people and no one else mentioned it. Since /dev/full is not > linux specific and code/binary size addition is minor I've opted to > just remove lindev and leave my implementation. It seems reasonable to have it unconditional, since it is so small when it is not a separate module. Please merge any better style from lindev to null.c. I only noticed a couple of remaining style bugs in the new code in null.c. There are a few more in old code in null.c. lindev has about the same density of style bugs: full.c: % /* ARGSUSED */ The __unused annotations are ugly enough. lint has been unusable for a long time. % static int % full_read(struct cdev *dev __unused, struct uio *uio, int flags __unused) % { % int error = 0; Initialization in declaration. % % while (uio->uio_resid > 0 && error == 0) % error = uiomove(zbuf, MIN(uio->uio_resid, PAGE_SIZE), uio); Use of the MIN() macro. MIN() was removed from the kernel in 4.4BSD, but was restored in FreeBSD to allow broken code that uses it to work. The min() family of inline functions is supposed to be used in 4.4BSD and later. It is still used a lot. However, it is broken as designed since it is type-specific and thus is especially hard to use with typedefed types like typeof(uio->uio_resid). MIN() has smaller design errors. The correct API is a type-generic version of min(). Unfortunately, the good name min() is already used (it means take the minimum of u_int values). % Extra blank line. % return (error); % } % % /* ARGSUSED */ As above. % static int % full_write(struct cdev *dev __unused, struct uio *uio __unused, % int flags __unused) % { % This blank line is not extra, but is perfectly ugly to style(9) spec. % return (ENOSPC); % } % % /* ARGSUSED */ % int % lindev_modevent_full(module_t mod __unused, int type, void *data __unused) % { % % switch(type) { % case MOD_LOAD: % zbuf = (void *)malloc(PAGE_SIZE, M_TEMP, M_WAITOK | M_ZERO); Bogus cast. Has no effect. This is not C++. % full_dev = make_dev(&full_cdevsw, 0, UID_ROOT, GID_WHEEL, % 0666, "full"); % if (bootverbose) % printf("full: <full device>\n"); I don't like hiding this under bootverbose. 1 line per device is not much spam. % break; % I don't like the style of an extra blank line for each case in a switch. This style is normal in the kernel, but is not permitted by style(9) (because the example in style(9) doesn't use it). Fully ugly versions of this style put the extra blank line before each case statement. This switch statement is missing the extra blank line before the first case statement. null.c does this inconsistently. It uses the extra blank line for the switch statement in null_modevent(), but not for any other switch statement in the file. indent(1) has no chance of preserving such differences. % case MOD_UNLOAD: % destroy_dev(full_dev); % free(zbuf, M_TEMP); % break; % % case MOD_SHUTDOWN: % break; % % default: % return (EOPNOTSUPP); % } % Extra blank line. % return (0); % } lindev.c: % ... % /* % * "lindev" is supposed to be a collection of linux-specific devices % * that we also support, just not by default. % * While currently there is only "/dev/full", we are planning to see % * more in the future. % * This file is only the container to load/unload all supported devices; % * the implementation of each should go into its own file. % */ There is only dev/full in lindev now. I don't like having lots of little files supporting 1 feature each or 1 feature across 3 files. After removing lindev/full.c but not all of lindev, the 2 remaining files support 0 features. Removing all of lindev would break adding more features, if any. % /* ARGSUSED */ % static int % lindev_modevent(module_t mod, int type, void *data) Here there is useless lint markup, but no useful compiler markup (__unused). Since this apparently compilers without the markup, why use it at all? lindev.h: % ... % #ifndef _DEV_LINDEV_LINDEV_H Tab instead of space after #ifndef Nonstandard idempotency macro name. Standard ones have a trailing underscore. % #define _DEV_LINDEV_LINDEV_H % % int lindev_modevent_full(module_t, int, void *); Non-fancy formatting (no indentation of the function name). % % #endif /* _DEV_LINDEV_LINDEV_H */ Backwards comment on endif. null.c (before your changes): % #include <sys/param.h> % #include <sys/systm.h> % #include <sys/conf.h> % #include <sys/uio.h> % #include <sys/kernel.h> % #include <sys/malloc.h> % #include <sys/module.h> % #include <sys/priv.h> % #include <sys/disk.h> % #include <sys/bus.h> % #include <sys/filio.h> Totally disorded includes. % /* For use with destroy_dev(9). */ % static struct cdev *null_dev; % static struct cdev *zero_dev; Unnecessary comment. The code is very simple, and commenting just this 1 part of it is not useful. % % static d_write_t null_write; % static d_ioctl_t null_ioctl; % static d_ioctl_t zero_ioctl; % static d_read_t zero_read; Unsorted declarations. They are in the same "logical" order as the cdevsw struct members. I don't like using the typedefs to hide the prototypes. % % static struct cdevsw null_cdevsw = { % .d_version = D_VERSION, % .d_read = (d_read_t *)nullop, % .d_write = null_write, % .d_ioctl = null_ioctl, % .d_name = "null", % }; % % static struct cdevsw zero_cdevsw = { % .d_version = D_VERSION, % .d_read = zero_read, % .d_write = null_write, % .d_ioctl = zero_ioctl, % .d_name = "zero", % .d_flags = D_MMAP_ANON, % }; Before C99, the cdevsw initializers had to be in struct member order. This order is still mostly used. % % /* ARGSUSED */ % static int % null_write(struct cdev *dev __unused, struct uio *uio, int flags __unused) % { Missing blank line after declarations. % uio->uio_resid = 0; % Extra blank line. % return (0); % } % % /* ARGSUSED */ % static int % null_ioctl(struct cdev *dev __unused, u_long cmd, caddr_t data __unused, % int flags __unused, struct thread *td) % { % int error; Missing blank line after declarations. % error = 0; % Extra blank line. % switch (cmd) { % case DIOCSKERNELDUMP: % error = priv_check(td, PRIV_SETDUMPER); % if (error == 0) % error = set_dumper(NULL, NULL); % break; % case FIONBIO: % break; % case FIOASYNC: % if (*(int *)data != 0) % error = EINVAL; % break; % default: % error = ENOIOCTL; % } % return (error); % } % % /* ARGSUSED */ % static int % zero_ioctl(struct cdev *dev __unused, u_long cmd, caddr_t data __unused, % int flags __unused, struct thread *td) Gnu-style continuation indentation (indent -lp). KNF indentation (indent -ci4) is used for zero_ioctl(). % { % int error; % error = 0; % Blank lines in all the wrong places, as above. % switch (cmd) { % case FIONBIO: % break; % case FIOASYNC: % if (*(int *)data != 0) % error = EINVAL; % break; % default: % error = ENOIOCTL; % } % return (error); % } % % % /* ARGSUSED */ % static int % zero_read(struct cdev *dev __unused, struct uio *uio, int flags __unused) % { % void *zbuf; % ssize_t len; % int error = 0; % Blank lines in all the wrong places, as above. % KASSERT(uio->uio_rw == UIO_READ, % ("Can't be in %s for write", __func__)); Obfuscation of the function name using __func__. The messages is rather cryptic, and not in a standard format like "<function name>: <message>". Why assert this at all. zero_read() is only attached to d_read, so this assertion is about as useful as a parity check on entry to zero_read() but not for functions that are actually important. % zbuf = __DECONST(void *, zero_region); Use of the __DECONST() to hide type errors. I don't see how to avoid the type errors. The problem is that uiomove() can go in both directions, and in one direction the pointer needs to be to-non-const, so pointers that start as to-const for the other direction need to be corrupted. Here we start with a const zero_region and have to corrupt the pointer. This is safe since the uiomove() is only from zero_region. struct uiovec has similar design errors (there is a void * pointer in it that has to be used for both directions). Old versions of /dev/zero avoided this problem by malloc()ing a zero region just for /dev/zero and not declaring the pointer to it as volatile. % while (uio->uio_resid > 0 && error == 0) { % len = uio->uio_resid; % if (len > ZERO_REGION_SIZE) % len = ZERO_REGION_SIZE; % error = uiomove(zbuf, len, uio); % } % Extra blank line. % return (error); % } % ... Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140502165438.D1112>