From: Pekka Enberg <penberg@cs.helsinki.fi>
To: "Daniel Aragonés" <danarag@gmail.com>
Cc: "Randy.Dunlap" <rdunlap@xenotime.net>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC] minix filesystem: Corrected patch
Date: Sun, 22 Jan 2006 21:17:43 +0200 [thread overview]
Message-ID: <84144f020601221117t2c529da2xc0a14ee4322da4bf@mail.gmail.com> (raw)
In-Reply-To: <43D3D03C.6030504@gmail.com>
Hi Daniel,
On 1/22/06, Daniel Aragonés <danarag@gmail.com> wrote:
> Answering to the suggestion of Randy, here come the patches already corrected.
>
> This patches concern the update for Minix V3 support for both kernels 2.6.x.y and 2.4.x.
>
> Attached as text files "V3_2dot6_patch.txt" and "V3_2dot4_patch.txt" are the corrected versions of my first post dated January 19. As I wrote there...
Please post the 2.4 and 2.6 patches as separetely and inline the patch
in the mail as per Documentation/SubmittingPatches. I am unable to
apply your patch because my mail client meshes them together.
> @@ -108,14 +111,22 @@
> limit = kaddr + minix_last_byte(inode, n) - chunk_size;
> for ( ; p <= limit ; p = minix_next_entry(p, sbi)) {
> minix_dirent *de = (minix_dirent *)p;
> + minix3_dirent *de3 = (minix3_dirent *)p;
> if (de->inode) {
> int over;
> - unsigned l = strnlen(de->name,sbi->s_namelen);
> -
> - offset = p - kaddr;
> - over = filldir(dirent, de->name, l,
> - (n<<PAGE_CACHE_SHIFT) | offset,
> - de->inode, DT_UNKNOWN);
> + if (!(sbi->s_version == MINIX_V3)) {
> + unsigned l = strnlen(de->name,sbi->s_namelen);
> + offset = p - kaddr;
> + over = filldir(dirent, de->name, l,
> + (n<<PAGE_CACHE_SHIFT) | offset,
> + de->inode, DT_UNKNOWN);
> + } else {
> + unsigned l = strnlen(de3->name,sbi->s_namelen);
> + offset = p - kaddr;
> + over = filldir(dirent, de3->name, l,
> + (n<<PAGE_CACHE_SHIFT) | offset,
> + de3->inode, DT_UNKNOWN);
Hmm, strange formatting. Wouldn't it be better if you introduced a
name pointer and moved those filldir bits outside of the if-else
block? Less code duplication that way.
> - for ( ; (char *) de <= kaddr ; de = minix_next_entry(de,sbi)) {
> - if (!de->inode)
> - continue;
> - if (namecompare(namelen,sbi->s_namelen,name,de->name))
> - goto found;
> + for ( ; (char *) de <= kaddr ;
> + de = minix_next_entry(de,sbi),
> + de3 = minix_next_entry(de3,sbi)) {
> + if (!(sbi->s_version == MINIX_V3)) {
> + if (!de->inode)
> + continue;
> + if (namecompare(namelen,sbi->s_namelen,name,de->name))
> + goto found;
> + } else {
> + if (!de3->inode)
> + continue;
> + if (namecompare(namelen,sbi->s_namelen,name,de3->name))
> + goto found;
> + }
Same here.
> @@ -226,9 +248,16 @@
> if (!de->inode)
> goto got_it;
> err = -EEXIST;
> - if (namecompare(namelen,sbi->s_namelen,name,de->name))
> - goto out_unlock;
> - de = minix_next_entry(de, sbi);
> + if (!(sbi->s_version == MINIX_V3)) {
> + if (namecompare(namelen,sbi->s_namelen,name,de->name))
> + goto out_unlock;
> + de = minix_next_entry(de, sbi);
> + } else {
> + if (namecompare(namelen,sbi->s_namelen,name,de3->name))
> + goto out_unlock;
> + de = minix_next_entry(de, sbi);
> + de3 = minix_next_entry(de3, sbi);
Why do you do both here?
> @@ -242,9 +271,15 @@
> err = page->mapping->a_ops->prepare_write(NULL, page, from, to);
> if (err)
> goto out_unlock;
> - memcpy (de->name, name, namelen);
> - memset (de->name + namelen, 0, sbi->s_dirsize - namelen - 2);
> - de->inode = inode->i_ino;
> + if (!(sbi->s_version == MINIX_V3)) {
> + memcpy (de->name, name, namelen);
> + memset (de->name + namelen, 0, sbi->s_dirsize - namelen - 2);
> + de->inode = inode->i_ino;
> + } else {
> + memcpy (de3->name, name, namelen);
> + memset (de3->name + namelen, 0, sbi->s_dirsize - namelen - 4);
> + de3->inode = inode->i_ino;
> + }
Strange formatting.
> @@ -301,11 +337,13 @@
> memset(kaddr, 0, PAGE_CACHE_SIZE);
>
> de = (struct minix_dir_entry *)kaddr;
> - de->inode = inode->i_ino;
> - strcpy(de->name,".");
> + de3 = (struct minix3_dir_entry *)kaddr;
> + de->inode = de3->inode = inode->i_ino;
> + (sbi->s_version == MINIX_V3) ? strcpy(de3->name,".") : strcpy(de->name,".");
> de = minix_next_entry(de, sbi);
> - de->inode = dir->i_ino;
> - strcpy(de->name,"..");
> + de3 = minix_next_entry(de3, sbi);
> + de->inode = de3->inode = dir->i_ino;
> + (sbi->s_version == MINIX_V3) ? strcpy(de3->name,"..") : strcpy(de->name,"..");
> kunmap_atomic(kaddr, KM_USER0);
Formatting and please use if-else instead.
> while ((char *)de <= kaddr) {
> if (de->inode != 0) {
> /* check for . and .. */
> - if (de->name[0] != '.')
> - goto not_empty;
> - if (!de->name[1]) {
> - if (de->inode != inode->i_ino)
> + if (!(sbi->s_version == MINIX_V3)) {
> + if (de->name[0] != '.')
> + goto not_empty;
> + if (!de->name[1]) {
> + if (de->inode != inode->i_ino)
> + goto not_empty;
> + } else if (de->name[1] != '.')
> + goto not_empty;
> + else if (de->name[2])
> + goto not_empty;
> + } else {
> + if (de3->name[0] != '.')
> goto not_empty;
> - } else if (de->name[1] != '.')
> - goto not_empty;
> - else if (de->name[2])
> - goto not_empty;
> + if (!de3->name[1]) {
> + if (de3->inode != inode->i_ino)
> + goto not_empty;
> + } else if (de3->name[1] != '.')
> + goto not_empty;
> + else if (de3->name[2])
> + goto not_empty;
> + }
Formatting. Shouldn't you make that non_empty check a separate
function so you don't need to duplicate it?
> @@ -197,6 +201,23 @@
> sbi->s_dirsize = 32;
> sbi->s_namelen = 30;
> sbi->s_link_max = MINIX2_LINK_MAX;
> + } else if ( *(__u16 *)(bh->b_data + 24) == MINIX3_SUPER_MAGIC) {
> +
> + s->s_magic = MINIX3_SUPER_MAGIC;
> + sbi->s_imap_blocks = *(__u16 *)(bh->b_data + 6);
> + sbi->s_zmap_blocks = *(__u16 *)(bh->b_data + 8);
> + sbi->s_firstdatazone = *(__u16 *)(bh->b_data + 10);
> + sbi->s_log_zone_size = *(__u16 *)(bh->b_data + 12);
> + sbi->s_max_size = *(__u32 *)(bh->b_data + 16);
> + sbi->s_nzones = *(__u32 *)(bh->b_data + 20);
You probably want to introduce a struct minix3_super_block for this.
It's much more readable that way.
> + sbi->s_dirsize = 64;
> + sbi->s_namelen = 60;
> + sbi->s_version = MINIX_V3;
> + sbi->s_link_max = MINIX2_LINK_MAX;
> + if ( *(__u16 *)(bh->b_data + 28) != 1024) {
> + if (!sb_set_blocksize(s,( *(__u16 *)(bh->b_data + 28))))
> + goto out_bad_hblock;
> + }
You're now setting the block size twice for the V3 case.
> #define MINIX_INODES_PER_BLOCK ((BLOCK_SIZE)/(sizeof (struct minix_inode)))
> -#define MINIX2_INODES_PER_BLOCK ((BLOCK_SIZE)/(sizeof (struct minix2_inode)))
> +#define MINIX2_INODES_PER_BLOCK(b) ((b)/(sizeof (struct minix2_inode)))
Maybe this should be called minix_inodes_per_block instead and be a
static inline function?
next prev parent reply other threads:[~2006-01-22 19:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-01-22 18:34 [PATCH/RFC] minix filesystem: Corrected patch Daniel Aragonés
2006-01-22 19:17 ` Pekka Enberg [this message]
2006-01-22 20:22 Daniel Aragonés
2006-01-22 20:37 ` Pekka Enberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84144f020601221117t2c529da2xc0a14ee4322da4bf@mail.gmail.com \
--to=penberg@cs.helsinki.fi \
--cc=danarag@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rdunlap@xenotime.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).