[PATCH] Fix warning in fs/xfs.c

[PATCH] Fix warning in fs/xfs.c

[Pavel Roskin]

ChangeLog:
	* fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
	those in Linux XFS code.  Provide a way to access 64-bit parent
	inode.
	(grub_xfs_iterate_dir): Use the new names.  Avoid reading past
	the end of struct grub_xfs_dir_header.
---

 fs/xfs.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/xfs.c b/fs/xfs.c
index 54d8031..7da3e40 100644
--- a/fs/xfs.c
+++ b/fs/xfs.c
@@ -55,9 +55,13 @@ struct grub_xfs_sblock
 
 struct grub_xfs_dir_header
 {
-  grub_uint8_t entries;
-  grub_uint8_t smallino;
-  grub_uint32_t parent;
+  grub_uint8_t count;
+  grub_uint8_t i8count;
+  union
+  {
+    grub_uint32_t i4;
+    grub_uint64_t i8;
+  } parent __attribute__ ((packed));
 } __attribute__ ((packed));
 
 struct grub_xfs_dir_entry
@@ -419,7 +423,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
     case XFS_INODE_FORMAT_INO:
       {
 	struct grub_xfs_dir_entry *de = &diro->inode.data.dir.direntry[0];
-	int smallino = !diro->inode.data.dir.dirhead.smallino;
+	int smallino = !diro->inode.data.dir.dirhead.i8count;
 	int i;
 	grub_uint64_t parent;
 
@@ -427,12 +431,12 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	   parent inode number is small too.  */
 	if (smallino)
 	  {
-	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent);
+	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
 	    parent = grub_cpu_to_be64 (parent);
 	  }
 	else
 	  {
-	    parent = *(grub_uint64_t *) &diro->inode.data.dir.dirhead.parent;
+	    parent = diro->inode.data.dir.dirhead.parent.i8;
 	    /* The header is a bit bigger than usual.  */
 	    de = (struct grub_xfs_dir_entry *) ((char *) de + 4);
 	  }
@@ -444,7 +448,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	if (call_hook (parent, ".."))
 	  return 1;
 
-	for (i = 0; i < diro->inode.data.dir.dirhead.entries; i++)
+	for (i = 0; i < diro->inode.data.dir.dirhead.count; i++)
 	  {
 	    grub_uint64_t ino;
 	    void *inopos = (((char *) de)

Re: [PATCH] Fix warning in fs/xfs.c

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> ChangeLog:
> 	* fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
> 	those in Linux XFS code.  Provide a way to access 64-bit parent
> 	inode.
> 	(grub_xfs_iterate_dir): Use the new names.  Avoid reading past
> 	the end of struct grub_xfs_dir_header.

*please* do not look at Linux code or whatever *and* contribute to
 GRUB.  It might cause copyright troubles I will have to deal with :-/

I do not see the advantage of this patch.  Can you please explain why
we need these name changes?

--
Marco

Re: [PATCH] Fix warning in fs/xfs.c

[Pavel Roskin]

On Thu, 2008-07-03 at 20:21 +0200, Marco Gerards wrote:
> Pavel Roskin <proski@gnu.org> writes:
> 
> > ChangeLog:
> > 	* fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
> > 	those in Linux XFS code.  Provide a way to access 64-bit parent
> > 	inode.
> > 	(grub_xfs_iterate_dir): Use the new names.  Avoid reading past
> > 	the end of struct grub_xfs_dir_header.
> 
> *please* do not look at Linux code or whatever *and* contribute to
>  GRUB.  It might cause copyright troubles I will have to deal with :-/

I just tried to make names similar without copying any code.  But it's a
useful reminder.

> I do not see the advantage of this patch.  Can you please explain why
> we need these name changes?

We were casting a pointer to a 32-bit integer to a pointer to a 64-bit
integer, which is bad, and gcc was emitting a warning about it.

Worse yet, the 64-bit value was "sticking" beyond the end the structure
we were using to describe the header.

i4 and i8 are generally used by Linux XFS code to describe 32-bit and
64-bit values if either can be used.  The "smallino" field was highly
misleading because it had to be negated.  It's the number of "big" (i8
or 64-bit) entries.  If it's 0, then the entries are "small".

So it was natural to call it "i8count".  And once it was "i8count", it
was natural to call the first value "count".

If you prefer another naming convention, let's rename the entries
according to it.  I was thinking having 2 32-bit integers "parent_hi"
and "parent_lo" or something like that.  Anyway, let's not use
"smallino" - "bigentries" would be better.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix warning in fs/xfs.c

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Thu, 2008-07-03 at 20:21 +0200, Marco Gerards wrote:
>> Pavel Roskin <proski@gnu.org> writes:
>> 
>> > ChangeLog:
>> > 	* fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
>> > 	those in Linux XFS code.  Provide a way to access 64-bit parent
>> > 	inode.
>> > 	(grub_xfs_iterate_dir): Use the new names.  Avoid reading past
>> > 	the end of struct grub_xfs_dir_header.
>> 
>> *please* do not look at Linux code or whatever *and* contribute to
>>  GRUB.  It might cause copyright troubles I will have to deal with :-/
>
> I just tried to make names similar without copying any code.  But it's a
> useful reminder.

What I meant is that even *looking* at code might cause problems.
People can claim you have stolen their ideas.  That would essentially
mean the same as copying code.  I just want to avoid such problems at
beforehand.

>> I do not see the advantage of this patch.  Can you please explain why
>> we need these name changes?
>
> We were casting a pointer to a 32-bit integer to a pointer to a 64-bit
> integer, which is bad, and gcc was emitting a warning about it.

Right

> Worse yet, the 64-bit value was "sticking" beyond the end the structure
> we were using to describe the header.
>
> i4 and i8 are generally used by Linux XFS code to describe 32-bit and
> 64-bit values if either can be used.  The "smallino" field was highly
> misleading because it had to be negated.  It's the number of "big" (i8
> or 64-bit) entries.  If it's 0, then the entries are "small".
>
> So it was natural to call it "i8count".  And once it was "i8count", it
> was natural to call the first value "count".
>
> If you prefer another naming convention, let's rename the entries
> according to it.  I was thinking having 2 32-bit integers "parent_hi"
> and "parent_lo" or something like that.  Anyway, let's not use
> "smallino" - "bigentries" would be better.

