svn commit: r341352 - head/usr.sbin/trim
Conrad Meyer
cem at freebsd.org
Sat Dec 1 02:34:22 UTC 2018
Hi Eugene,
Generally when a committer puts a patch on phabricator, they'll commit
it when they're ready. In particular this patch hadn't actually been
approved by any reviewers yet and review discussion was still active.
As far as I can tell, imp@ hadn't indicated he wasn't going to commit
his patch himself. I'd love to be imagining things, but it seems like
the general trend is you consistently fail to follow basic etiquette
and consistently ignore review and concerns.
This time around you definitely don't have the excuse of phabricator
being silent for months — the review hadn't seen any 24h period of
inactivity since its creation.
I was tentatively ok with trim staying in based on imp@ taking charge
of cleaning up the deficiencies, but you've gone around imp@ and
ignored me completely.
Please back out trim and consider taking a few days off. The way trim
has entered the tree is just not how we do things as a project. I
think there's universal consensus that we want a tool that does what
trim does in base; there's some bikeshedding about whether it should
live in dd or not (it shouldn't); but rest assured, it will go in in
some form. My ask is that it to go in the right way, instead of the
total mess it has so been so far.
Thanks,
Conrad
On Fri, Nov 30, 2018 at 1:57 PM Eugene Grosbein <eugen at freebsd.org> wrote:
>
> Author: eugen
> Date: Fri Nov 30 21:57:02 2018
> New Revision: 341352
> URL: https://svnweb.freebsd.org/changeset/base/341352
>
> Log:
> trim(8): serveral style fixes
>
> Submitted by: imp
> MFC after: 1 month
> Differential Revision: https://reviews.freebsd.org/D18380
>
> Modified:
> head/usr.sbin/trim/trim.8
> head/usr.sbin/trim/trim.c
>
> Modified: head/usr.sbin/trim/trim.8
> ==============================================================================
> --- head/usr.sbin/trim/trim.8 Fri Nov 30 20:58:08 2018 (r341351)
> +++ head/usr.sbin/trim/trim.8 Fri Nov 30 21:57:02 2018 (r341352)
> @@ -50,8 +50,8 @@
> The
> .Nm
> utility erases specified region of the device.
> -It is only relevant for flash based storage devices that use wear-leveling
> -algorithms.
> +It is mostly relevant for storage that implement trim (like flash based,
> +or thinly provisioned storage).
> .Sy All erased data is lost.
> .Pp
> The following options are available:
> @@ -153,12 +153,13 @@ is special device file not supporting DIOCGMEDIASIZE
> .Xr ada 4 ,
> .Xr da 4 ,
> .Xr ioctl 2 ,
> +.Xr nda 4 ,
> .Xr sysexits 3
> .Sh HISTORY
> The
> .Nm
> utility first appeared in
> -.Fx 12.0 .
> +.Fx 12.1 .
> .Sh AUTHORS
> The
> .Nm
>
> Modified: head/usr.sbin/trim/trim.c
> ==============================================================================
> --- head/usr.sbin/trim/trim.c Fri Nov 30 20:58:08 2018 (r341351)
> +++ head/usr.sbin/trim/trim.c Fri Nov 30 21:57:02 2018 (r341352)
> @@ -37,49 +37,49 @@
> #include <libutil.h>
> #include <limits.h>
> #include <paths.h>
> +#include <stdbool.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sysexits.h>
> #include <unistd.h>
>
> -#ifndef lint
> -static const char rcsid[] =
> - "$FreeBSD$";
> -#endif /* not lint */
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
>
> -static int trim(char *path, off_t offset, off_t length, int dryrun, int verbose);
> -static off_t getsize(char *path);
> -static void usage(char *name) __dead2;
> +static off_t getsize(const char *path);
> +static int opendev(const char *path, int flags);
> +static int trim(const char *path, off_t offset, off_t length, bool dryrun, bool verbose);
> +static void usage(const char *name);
>
> int
> main(int argc, char **argv)
> {
> off_t offset, length;
> uint64_t usz;
> - int ch, dryrun, error, verbose;
> + int ch, error;
> + bool dryrun, verbose;
> char *fname, *name;
>
> error = 0;
> length = offset = 0;
> name = argv[0];
> - dryrun = verbose = 1;
> + dryrun = verbose = true;
>
> while ((ch = getopt(argc, argv, "Nfl:o:qr:v")) != -1)
> switch (ch) {
> case 'N':
> - dryrun = 1;
> - verbose = 1;
> + dryrun = true;
> + verbose = true;
> break;
> case 'f':
> - dryrun = 0;
> + dryrun = false;
> break;
> case 'l':
> case 'o':
> if (expand_number(optarg, &usz) == -1 ||
> - (off_t)usz < 0 ||
> - (usz == 0 && ch == 'l'))
> + (off_t)usz < 0 || (usz == 0 && ch == 'l'))
> errx(EX_USAGE,
> - "invalid %s of the region: `%s'",
> + "invalid %s of the region: %s",
> ch == 'o' ? "offset" : "length",
> optarg);
> if (ch == 'o')
> @@ -88,16 +88,16 @@ main(int argc, char **argv)
> length = (off_t)usz;
> break;
> case 'q':
> - verbose = 0;
> + verbose = false;
> break;
> case 'r':
> if ((length = getsize(optarg)) == 0)
> errx(EX_USAGE,
> "invalid zero length reference file"
> - " for the region: `%s'", optarg);
> + " for the region: %s", optarg);
> break;
> case 'v':
> - verbose = 1;
> + verbose = true;
> break;
> default:
> usage(name);
> @@ -117,28 +117,38 @@ main(int argc, char **argv)
> return (error ? EXIT_FAILURE : EXIT_SUCCESS);
> }
>
> -static off_t
> -getsize(char *path)
> +static int
> +opendev(const char *path, int flags)
> {
> - struct stat sb;
> - char *tstr;
> - off_t mediasize;
> int fd;
> + char *tstr;
>
> - if ((fd = open(path, O_RDONLY | O_DIRECT)) < 0) {
> + if ((fd = open(path, flags)) < 0) {
> if (errno == ENOENT && path[0] != '/') {
> if (asprintf(&tstr, "%s%s", _PATH_DEV, path) < 0)
> errx(EX_OSERR, "no memory");
> - fd = open(tstr, O_RDONLY | O_DIRECT);
> + fd = open(tstr, flags);
> free(tstr);
> }
> }
>
> if (fd < 0)
> - err(EX_NOINPUT, "`%s'", path);
> + err(EX_NOINPUT, "fstat failed: %s", path);
>
> + return (fd);
> +}
> +
> +static off_t
> +getsize(const char *path)
> +{
> + struct stat sb;
> + off_t mediasize;
> + int fd;
> +
> + fd = opendev(path, O_RDONLY | O_DIRECT);
> +
> if (fstat(fd, &sb) < 0)
> - err(EX_IOERR, "`%s'", path);
> + err(EX_IOERR, "fstat failed: %s", path);
>
> if (S_ISREG(sb.st_mode) || S_ISDIR(sb.st_mode)) {
> close(fd);
> @@ -148,62 +158,50 @@ getsize(char *path)
> if (!S_ISCHR(sb.st_mode) && !S_ISBLK(sb.st_mode))
> errx(EX_DATAERR,
> "invalid type of the file "
> - "(not regular, directory nor special device): `%s'",
> + "(not regular, directory nor special device): %s",
> path);
>
> if (ioctl(fd, DIOCGMEDIASIZE, &mediasize) < 0)
> - errx(EX_UNAVAILABLE,
> + err(EX_UNAVAILABLE,
> "ioctl(DIOCGMEDIASIZE) failed, probably not a disk: "
> - "`%s'", path);
> - close(fd);
> + "%s", path);
>
> + close(fd);
> return (mediasize);
> }
>
> static int
> -trim(char *path, off_t offset, off_t length, int dryrun, int verbose)
> +trim(const char *path, off_t offset, off_t length, bool dryrun, bool verbose)
> {
> off_t arg[2];
> - char *tstr;
> int error, fd;
>
> if (length == 0)
> length = getsize(path);
>
> if (verbose)
> - printf("trim `%s' offset %ju length %ju\n",
> - path, (uintmax_t)offset, (uintmax_t)length);
> + printf("trim %s offset %ju length %ju\n",
> + path, (uintmax_t)offset, (uintmax_t)length);
>
> if (dryrun) {
> printf("dry run: add -f to actually perform the operation\n");
> return (0);
> }
>
> - if ((fd = open(path, O_WRONLY | O_DIRECT)) < 0) {
> - if (errno == ENOENT && path[0] != '/') {
> - if (asprintf(&tstr, "%s%s", _PATH_DEV, path) < 0)
> - errx(EX_OSERR, "no memory");
> - fd = open(tstr, O_WRONLY | O_DIRECT);
> - free(tstr);
> - }
> - }
> -
> - if (fd < 0)
> - err(EX_NOINPUT, "`%s'", path);
> -
> + fd = opendev(path, O_WRONLY | O_DIRECT);
> arg[0] = offset;
> arg[1] = length;
>
> error = ioctl(fd, DIOCGDELETE, arg);
> if (error < 0)
> - warn("ioctl(DIOCGDELETE) failed for `%s'", path);
> + warn("ioctl(DIOCGDELETE) failed: %s", path);
>
> close(fd);
> return (error);
> }
>
> static void
> -usage(char *name)
> +usage(const char *name)
> {
> (void)fprintf(stderr,
> "usage: %s [-[lo] offset[K|k|M|m|G|g|T|t]] [-r rfile] [-Nfqv] device ...\n",
>
More information about the svn-src-head
mailing list