Re: [PATCH 4/4] xfs: V5 filesystem format support

[Andrei Borzenkov]

В Tue, 12 May 2015 15:47:40 +0200
Jan Kara <address@hidden> пишет:

> > > +{
> > > + return ((grub_uint8_t *)(de + 1)) + de->len - 1 +
> > The outer parens are somehow confusing.
> 
>   I can remove them but I find it better to explicitely show where the
> typecast happens with parenthesis...
> 

OK

> > +
> > > +static grub_uint64_t *
> > > +grub_xfs_btree_keys(struct grub_xfs_data *data,
> > > +             struct grub_xfs_btree_node *leaf)
> > > +{
> > > +  char *p = (char *)(leaf + 1);
> > > +
> > > +  if (data->hascrc)
> > > +    p += 48;     /* crc, uuid, ... */
> > > +  /*
> > > +   * We have to first type to void * to avoid complaints about possible
> > > +   * alignment issues on some architectures
> > > +   */
> > > +  return (grub_uint64_t *)(void *)p;
> > 
> > Leaving it as grub_uint64_t keys and using &keys[6] would avoid this
> > warning as well, not? Also having keys[0] will likely simplify other
> > places as well (we do require GCC in any case).
> 
> Well, the trouble with this is that we'd need two structures defined -
> one for crc-enabled fs and one for old fs. That seemed like a wasted effort
> to me when we could do:
>   if (data->hascrc)
>     p += 48;  /* crc, uuid, ... */
> like the above. The same holds for inodes, directory entries, etc. I'd
> prefer not to bloat the code with structure definitions we don't actually
> use but if you really insisted, I could do that. So what do you think?

Why 2 structures? What I actually meant was

struct grub_xfs_btree_node
{
  grub_uint8_t magic[4];
  grub_uint16_t level;
  grub_uint16_t numrecs;
  grub_uint64_t left;
  grub_uint64_t right;
  grub_uint64_t keys[0];
}  GRUB_PACKED;

if (data->hascrc)
  return &leaf->keys[6]
else
  return &leaf->keys[0]

with suitable comment. It is not perfect either but at least leaves
compiler check in place.