What I suggest is that you pick the names yourself or from a standard,
instead of from Linux code.

--
Marco

Re: [PATCH] Fix warning in fs/xfs.c

[Pavel Roskin]

On Sun, 2008-07-20 at 20:50 +0200, Marco Gerards wrote:
> Pavel Roskin <proski@gnu.org> writes:
> > i4 and i8 are generally used by Linux XFS code to describe 32-bit and
> > 64-bit values if either can be used.  The "smallino" field was highly
> > misleading because it had to be negated.  It's the number of "big" (i8
> > or 64-bit) entries.  If it's 0, then the entries are "small".
> >
> > So it was natural to call it "i8count".  And once it was "i8count", it
> > was natural to call the first value "count".
> >
> > If you prefer another naming convention, let's rename the entries
> > according to it.  I was thinking having 2 32-bit integers "parent_hi"
> > and "parent_lo" or something like that.  Anyway, let's not use
> > "smallino" - "bigentries" would be better.
> 
> What I suggest is that you pick the names yourself or from a standard,
> instead of from Linux code.

OK, I'll try to be more creative.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix warning in fs/xfs.c

[Cesare Leonardi]

Marco Gerards wrote:
> Pavel Roskin <proski@gnu.org> writes:
> 
>> On Thu, 2008-07-03 at 20:21 +0200, Marco Gerards wrote:
>>> Pavel Roskin <proski@gnu.org> writes:
>>>
>>>> ChangeLog:
>>>> 	* fs/xfs.c (struct grub_xfs_dir_header): Use names similar to
>>>> 	those in Linux XFS code.  Provide a way to access 64-bit parent
>>>> 	inode.
>>>> 	(grub_xfs_iterate_dir): Use the new names.  Avoid reading past
>>>> 	the end of struct grub_xfs_dir_header.
>>> *please* do not look at Linux code or whatever *and* contribute to
>>>  GRUB.  It might cause copyright troubles I will have to deal with :-/
>> I just tried to make names similar without copying any code.  But it's a
>> useful reminder.
> 
> What I meant is that even *looking* at code might cause problems.
> People can claim you have stolen their ideas.  That would essentially
> mean the same as copying code.  I just want to avoid such problems at
> beforehand.

But one of the best aspect of the free software is exactly that you 
could look inside other's code and reuse it and its ideas.
Would be a problem if you look the source of a program covered by NDA, 
or by some other closed license, but with Linux kernel?? If you couldn't 
look inside a GPL2 project then it would be a loss of the free software 
and the GPL3. And you have to reinvent something that others has already 
done well...

Furthermore, AFAIK, the problem of protecting code implementation is 
covered by copyright, while protecting ideas is a patent job. Now, Linux 
is GPL2 and it's not encumbered by patents, so what's the problem?

Regards.

Cesare.