Re: [PATCH 6/9] btrfs: Refactor the code that read from disk

[Daniel Kiper]

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 <address@hidden>
> ---
>  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