Re: [PATCH] Corrections to affs and sfs

Re: [PATCH] Corrections to affs and sfs

[Kalamatee]

<bump>

FWIW - This patch has been used in the AROS build of grub for the best
part of the last year - without it SFS doesnt work.

Re: [PATCH] Corrections to affs and sfs

[Robert Millan]

On Sun, Feb 22, 2009 at 01:30:55AM +0000, Kalamatee wrote:
> <bump>
> 
> FWIW - This patch has been used in the AROS build of grub for the best
> part of the last year - without it SFS doesnt work.

I just committed it.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[PATCH] Corrections to affs and sfs

[Krzysztof Smiechowicz]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

Hello,

I would like to submit the following patch:

* fs/affs.c: Return failure when directory iteration failed.

* fs/sfs.c: Return failure when directory iteration failed. Correct
order in which btree nodes are read.

These changes are needed to boot AROS Research Operating System from SFS
/ FFS.

Best regards,
Krzysztof


[-- Attachment #2: affs-sfs.diff --]
[-- Type: text/x-diff, Size: 1345 bytes --]

Index: fs/affs.c
===================================================================
--- fs/affs.c	(revision 1919)
+++ fs/affs.c	(working copy)
@@ -381,7 +381,7 @@
  fail:
   grub_free (node);
   grub_free (hashtable);
-  return 1;
+  return 0;
 }
 
 
Index: fs/sfs.c
===================================================================
--- fs/sfs.c	(revision 1919)
+++ fs/sfs.c	(working copy)
@@ -172,7 +172,8 @@
 	  return grub_errno;
 	}
 
-      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
+      grub_uint16_t nodescount = grub_be_to_cpu16(tree->nodes);
+      for (i = nodescount - 1; i >= 0; i--)
 	{
 
 #define EXTNODE(tree, index)						\
@@ -180,17 +181,9 @@
 					 + (index) * (tree)->nodesize))
 
 	  /* Follow the tree down to the leaf level.  */
-	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) >= block)
+	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) <= block)
 	      && !tree->leaf)
 	    {
-	      next = grub_be_to_cpu32 (EXTNODE (tree, i - 1)->data);
-	      break;
-	    }
-
-	  /* In case the last node is reached just use that one, it is
-	     the right match.  */
-	  if (i + 1 == grub_be_to_cpu16 (tree->nodes) && !tree->leaf)
-	    {
 	      next = grub_be_to_cpu32 (EXTNODE (tree, i)->data);
 	      break;
 	    }
@@ -451,7 +444,7 @@
 
  fail:
   grub_free (objc_data);
-  return 1;
+  return 0;
 }
 
 

Re: [PATCH] Corrections to affs and sfs

[Robert Millan]

On Sat, Nov 22, 2008 at 08:55:19PM +0100, Krzysztof Smiechowicz wrote:
> Hello,
> 
> I would like to submit the following patch:
> 
> * fs/affs.c: Return failure when directory iteration failed.
> 
> * fs/sfs.c: Return failure when directory iteration failed. Correct
> order in which btree nodes are read.
> 
> These changes are needed to boot AROS Research Operating System from SFS
> / FFS.

Hi,

> Index: fs/affs.c
> ===================================================================
> --- fs/affs.c	(revision 1919)
> +++ fs/affs.c	(working copy)
> @@ -381,7 +381,7 @@
>   fail:
>    grub_free (node);
>    grub_free (hashtable);
> -  return 1;
> +  return 0;
>  }

I checked in this hunk (and the equivalent sfs.c one)

> Index: fs/sfs.c
> ===================================================================
> --- fs/sfs.c	(revision 1919)
> +++ fs/sfs.c	(working copy)
> @@ -172,7 +172,8 @@
>  	  return grub_errno;
>  	}
>  
> -      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
> +      grub_uint16_t nodescount = grub_be_to_cpu16(tree->nodes);
> +      for (i = nodescount - 1; i >= 0; i--)

nodescount is only used once; why adding a variable?

>  	  /* Follow the tree down to the leaf level.  */
> -	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) >= block)
> +	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) <= block)
>  	      && !tree->leaf)
>  	    {
> -	      next = grub_be_to_cpu32 (EXTNODE (tree, i - 1)->data);
> -	      break;
> -	    }
> -
> -	  /* In case the last node is reached just use that one, it is
> -	     the right match.  */
> -	  if (i + 1 == grub_be_to_cpu16 (tree->nodes) && !tree->leaf)
> -	    {
>  	      next = grub_be_to_cpu32 (EXTNODE (tree, i)->data);
>  	      break;
>  	    }

I'm not familiar with our SFS code.  Marco, if you have a minute could you
review this part?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Corrections to affs and sfs

[Krzysztof Smiechowicz]

Hello,

Robert Millan pisze:

>> Index: fs/sfs.c
>> ===================================================================
>> --- fs/sfs.c	(revision 1919)
>> +++ fs/sfs.c	(working copy)
>> @@ -172,7 +172,8 @@
>>  	  return grub_errno;
>>  	}
>>  
>> -      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
>> +      grub_uint16_t nodescount = grub_be_to_cpu16(tree->nodes);
>> +      for (i = nodescount - 1; i >= 0; i--)
> 
> nodescount is only used once; why adding a variable?

