Date: Fri, 30 Nov 2018 18:34:03 -0800 From: Conrad Meyer <cem@freebsd.org> To: eugen@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r341352 - head/usr.sbin/trim Message-ID: <CAG6CVpWMCsoho-gDnuSHWjh=KsbsyL6=xBT%2BQX=dfRhh3Cx-gg@mail.gmail.com> In-Reply-To: <201811302157.wAULv2iH057184@repo.freebsd.org> References: <201811302157.wAULv2iH057184@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 =E2=80=94 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@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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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-leveli= ng > -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 DIOCGMEDIASIZ= E > .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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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[] =3D > - "$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 d= ryrun, 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 =3D 0; > length =3D offset =3D 0; > name =3D argv[0]; > - dryrun =3D verbose =3D 1; > + dryrun =3D verbose =3D true; > > while ((ch =3D getopt(argc, argv, "Nfl:o:qr:v")) !=3D -1) > switch (ch) { > case 'N': > - dryrun =3D 1; > - verbose =3D 1; > + dryrun =3D true; > + verbose =3D true; > break; > case 'f': > - dryrun =3D 0; > + dryrun =3D false; > break; > case 'l': > case 'o': > if (expand_number(optarg, &usz) =3D=3D -1 || > - (off_t)usz < 0 || > - (usz =3D=3D 0 && ch =3D=3D 'l')) > + (off_t)usz < 0 || (usz =3D=3D 0 &= & ch =3D=3D 'l')) > errx(EX_USAGE, > - "invalid %s of the region: `%s'", > + "invalid %s of the region: %s", > ch =3D=3D 'o' ? "offset" : "lengt= h", > optarg); > if (ch =3D=3D 'o') > @@ -88,16 +88,16 @@ main(int argc, char **argv) > length =3D (off_t)usz; > break; > case 'q': > - verbose =3D 0; > + verbose =3D false; > break; > case 'r': > if ((length =3D getsize(optarg)) =3D=3D 0) > errx(EX_USAGE, > "invalid zero length reference fi= le" > - " for the region: `%s'", optarg); > + " for the region: %s", optarg); > break; > case 'v': > - verbose =3D 1; > + verbose =3D 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 =3D open(path, O_RDONLY | O_DIRECT)) < 0) { > + if ((fd =3D open(path, flags)) < 0) { > if (errno =3D=3D ENOENT && path[0] !=3D '/') { > if (asprintf(&tstr, "%s%s", _PATH_DEV, path) < 0) > errx(EX_OSERR, "no memory"); > - fd =3D open(tstr, O_RDONLY | O_DIRECT); > + fd =3D 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 =3D 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 dis= k: " > - "`%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 ver= bose) > { > off_t arg[2]; > - char *tstr; > int error, fd; > > if (length =3D=3D 0) > length =3D 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 =3D open(path, O_WRONLY | O_DIRECT)) < 0) { > - if (errno =3D=3D ENOENT && path[0] !=3D '/') { > - if (asprintf(&tstr, "%s%s", _PATH_DEV, path) < 0) > - errx(EX_OSERR, "no memory"); > - fd =3D open(tstr, O_WRONLY | O_DIRECT); > - free(tstr); > - } > - } > - > - if (fd < 0) > - err(EX_NOINPUT, "`%s'", path); > - > + fd =3D opendev(path, O_WRONLY | O_DIRECT); > arg[0] =3D offset; > arg[1] =3D length; > > error =3D 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", >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWMCsoho-gDnuSHWjh=KsbsyL6=xBT%2BQX=dfRhh3Cx-gg>