From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fTRWE-0000LW-5W for mharc-grub-devel@gnu.org; Thu, 14 Jun 2018 08:38:26 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fTRWC-0000LE-1f for grub-devel@gnu.org; Thu, 14 Jun 2018 08:38:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fTRW6-0002rk-C5 for grub-devel@gnu.org; Thu, 14 Jun 2018 08:38:24 -0400 Received: from boksu.net-space.pl ([185.15.1.105]:38028) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1fTRW6-0002qW-0S for grub-devel@gnu.org; Thu, 14 Jun 2018 08:38:18 -0400 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1844678AbeFNMiP (ORCPT ); Thu, 14 Jun 2018 14:38:15 +0200 Date: Thu, 14 Jun 2018 14:38:15 +0200 From: Daniel Kiper To: The development of GNU GRUB Cc: Goffredo Baroncelli Subject: Re: [PATCH 6/9] btrfs: Refactor the code that read from disk Message-ID: <20180614123815.GF6436@router-fw-old.local.net-space.pl> References: <20180603185348.1780-1-kreijack@inwind.it> <20180603185348.1780-7-kreijack@inwind.it> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180603185348.1780-7-kreijack@inwind.it> User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jun 2018 12:38:25 -0000 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 > --- > 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