On Sun, Jun 03, 2018 at 08:53:45PM +0200, Goffredo Baroncelli wrote: > Move the code in charge to read the data from disk in a separate
s/in a/into a/ > function. This helps to separate the error handling logic (which depend by s/depend by/depends on/ Please fix this here and in other patches too. > the different raid profiles) from the read from disk logic. > Refactoring this code increases the general readability too. > > This is a preparatory patch, to help the adding of the RAID 5/6 recovery > code. > > Signed-off-by: Goffredo Baroncelli <kreij...@inwind.it> > --- > grub-core/fs/btrfs.c | 76 ++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 31 deletions(-) > > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > index e2baed665..9cdbfe792 100644 > --- a/grub-core/fs/btrfs.c > +++ b/grub-core/fs/btrfs.c > @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t > id) > return ctx.dev_found; > } > > +static grub_err_t > +btrfs_read_from_chunk (struct grub_btrfs_data *data, > + struct grub_btrfs_chunk_item *chunk, > + grub_uint64_t stripen, grub_uint64_t stripe_offset, > + int redundancy, grub_uint64_t csize, > + void *buf) > +{ > + Please drop this empty line. > + struct grub_btrfs_chunk_stripe *stripe; > + grub_disk_addr_t paddr; > + grub_device_t dev; > + grub_err_t err; > + > + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > + /* Right now the redundancy handling is easy. > + With RAID5-like it will be more difficult. */ > + stripe += stripen + redundancy; > + > + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; > + > + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T > + " maps to 0x%" PRIxGRUB_UINT64_T "\n", > + stripen, stripe->offset); > + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", > paddr); Could you put this into one grub_dprintf() call? > + > + dev = find_device (data, stripe->device_id); > + if (!dev) > + { > + grub_dprintf ("btrfs", > + "couldn't find a necessary member device " > + "of multi-device filesystem\n"); > + grub_errno = GRUB_ERR_NONE; > + return GRUB_ERR_READ_ERROR; Why grub_errno = GRUB_ERR_NONE and then return GRUB_ERR_READ_ERROR? If you do things like that I think that you should add a few words of comment before such code. Otherwise it is confusing. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel