Skip site navigation (1)Skip section navigation (2)
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>