No particular reason. Feel free to correct it :)

Best regards,
Krzysztof

Re: [PATCH] Corrections to affs and sfs

[Krzysztof Smiechowicz]

Robert Millan pisze:

>> Index: fs/sfs.c
>> ===================================================================
>> --- fs/sfs.c	(revision 1919)
>> +++ fs/sfs.c	(working copy)
>> @@ -172,7 +172,8 @@
>>  	  return grub_errno;
>>  	}
>>  
>> -      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
>> +      grub_uint16_t nodescount = grub_be_to_cpu16(tree->nodes);
>> +      for (i = nodescount - 1; i >= 0; i--)
> 
> nodescount is only used once; why adding a variable?
> 
>>  	  /* Follow the tree down to the leaf level.  */
>> -	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) >= block)
>> +	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) <= block)
>>  	      && !tree->leaf)
>>  	    {
>> -	      next = grub_be_to_cpu32 (EXTNODE (tree, i - 1)->data);
>> -	      break;
>> -	    }
>> -
>> -	  /* In case the last node is reached just use that one, it is
>> -	     the right match.  */
>> -	  if (i + 1 == grub_be_to_cpu16 (tree->nodes) && !tree->leaf)
>> -	    {
>>  	      next = grub_be_to_cpu32 (EXTNODE (tree, i)->data);
>>  	      break;
>>  	    }
> 
> I'm not familiar with our SFS code.  Marco, if you have a minute could you
> review this part?
> 

Hello,

Any news about applying this patch? :)

Best regards,
Krzysztof
AROS Development Team

Re: [PATCH] Corrections to affs and sfs

[Robert Millan]

On Tue, Jan 06, 2009 at 06:44:46PM +0100, Krzysztof Smiechowicz wrote:
> Robert Millan pisze:
> 
> >>Index: fs/sfs.c
> >>===================================================================
> >>--- fs/sfs.c	(revision 1919)
> >>+++ fs/sfs.c	(working copy)
> >>@@ -172,7 +172,8 @@
> >> 	  return grub_errno;
> >> 	}
> >> 
> >>-      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
> >>+      grub_uint16_t nodescount = grub_be_to_cpu16(tree->nodes);
> >>+      for (i = nodescount - 1; i >= 0; i--)
> >
> >nodescount is only used once; why adding a variable?
> >
> >> 	  /* Follow the tree down to the leaf level.  */
> >>-	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) >= block)
> >>+	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) <= block)
> >> 	      && !tree->leaf)
> >> 	    {
> >>-	      next = grub_be_to_cpu32 (EXTNODE (tree, i - 1)->data);
> >>-	      break;
> >>-	    }
> >>-
> >>-	  /* In case the last node is reached just use that one, it is
> >>-	     the right match.  */
> >>-	  if (i + 1 == grub_be_to_cpu16 (tree->nodes) && !tree->leaf)
> >>-	    {
> >> 	      next = grub_be_to_cpu32 (EXTNODE (tree, i)->data);
> >> 	      break;
> >> 	    }
> >
> >I'm not familiar with our SFS code.  Marco, if you have a minute could you
> >review this part?
> >
> 
> Hello,
> 
> Any news about applying this patch? :)

Since Marco (who wrote that file) is not currently active, and nobody else
seems to be able to test and/or proofread it, we could just trust that you
know what you're doing.

Please, could you resend the patch without the unneeded 'nodescount' and
a ChangeLog entry?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Corrections to affs and sfs

[Krzysztof Smiechowicz]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

> Hello,
> 
> Any news about applying this patch? :)
> 

Hello,

I'm resubmitting the updated patch.

Changelog:

2009-02-11 Krzysztof Smiechowicz <deadwood@wp.pl>
	* fs/sfs.c (grub_sfs_read_extent) : Correction to traversing 		extent 
b-tree


[-- Attachment #2: sfs.c.diff --]
[-- Type: text/x-diff, Size: 956 bytes --]

Index: fs/sfs.c
===================================================================
--- fs/sfs.c	(wersja 1940)
+++ fs/sfs.c	(kopia robocza)
@@ -172,7 +172,7 @@
 	  return grub_errno;
 	}
 
-      for (i = 0; i < grub_be_to_cpu16 (tree->nodes); i++)
+      for (i = grub_be_to_cpu16(tree->nodes) - 1; i >= 0; i--)
 	{
 
 #define EXTNODE(tree, index)						\
@@ -180,17 +180,9 @@
 					 + (index) * (tree)->nodesize))
 
 	  /* Follow the tree down to the leaf level.  */
-	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) >= block)
+	  if ((grub_be_to_cpu32 (EXTNODE(tree, i)->key) <= block)
 	      && !tree->leaf)
 	    {
-	      next = grub_be_to_cpu32 (EXTNODE (tree, i - 1)->data);
-	      break;
-	    }
-
-	  /* In case the last node is reached just use that one, it is
-	     the right match.  */
-	  if (i + 1 == grub_be_to_cpu16 (tree->nodes) && !tree->leaf)
-	    {
 	      next = grub_be_to_cpu32 (EXTNODE (tree, i)->data);
 	      break;
 	    }