From owner-svn-src-head@freebsd.org Sat Dec 1 02:34:22 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 65F961158F01; Sat, 1 Dec 2018 02:34:22 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-io1-f42.google.com (mail-io1-f42.google.com [209.85.166.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id BCCC98386B; Sat, 1 Dec 2018 02:34:21 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-io1-f42.google.com with SMTP id s22so6122118ioc.8; Fri, 30 Nov 2018 18:34:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc:content-transfer-encoding; bh=54+mfksFnulO+FonJ4eh/8+t+qQtE79DF69XWtc4Fmc=; b=cMDBSl1QJnb0vL/IA81dsGD7x5sUp9W3y6JaAA8E4Uo8ExwNZ62cD6un0/zLHeOPMB /tXAX3yOQwErglX1p62iqgZ14cyeHD3XTU301PZhwebGv4nD2WUrwDBjCtln7ejfsza0 745WwA/95upCIJ+QNx8JWJt5f29IuS28OJZyEPJqrDhHRBSlOmYu86+ZWE45nOsk7T0G OoDnxQfdPqtPZ6MDXsXnsdjQl5OwNMO9rqE3D8k34PJhlJxtV0l9KY/RMjedV1QgBdch z4NKwJiBbNDYhwIHBjc5wHjv9qY7bsnHI7X8Xj8BSV8JCY7nEFbBXjUmX+MQAcHBJO1S DGsg== X-Gm-Message-State: AA+aEWYCOf47ND6YgSJ2iHw9UAVUo6AtCYbGZpm2vBILzepOSVCTZ6DG BkyQnfn3v6ElmpXt8S1zmKbjLXsm X-Google-Smtp-Source: AFSGD/WK64wkmUF+P02qfuN3i3Hm6oeK6yccjK0yNl/0FlCvYvM3zfHpne1IDbCO5GrvXVKrwjOmqA== X-Received: by 2002:a5d:8788:: with SMTP id f8mr2136417ion.239.1543631654849; Fri, 30 Nov 2018 18:34:14 -0800 (PST) Received: from mail-io1-f42.google.com (mail-io1-f42.google.com. [209.85.166.42]) by smtp.gmail.com with ESMTPSA id 14sm1553685itm.3.2018.11.30.18.34.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 30 Nov 2018 18:34:14 -0800 (PST) Received: by mail-io1-f42.google.com with SMTP id k7so6125575iob.6; Fri, 30 Nov 2018 18:34:14 -0800 (PST) X-Received: by 2002:a6b:6111:: with SMTP id v17mr77696iob.107.1543631654186; Fri, 30 Nov 2018 18:34:14 -0800 (PST) MIME-Version: 1.0 References: <201811302157.wAULv2iH057184@repo.freebsd.org> In-Reply-To: <201811302157.wAULv2iH057184@repo.freebsd.org> Reply-To: cem@freebsd.org From: Conrad Meyer Date: Fri, 30 Nov 2018 18:34:03 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r341352 - head/usr.sbin/trim To: eugen@freebsd.org Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: BCCC98386B X-Spamd-Result: default: False [-3.95 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; HAS_REPLYTO(0.00)[cem@freebsd.org]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; REPLYTO_ADDR_EQ_FROM(0.00)[]; RCVD_COUNT_THREE(0.00)[4]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; NEURAL_HAM_SHORT(-0.97)[-0.970,0]; FORGED_SENDER(0.30)[cem@freebsd.org,csecem@gmail.com]; RCVD_TLS_LAST(0.00)[]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; TAGGED_FROM(0.00)[]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; FROM_NEQ_ENVFROM(0.00)[cem@freebsd.org,csecem@gmail.com]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[freebsd.org]; IP_SCORE(-0.97)[ipnet: 209.85.128.0/17(-3.39), asn: 15169(-1.34), country: US(-0.09)]; RCVD_IN_DNSWL_NONE(0.00)[42.166.85.209.list.dnswl.org : 127.0.5.0]; RWL_MAILSPIKE_POSSIBLE(0.00)[42.166.85.209.rep.mailspike.net : 127.0.0.17] X-Rspamd-Server: mx1.freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Dec 2018 02:34:22 -0000 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 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 > #include > #include > +#include > #include > #include > #include > #include > > -#ifndef lint > -static const char rcsid[] =3D > - "$FreeBSD$"; > -#endif /* not lint */ > +#include > +__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", >