On Wed, Sep 26, 2018 at 09:55:57PM +0200, Goffredo Baroncelli wrote: > On 25/09/2018 21.10, Daniel Kiper wrote: > > On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote: > >> From: Goffredo Baroncelli <kreij...@inwind.it> > >> > >> Add support for recovery for a RAID 5 btrfs profile. In addition > >> it is added some code as preparatory work for RAID 6 recovery code. > >> > >> Signed-off-by: Goffredo Baroncelli <kreij...@inwind.it> > >> --- > >> grub-core/fs/btrfs.c | 169 +++++++++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 164 insertions(+), 5 deletions(-) > >> > >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > >> index 5c1ebae77..55a7eeffc 100644 > >> --- a/grub-core/fs/btrfs.c > >> +++ b/grub-core/fs/btrfs.c > >> @@ -29,6 +29,7 @@ > >> #include <minilzo.h> > >> #include <grub/i18n.h> > >> #include <grub/btrfs.h> > >> +#include <grub/crypto.h> > >> > >> GRUB_MOD_LICENSE ("GPLv3+"); > >> > >> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, > >> return err; > >> } > >> > >> +struct raid56_buffer { > >> + void *buf; > >> + int data_is_valid; > >> +}; > >> + > >> +static void > >> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, > >> + grub_uint64_t nstripes, grub_uint64_t csize) > >> +{ > >> + grub_uint64_t i; > >> + int first; > >> + > >> + i = 0; > >> + while (buffers[i].data_is_valid && i < nstripes) > >> + ++i; > > > > for (i = 0; buffers[i].data_is_valid && i < nstripes; i++); > > > >> + if (i == nstripes) > >> + { > >> + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are > >> OK\n"); > >> + return; > >> + } > >> + > >> + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T > >> "\n", > >> + i); > > > > One line here please. > > > >> + for (i = 0, first = 1; i < nstripes; i++) > >> + { > >> + if (!buffers[i].data_is_valid) > >> + continue; > >> + > >> + if (first) { > >> + grub_memcpy(dest, buffers[i].buf, csize); > >> + first = 0; > >> + } else > >> + grub_crypto_xor (dest, dest, buffers[i].buf, csize); > >> + > >> + } > > > > Hmmm... I think that this function can be simpler. You can drop first > > while/for and "if (i == nstripes)". Then here: > > > > if (first) { > > grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); > > > > Am I right? > > Ehm.. no. The "if" is an internal check to avoid BUG. rebuild_raid5() should > be called only if some disk is missed. > To perform this control, the code checks if all buffers are valid. Otherwise > there is an internal BUG.
Something is wrong here. I think that the code checks if it is an invalid buffer. If there is not then GRUB complains. Right? However, it looks that I misread the code and made a mistake here. So, please ignore this change. Though please change while() with for() at the beginning. Daniel