[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: Extended Attributes and Access Control Lists

On Oct 28, 2001  23:03 +0100, Andreas Gruenbacher wrote:
> Since I'm not very much into the innards of ext3, can some of you please
> take a look at the patch, and see whether it contains any flaws (and tell
> me which flaws)? Thanks!
> The patch can be found at <http://acl.bestbits.at/>.

Hmm, I had a good look around, and couldn't find it.  Finally, I see
that it is already part of the "current" patches.


Well, as a start, it would be nice if the EA/ACL code followed the
coding style of ext2 and ext3 (and Linus) (i.e. function declarations).

- It seems that there are not enough files modified in the ext3 tree
  to implement EAs properly (at least compared to the number of files
  changed in the ext2 tree.  Is part of the patch missing?

- ext3_journal_stop() can return an error code.
- I would suggest using more specific macro names than "HDR" and "ENTRY"
- I'm not saying that there is a problem, but it in ext3_set_ext_attr()
  it is very hard to see whether your ext3_journal_get_write_access()
  calls always have an ext3_journal_dirty_metadata() to go along with
  them, and I think the original coders have the same problem (e.g. the
  commented-out call to ext3_journal_dirty_metadata() at the end.  Is it
  possible to split up this _huge_ function into easier-to-digest bits?
- In ext3_attr_free_inode() it doesn't appear that you are journaling
  the changes to the inode itself.  Are you doing this elsewhere?
- Where/how is ext3_ext_attr_check_block() used?  It would seem that this
  should be part of e2fsck only.  If runtime checks are really needed,
  could you do something like ext3_check_page() where we only check the
  data a single time when it is read from disk?  If you find a bad EA
  block, you should probably call ext3_error() to force fsck on the
  next boot.
- ea_error() should probably just be replaced with a call to ext3_error()
  or ext3_warning(), as appropriate, which would solve the problem above.
- In ext3_ext_attr_cmp() shouldn't it be enough to compare only the
  hash values of each entry (or even better - only the hash value in
  the header)?  Maybe the entry hash should include the name as well?
  I don't think you will ever want to merge EAs with identical data
  but different names.  It seems like we are comparing too many things.
- Conversely, it appears that we are not comparing EA values in any
  cardinal "order", so that if you have one block with EAs foo,bar,
  and another block with EAs bar,foo (both with identical contents)
  they will not merge even though they should (AFAIK, position within
  the EA block should not be important).  Maybe comparing in EA-name
  order is best? 
- The mbcache stuff - you may as well move dcache, icache, dqcache
  into the list of caches, since we seem to be getting a lot of them,
  and it would clean up the code a bit.

- You should probably see if Linus will allow you to reserve the EA
  syscall numbers, so that you don't need to change them again if he
  adds new syscalls.  The EA stuff is mature enough (and close enough
  to inclusion into the main kernel) that allocating syscall numbers
  is a reasonable thing to do, even if the code is not included yet.

Cheers, Andreas
Andreas Dilger

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]