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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
