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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 24 Sep 2010 11:19:13 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	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 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.

> 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.

> 	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?

> 	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.

> 	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

--
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