Re: About the code style requirement

[Qu Wenruo]

On 2021/10/8 08:22, Qu Wenruo via Grub-devel wrote:
> On 2021/10/7 21:51, Daniel Kiper wrote:
> > On Fri, Oct 01, 2021 at 03:14:02PM +0800, Qu Wenruo via Grub-devel wrote:
> > > On 2021/10/1 14:08, Vladimir 'phcoder' Serbinenko wrote:
> >
> > [...]
> >
> > > > GRUB already has a btrfs implementation. Writing new one from scratch
> > > > instead of existing one is unwelcome. From scratch means new bugs.
> > > > Do you have problems with existing implementation?
> > >
> > > In fact, quite some:
> > >
> > > - Can't read shared data extents of reflinked files
> > >    This can be fixed without a big refactor.
> > >
> > > - No csum verification
> > > - No support for extra checksums (xxhash/sha256/blake2)
> > > - No very basic tree block sanity check
> > >    The only sanity check is bytenr and fsid, which is far from ideal.
> > >    The most deadly case like parent-child tree block level mismatch is
> > >    not handled at all.
> > >    (which can easily cause a dead loop,
> > >     e.g. tree_block1 -> tree_block2 -> tree_block 1)
> > >
> > > > If so I prefer you
> > > > to fix the exact bugs rather than attempt to rewrite the entire thing
> > > > from scratch
> > >
> > > If only for the read failure of the reflinked files, I'd be pretty happy
> > > just to fix that bug.
> > >
> > > But for the long run, I really prefer to have a more-or-less common base
> > > which every btrfs developer can easily start their work easily.
> > >
> > > Your work on the existing grub btrfs code is very appreciated, but 
> > > I'm not
> > > sure if it is the best solution in the long run.
> > >
> > > Thus please let us btrfs developers to contribute our experience to 
> > > make a
> > > better and more unified implementation.
> > > (Although still less unified due to license conflicts)
> >
> > I think it would be nice to have a kind of library which could be
> > imported into the GRUB source code in the similar way how it is done
> > e.g. for gnulib or JSON. Please take a look at grub-core/lib directory
> > and e.g. commit 528938d50 (json: Import upstream jsmn-1.1.0). Of course
> > it has to have compatible license with the GRUB. You can find more info
> > about licensing here [1].
>
> Yep, it would be the best practice to import a good btrfs implementation 
> directly to GRUB.
>
> But as you know, currently we don't have such lib which is license 
> compatible yet.
>
> > WRT coding style, it is described here [2]. As Daniel A. said it is GNU
> > coding style with some (minor) exceptions. However, we do not enforce
> > this coding style for the code which is imported into the GRUB as 
> > libraries. >
> > Anyway, we want to have support for latest Btrfs features in the GRUB.
> > So, I think your proposal is interesting. However, we have to get more
> > details how to solve various problems. So, if you could consider various
> > options and present their pros and cons then I think we can continue
> > discussion how to make Btrfs development for GRUB easier.
>
> Currently there seems to be 3 different plans:
>
> 1. Continue with current GRUB btrfs code base
>
>     Pros: No need for a new project/code base
>
>     Cons: Very few (if any) btrfs developers is interested in handling a
>           completely different code base.
>           Thus the code can easily (and is already) get out-of-sync with
>           latest btrfs features.

I pinned down the bug why GRUB fails to read some extents.

It turns out to be a much bigger problem, that GRUB can't even handle 
the soon-to-be-default features like NO_HOLES.
(And mixed inline with regular extents)

Sure, I will send out patches for that bug, but it really takes me too 
long time just to pin down the bug inside a completely different code base.

Thanks,
Qu

> 2. Import code from U-boot
>
>     Pros: The most unified code base (directly from btrfs-progs/kernel).
>           More or less tested (only two bugs found after the cross-port
>           to U-boot).
>           Mostly feature rich (multi-device not implemented due to U-boot
>           limit, lacking only blake2 hash support)
>
>     Cons: License incompatible. The btrfs-progs code may be re-licensed,
>           but there are still things like rbtree and list are copied from
>           kernel, thus a full re-license seems unfeasible.
>
> 3. New btrfs FUSE based project <<< Trying to do in the long run
>
>     Pros: More btrfs community members can get involved.
>           Easier to test thanks to FUSE.
>           License compatible (MIT planned).
>           Extra educational purpose for new developers.
>
>     Cons: A new project means new bugs.
>           Still a very different code base, thus not that unified to
>           btrfs-progs/kernel.
>           Need extra infrastructure/interface to be cross-ported to GRUB.
>
> > Daniel
> >
> > [1] https://www.gnu.org/licenses/licenses.html.en
> > [2] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html
> Thanks for all the info, it would definitely help when I start coding 
> inside GRUB.
>
> Thanks,
> Qu
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel