Messages in this thread | | | From | Mike Frysinger <> | Subject | Re: Utility code to generate a VMUFAT filesystem | Date | Sun, 25 Mar 2012 02:32:27 -0400 |
| |
On Wednesday 21 March 2012 00:41:17 Adrian McMenamin wrote: > /* > * mkfs.vmufat.c - make a linux (VMUFAT) filesystem > * > * Copyright (c) 2012 Adrian McMenamin adrianmcmenamin@gmail.com > * Licensed under Version 2 of the GNU General Public Licence > * > * Parts shamelessly copied from other mkfs code > * copyright Linus Torvalds and others > */
might be useful to sneak into tools/ or Documentation/ or fs/vmufat/
> void usage()
(void)
seems like most (if not just about all) funcs in here should be static
> int checkmount(char* device_name) > { > FILE * f; > struct mntent * mnt;
kernel style wise, this really should be: char *foo; FILE *f; struct mntent *mnt; etc...
device_name here should be const
> if ((f = setmntent(_PATH_MOUNTED, "r")) == NULL) > return;
there is libmount now, but that might be overkill for this small tool
> int readforbad(struct badblocklist** root, char* filename, int verbose)
filename should be const
> { > int error = 0; > FILE *listfile; > int badblocks = 0; > unsigned long blockno; > ... > > while (!feof(listfile)) { > if (fscanf(listfile, "%ld\n", &blockno) != 1) {
%ld is "signed long" but blockno is "unsigned long". probably want %lu here and elsewhere in this func.
> void _fill_root_block(char* buf, const struct vmuparam* param)
incoming buf is "char *" ...
> uint16_t* wordbuf; > > wordbuf = (uint16_t *)buf;
... which you cast up to "uint16_t *" ...
> int mark_root_block(int device_numb, const struct vmuparam *param, int > verbose) { > char zilches[BLOCKSIZE]; > int i, error = 0; > > for (i = 0; i < BLOCKSIZE; i++) > zilches[i] = '\0'; > > _fill_root_block(zilches, param);
... and is declared on the stack as "char *". there are no alignment requirements here, and iirc, superh doesn't support unaligned loads. so you should add gcc aligned attributes, or declare the buffer better: uint16_t zilches[BLOCKSIZE / 2];
> int zero_blocks(int device_numb, const struct vmuparam *param, int verbose) > { > char zilches[BLOCKSIZE]; > int i, error = -1; > > for (i = 0; i < BLOCKSIZE; i++) > zilches[i] = '\0';
memset(zilches, '\0', BLOCKSIZE);
> int scanforbad(int device_numb, struct badblocklist** root, int verbose) > { > int error = 0, i; > struct badblocklist *lastbadblock = NULL; > off_t size; > long got; > char buffer[BLOCKSIZE]; > > size = lseek(device_numb, 0, SEEK_END);
if you use fstat() here ...
> for (i = 0; i < size/BLOCKSIZE; i++) > { > if (verbose > 0) > printf("Testing block %i\n", i); > if (lseek(device_numb, i * BLOCKSIZE, SEEK_SET) != > i * BLOCKSIZE) { > printf("Seek failed on device\n"); > error = -1; > goto out; > } > got = read(device_numb, buffer, BLOCKSIZE);
... and pread() here, there's no need for the lseek()'s at all.
> if (lseek(device_numb, fatblock * BLOCKSIZE, SEEK_SET) < 0) > goto out; > if (read(device_numb, buffer, BLOCKSIZE) != BLOCKSIZE) > goto out;
use pread() instead of lseek() && read()
> if (lseek(device_numb, fatblock * BLOCKSIZE, SEEK_SET) < 0) > goto out; > if (write(device_numb, buffer, BLOCKSIZE) != BLOCKSIZE) > goto out;
and pwrite() instead of lseek() && write()
> if (checkmount(device_name) < 0) > goto out; > > if (stat(device_name, &statbuf) < 0) { > printf("Cannot get status of %s\n", device_name); > goto out; > } > if (!S_ISBLK(statbuf.st_mode)) { > printf("%s must be a block device\n", device_name); > goto out; > } > device_numb = open(device_name, O_RDWR|O_EXCL);
technically you've got a race condition here. really should do the open() and then do fstat() on the fd you get back.
also, what's with the O_EXCL ? that only makes sense with O_CREAT.
should you open+fstat before checking for the mount point ? guess it doesn't really matter.
it's useful to be able to format regular files as filesystems especially for debugging. most mkfs tools issue a warning or prompt by default, and using the -f (force) flag allows you to format regular files. -mike [unhandled content-type:application/pgp-signature] | |