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

[Jan Kara <jack@suse.cz>]

On Wed 13-05-15 07:50:38, Andrei Borzenkov wrote:
> В Tue, 12 May 2015 15:47:40 +0200
> Jan Kara <jack@suse.cz> пишет:
> > > +
> > > > +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.
  Ah, I see. I probably won't call it 'keys' but something like 'data' and
add a comment explaining what's going on.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR