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@xxxxxxxxx
> * 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
Attachment:
signature.asc
Description: This is a digitally signed message part.
