On Thu, 1 May 2014, Eitan Adler wrote:
On 1 May 2014 09:59, Jung-uk Kim <j...@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
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
