Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

Quoting Vladimir 'phcoder' Serbinenko <address@hidden>:

> Update: added symlink support
>
> On Fri, Jul 17, 2009 at 9:30 PM, Vladimir 'phcoder'
> Serbinenko<address@hidden> wrote:
> > Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which
> > is similar to AFS. So I took the later as codebase. I found some bugs
> > and incompletenesses in it. Here is the fix. Tested using grub-fstest
> > on virtual machine image downloaded from Syllable website and then
> > successfully booted from it using grub-mkrescue image

Many changes have no value at all, e.g.:

-#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331      /* AFS1 */
+/* AFS1 in hexadecimal.  */
+#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331
 #define        GRUB_AFS_SBLOCK_MAGIC2  0xdd121031
 #define        GRUB_AFS_SBLOCK_MAGIC3  0x15b6830e

The original comment suggested that only the first value made it clear  
that only the first value is AFS1.  Now it's unclear.  There is no  
need to point out that AFS1 is in hexadecimal notation.  It's obvious  
to anyone competent to read C code.  Besides, there is no need to  
single out AFS1.

-  grub_uint32_t index_type;            /* Key data-key only used for  
index files */
+  /* Key data-key only used for index files. */
+  grub_uint32_t index_type;
   grub_uint32_t inode_size;

The same comment applies.

-  if ((! dir->inode.stream.size) ||
+  if ((dir->inode.stream.size == 0) ||

The later is marginally better, but it would be easier to review your  
patches if you don't include such changes.

Double semicolons can be removed in all files, and it doesn't need a review.

It's better to split fixes from the new features.

I don't have afs images around.  It would be great if you test all new  
functionality with valgrind.  It's very good at finding mistakes in  
the code.

--
Regards,
Pavel Roskin