Re: Fatfs patch for writing support

[Marcus Brinkmann]

On Sat, Jul 19, 2003 at 04:50:42PM +0200, Marco Gerards wrote:
> Here is a new version of this patch. I've included some GCS related
> updates and removed something that is not relevant form the last
> patch. I've also changed the changelog entry a bit so the changes are
> grouped.

Great, thanks for this.  I have a couple of remarks.  Please consider them
and then upload the reviewed patch to Savannah.

> 2003-07-07  Marco Gerards  <metgerards@student.han.nl>
> 
>       * node-create.c: New file.

It would be nice to have a comment in that file (like, before the actual
content) why we need to override this from libdiskfs.  IE, explain the main
difference in operation.

>       * Makefile (SRCS): Added node-created.c.
>       * dir.c: Include <hurd/fsys.h>.
>       (diskfs_direnter_hard): Initialize a new block with zeros.
>       (diskfs_direnter_hard): Enter direntry and setup the virtual
>       inode. Also handle directories correctly.

We don't repeat the function name this way, just list all changes in one
entry.
        
>       (diskfs_rewrite_hard): Function rewritten.
>       
>       (diskfs_dirempty): Change logic to test if a file was deleted.

I don't like the empty lines here, with no new "* dir.c".  Either they are
different changes and require a new "*", or they belong together.  In this
case, I suggest to just lump everything together.  They are lots of small
cleanups for one goal, write support.
        
>       * fat.c (fat_extend_chain): Move spin_lock to prevent deadlock.

This one is wrong, you must not access dn->length_of_chain without holding
the lock (the caller only has the reader lock of alloc_lock).  So just
unlock the lock before the "return 0" in the if (because that is the bug you
were seeing, I guess).

>       (fat_extend_chain): Set dn->last to 0 when deallocating the
>       complete file.
>       (fat_extend_chain): Update dn->last when not deallocating the
>       complete file.
>       (fat_extend_chain): Set dn->first to zero when the complete file
>       was deallocated. Also update dn->length_of_chain to the new amount
>       of clusters in the chain.

Again, just lump all in one (fat_extend_chain).
        
>       * main.c (diskfs_readonly): Remove global variable.
>       (diskfs_hard_readonly): Likewise.
> 
> Common subdirectories: /home/marco/src/hurdcvs/hurd/fatfs/CVS and fatfs/CVS
> diff -BNup /home/marco/src/hurdcvs/hurd/fatfs/dir.c fatfs/dir.c
> --- /home/marco/src/hurdcvs/hurd/fatfs/dir.c  2003-05-10 02:12:29.000000000 
> +0200
> +++ fatfs/dir.c       2003-07-19 18:43:14.000000000 +0200
> @@ -1,5 +1,7 @@
> -/* main.c - FAT filesystem.
> -   Copyright (C) 1997, 1998, 1999, 2002 Free Software Foundation, Inc.
> +/* dir.c - Directory management routines.
> +   Copyright (C) 1997, 1998, 1999, 2002, 2003 Free Software
> +   Foundation, Inc.

Try hard to get it in one line, by removing the spaced between the years.


Also watch your coding style.  I have seen this (two spaces):

+  node->dn->length_of_chain =  clusters_to_keep;

:)

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' GNU      http://www.gnu.org    marcus@gnu.org
Marcus Brinkmann              The Hurd http://www.gnu.org/software/hurd/
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de/