linux-ext4 - Re: [PATCH 0/3 v. 8] Ext3/Ext4 Batched discard support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1009271121130.2743@dhcp-lab-213.englab.brq.redhat.com>
Date:	Mon, 27 Sep 2010 11:30:10 +0200 (CEST)
From:	Lukas Czerner <lczerner@...hat.com>
To:	Andreas Dilger <adilger@...ger.ca>
cc:	Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org,
	tytso@....edu, rwheeler@...hat.com, sandeen@...hat.com,
	snitzer@...il.com
Subject: Re: [PATCH 0/3 v. 8] Ext3/Ext4 Batched discard support

On Fri, 24 Sep 2010, Andreas Dilger wrote:

> On 2010-09-24, at 09:38, Lukas Czerner wrote:
> > /*
> > * e2trim.c - discard the part (or whole) mounted filesystem.
> > *
> > * Usage: e2trim [options] <mount point>
> > */
> 
> This tool isn't extN specific at all, and should probably go into util-linux or similar.  That said, it might make sense to add an option to e2fsck to have it trim the free space at the end, since it will just have verified the bitmaps are correct (which is the safest time to do this).
> 
> > const char *program_name = "fstrim";
> 
> Ah, your comment block has an inconsistent name for the program.

Right, I have changed it from e2trim to fstrim because of the same reason
you've just pointed out. Looks like I had missed some old comments.
> 
> > static void usage(void)
> > {
> > 	fprintf(stderr, "Usage: %s [-s start] [-l length] [-m minimum-extent]"
> > 	" [-v] directory\n\t-s Starting Byte to discard from\n"
> 
> This formatting, while saving a line, makes it hard to read.  Better is:
> 
> 	fprintf(stderr,
>                 "Usage: %s [-s start] [-l length] [-m minimum-extent]"
> 	        " [-v] {mountpoint}\n\t"
> 		"-s Starting Byte to discard from\n\t"
> 		"-l Number of Bytes to discard from the start\n\t"
> 		"-m Minimum extent length to discard\n\t"
> 		"-v Verbose - number of discarded bytes\n",
> 		program_name);
> 
> > static unsigned long long get_number(char **optarg)
> > {
> > 	/* compute the number */
> > 	while ((*opt >= '0') && (*opt <= '9') && (number < max)) {
> > 		number = number * 10 + *opt++ - '0';
> > 	}
> 
> Umm, strtoul(opt, &end, 0) is a much nicer way to parse this, and it also allows hex-formatted input.

That's right, but my original idea was to let the user write parameters
like 10kk etc. But when I think of it now, it is not exactly useful :)
> 
> > 	while (1) {
> > 		/* determine if units are defined */
> > 		switch(*opt++) {
> > 			case 'K': /* kilobytes */
> > 			case 'k': 
> > 				number *= 1024;
> > 				break;
> > 			case 'M': /* megabytes */
> > 			case 'm':
> > 				number *= 1024 * 1024;
> > 				break;
> > 			case 'G': /* gigabytes */
> > 			case 'g':
> > 				number *= 1024 * 1024 * 1024;
> > 				break;
> 
> I usually implement this by descending TGMK, multiply by 1024 at each step, and then skip the "break" in between.  Definitely you want to have "T" as a suffix these days.
> 
> > 			case ':': /* delimiter */
> 
> It isn't clear from the usage message what this delimiter is used for?

Oh, I have just reused this part from another project of mine, so I have
probably missed that out. Thanks.

> 
> > 	if (argc > 2) {
> > 		opts->range = calloc(3, sizeof(uint64_t));
> 
> Thinking about this more, it might make more sense to have a structure with named fields here.  This simplifies the ioctl definition, but also makes it much more clear from the caller which field is which.  Otherwise, there will be callers that get the order of parameters wrong, etc.

Definitely, looks like I am going to use a structure for this ioctl.

> 
> > 	if (stat(opts->mpoint, &sb) == -1) {
> > 		fprintf(stderr,"%s is not a valid directory\n", opts->mpoint);
> 
> Using strerror() here to print the actual error is better.  It might be e.g. a permission problem, etc.  Also, using perror() will localize the message.
> 
> > 	fd = open(opts->mpoint, O_RDONLY);
> > 	if (fd < 0) {
> > 		perror("open");
> 
> I prefer strerror() over perror(), since it allows creating a better error:
> 
>                   fprintf(stderr, "%s: open(%s): %s\n",
>                            program_name, opts->mpoint, strerror(errno));
> 
> 
> Cheers, Andreas
> 

Andreas, thanks a lot for reviewing this.

-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html