В Tue, 12 May 2015 15:47:40 +0200
Jan Kara <j...@suse.cz> пишет:
> > > +{
> > > + 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.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
