Re: [PATCH v4 12/13] error: Use format code llu for 64-bit uint bp->blk_prop in grub_error

[Glenn Washburn]

On Wed, 3 Mar 2021 19:17:31 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sun, Feb 28, 2021 at 03:10:52PM -0600, Glenn Washburn wrote:
> > On Sat, 27 Feb 2021 13:05:09 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Thu, Feb 18, 2021 at 08:47:13PM -0600, Glenn Washburn wrote:
> > > > For some reason PRIuGRUB_UINT64_T is not expanding to llu, but
> > > > to lu, which causes the format string check to fail. Use
> > > > literal and force cast until this is debugged.
> > >
> > > I think the problem is that currently BF64_DECODE() uses "1ULL". I
> > > think it should look like this
> > >
> > >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low),
> > > ((grub_uint64_t) 1) << (len))
> > >
> > > instead of
> > >
> > >   #define BF64_DECODE(x, low, len) P2PHASE((x) >> (low), 1ULL <<
> > > (len))
> > >
> > > Same or similar for other macros there.
> > >
> > > I would prefer if you fix macros in include/grub/zfs/spa.h first
> > > and then do proper fix here.
> >
> > Yep, completely agree, just hadn't figured out a proper solution.
> > And not being familiar with the code and not doing the zfs
> > functional tests (GitLab CI shared runners do not have zfs kernel
> > support), I'm hesitant to make such intrusive changes. However,
> > your suggestion is the better way to do it and I've confirmed that
> > it work on i386 and x86_64. I'll update the patch in the next patch
> > series.
> 
> I am still afraid doing that before release until we are able to
> confirm that everything works for 32-bit and 64-bit architectures. If
> not I would not merge this patch and next one at this point.

Is there anyone here that can do this testing?

I does seem a little ridiculous that patch #13 would be blocked by zfs,
since its generally useful for the whole code base. My presumption
based on experience here is that if I'm not here to keep pushing for
#13, it will get forgotten about. This is why I'd like to get it in
now. Patch #12 may not be ideal, but its certainly an improvement. I
think it makes sense to include it for now, so that #13 can be
merged. Then at a later date when zfs testing can be done, the better
fix can be done.

Glenn