Re: Bugs and tasks for 2.02[~rc1]

[Peter Jones]

On Sat, Mar 05, 2016 at 11:38:00AM +0300, Andrei Borzenkov wrote:
> 04.03.2016 23:06, Peter Jones пишет:
> > On Wed, Mar 02, 2016 at 03:01:03PM +0000, Vladimir 'phcoder' Serbinenko 
> > wrote:
> >> Hello, all. I went through the list of bugs and created a shortlist of bugs
> >> that need to be looked at for 2.02. I have marked them with plan_release_id
> >> set to 2.02.
> >> Statistics: [1]
> >> Search (with loads of false positives unfortunately): [2]
> >> Not every bug there is a release blocker, for some of them it just would be
> >> nice to know status before releasing. Some of them are probably already
> >> fixed.
> >>
> >> Additionally I created a category "Hardware specific" [3]. Bugs there are
> >> not release blockers but fixing them could benefit the release.
> > 
> > In the interest of fixing them up eventually, here's a chunk
> > of ones that look reasonably well-suited for upstream without much work
> > on them, which I've rebased against your master branch today.
> > 
> > https://github.com/vathpela/grub2-fedora/tree/for-upstream
> > 
> > Most of these are not critical for this release - really only 3 of them.
> > Here are some notes on each; I can send them individually to the list if
> > you think it's worthwhile.
> > 
> > There are only a couple that are "critical", and we really want in this
> > release:
> > 
> > bf4d216 Fix crash on http
> 
> Fixed differently in 92c8f58d973a621890e302cba3d80fe0bbc208d7

Oh, thanks, I missed that.

> > 78b3509 Update to minilzo-2.08
> 
> I know. But the sheer size of update makes me afraid of doing it now.

Yeah, I see that concern, I'm just worried there's a CVE here we're
missing.  FWIW we've been carrying that patch for 15 months or so
without seeing any issue whatsoever, but then we're not building for
more extravagant platforms - it's really just the x86, arm, and ppc
families.

> > eaa05aa Failed config now returns exit code (#1252311)
> 
> I think it makes sense.
> 
> > Then these are just generic network handling patches:
> > 
> > 836b528 DHCP client ID and UUID options added.
> 
> Use case would be interesting. You can query arbitrary option already.
> 
> > eb1adf5 trim arp packets with abnormal size
> > 
> 
> nb is not used anywhere in this branch, so I do not understand what this
> code does at all.

Fair enough; I'll move these two for later and try to figure them out.
I think I do have another patch that uses the DHCP one that I didn't put on
this list, but it's completely likely that I'm just carrying an old
patch that's been supplanted by something else.

> > And these are hardware specific.  They're not critical, in the sense
> > that I can keep carrying them if you have any problems and we can work
> > it out for the /next/ release.
> > 
> > beee9fc Add vlan-tag support on IBM PPC machines
> 
> Yes, openSUSE (and I think SUSE) also carries variant of this patch. We
> probably need to revisit it.
> 
> > 93a6fae IBM client architecture (CAS) reboot support
> 
> Is in (open)SUSE as well
> 
> > 297d32d for ppc, reset console display attr when clear screen
> 
> Does not apply cleanly to upstream (is on top of some other patch?)

I'm not seeing any reason this should not apply.  I think probably the
literal nonprintable ^L in the string breaks "git am" (or "git apply"),
and if you "git fetch" + "git cherry-pick" it works?  Or add the file to
.gitattributes as:

grub-core/term/terminfo.c binary

And then do "git format-patch $commitid" and apply that result.  But
that's a blunt enough hammer that it's not something we really want to
/commit/ for a .c file, I would think, since it makes reading the
patches difficult.  Also you have to fetch the remote anyway, so
cherry-pick makes more sense.

At the same time, we probably ought to change it to \x0c instead of the
^L.  So I've done that in my current version of the patch, but it won't
help this time.

> > 0ca5375 Disable GRUB video support for IBM power machines
> 
> Is in (open)SUSE as well
> 
> > 2f3c666 Add support for UEFI operating systems returned by os-prober
> 
> We already support it for quite some time, although differently (based
> on os-prober support).

Ah, yeah, looks like f25870887b7.  Thanks.

> > 05f2dc3 Make efi machines load an env block from a variable
> 
> This needs discussion. As well, as openSUSE "store environment block in
> btrfs reserved area" patch. And there was intention to use PV reserved
> areas for it as well.

Completely fair.  I picked an EFI variable because I was using this to
control some debugging code that's still in progress, and it's nice that
you can set it before the bootloader starts (or restore it using
dmpstore, etc.)

> > 1f1a695 Make "exit" take a return code.
> > 
> 
> What about
> https://lists.gnu.org/archive/html/grub-devel/2016-01/msg00049.html and
> followup?

Well, that's a good question.  The code in this patch is sort of a
middle ground there - it makes GRUB_EFI_LOAD_ERROR the default, and
makes "exit" and "exit N" both work.  So if you do "exit 0", you get no
fallback to the next item, but "exit" alone, "exit N" for any N but 0,
and all the failure paths in the C code all wind up showing the next
menu item.

I can make it another command if you like, but it seems like a pretty
natural semantic for exit to have.  So the issue is that it won't do
anything on a bunch of platforms other than what they're already doing.
Is that a big deal?  We have plenty of commands that perform a level of
functionality based on the underlying support.

> > The rest are all about the git repo and compilers and similar.  These
> > are well into "nice to have" for 2.02.  I can re-send them after the
> > release, they're just what's left on my list that's pretty close to
> > ready for upstream.
> > 
> > cb62c40 Mark po/exclude.pot as binary so git won't try to diff 
> > nonprintables.
> 
> Does it cause a problem? It does not look like there are many of them.

Well, in the glorious world where we have a tarball newer than
0d6498a67d4 it should cause a lot fewer problems :)  But if your
build does:

tar xf grub-2.02~beta2.tar.xz
cd grub-2.02~beta2
git init
git add .
git commit -a -m grub-2.02-beta2
git am <all the patches we want since then>

Then that last step fails because nonprintables in diff really do not
work well.  This patch makes "git format-patch" output that file's diff
as a binary diff instead, and so it works.

I wouldn't want to do this for a C file, but exclude.pot doesn't see a
lot of changes.  And it has a lot of junk in it to begin with - that's
what it exists for.  

Still, if we have actual regular releases, this isn't particularly
important.  It's just if distros (or users) wind up applying a pile of
patches from the repo to make their packages that it becomes an issue.

> > e0bb91a Fix bzr's ignore artificats in .gitignore

Did you accidentally skip this one?

> > ecaecc9 Add some __unused__ where gcc 5.x is more picky about it.
> 
> How this can become unused?
>
> >  grub_gettext_env_write_lang (struct grub_env_var *var
> >                          __attribute__ ((unused)), const char *val)
> >   {
> >  -  grub_err_t err;
> >  +  grub_err_t __attribute__((__unused__)) err;
> >     err = grub_gettext_init_ext (&main_context, val, grub_env_get 
> > ("locale_dir"),
> >                            grub_env_get ("prefix"));
> >     if (err)
> 
> And in normal, entry is used. So more detailed explanation how either of
> them become unused is needed (and BTW it is compiled with gcc 5.x on
> openSUSE and apparently without errors).

Yep, you're right - sorry about that, the last one should have been
stripped out - it's the artifact of another patch that's not really
upstreamable in its current form.  The whole first file looks valid to
me, though?  I'll rebase it as two patches and leave one of them in
for-upstream.

> > e704140 Move bash completion script (#922997)
> 
> Well, this is obvious compatibility question. Is there any way to detect
> it at configure time? Does bash have pkg-config or similar?

I don't see anything obviously like that, unfortunately, and I'm not
really sure in what version they switched it.

> > bc5d351 Allow "fallback" to include entries by title, not just number.
> 
> What about multiple entries? fallback allows them.

I'm not sure I understand your question.  This still allows that if you
treat them numerically or by id.  I suppose it's possible to break the
value up as a list of quoted strings to test by title, but it gets messy
with corner cases pretty quickly.  FWIW the documentation *doesn't* say
that it allows multiple entries, but *does* say, more or less by
accident, that you can use titles:
-------------------------------------------------
@node fallback
@subsection fallback

If this variable is set, it identifies a menu entry that should be
selected if the default menu entry fails to boot.  Entries are
identified in the same way as for @samp{default} (@pxref{default}).
-------------------------------------------------
and then:
-------------------------------------------------
@node default
@subsection default

If this variable is set, it identifies a menu entry that should be
selected by default, possibly after a timeout (@pxref{timeout}).  The
entry may be identified by number or by id.

For example, if you have:

@verbatim
menuentry 'Example GNU/Linux distribution' --class gnu-linux --id 
example-gnu-linux {
        ...
}
@end verbatim

then you can make this the default using:

@example
default=example-gnu-linux
@end example

If the entry is in a submenu, then it must be identified using the
titles of each of the submenus starting from the top level followed by
the number or title of the menu entry itself, separated by @samp{>}.
For example, take the following menu structure:

@example
Submenu 1
  Menu Entry 1
  Menu Entry 2
Submenu 2
  Submenu 3
    Menu Entry 3
    Menu Entry 4
  Menu Entry 5
@end example

``Menu Entry 3'' would then be identified as
@samp{Submenu 2>Submenu 3>Menu Entry 3}.
...
-------------------------------------------------
(mildly reformatted for mail purposes.)

So this patch actually brings it closer into conformance with the
documentation, but still allows multiple fallbacks by index or id to
work as they currently do.

> > 7401bf6 Honor a symlink when generating configuration by grub2-mkconfig
> 
> Makes sense
> 
> > 5212412 Fix bad test on GRUB_DISABLE_SUBMENU.
> 
> What is bad here? The documented valued is "y".

Actually the real issue is that we're inconsistent among many variables,
where we use (and document) "yes", "y", and "true".  So we discovered a
tendency to put the wrong thing in, and I got tired of it and made this
one take either of those.  Maybe this should be addressed more
systemically with a function to judge true or false that recognizes
1/true/y/yes as true?

> > 73545c7 Add GRUB_DISABLE_UUID.
> 
> If name as detected by GRUB is correct, there will be no search because
> hints will be correct (just direct verification that device is indeed
> correct). If name is wrong you need search, otherwise you fail to boot
> or boot wrong binary. I do not see what we gain here.

So, the bug report from our QA dept believed
GRUB_DISABLE_LINUX_UUID=true should accomplish this, and that it's
pointless without it.  And I think they've kind of got a point, since if
the user has the problem GRUB_DISABLE_LINUX_UUID was meant to solve,
there's no reason to believe they can't have the same problem with the
other filesystem.  We made them separate settings because one is about
/boot and one is about / , but fundamentally they're both doing parts of
the same thing.

I'm not really disagreeing with your logic here - but if we have it
avoiding UUIDs for one of those, it would seem like the other would also
be something we can avoid.

> >> I would also appreciate if distros would tell which patches they would
> >> carry if 2.02 was released as it is now. If some patches are in more than 1
> >> distro we probably need to look into including them.
> > 
> > Well, I have a bunch of patches that need to be clean up (or even
> > re-examined), and I've also got the secure-boot branch here:
> > 
> > https://github.com/vathpela/grub2-fedora/tree/sb
> > 
> > Which is all the patches distros should be carrying to work with Secure
> > Boot correctly.  This branch is also recently rebased against master,
> > though I'm not sure what the current thinking is regarding their path
> > upstream.
> > 
> 
> Personally I'd rather include support for it. I'm tired of linux vs.
> linuxefi nightmare, and patches have been in the wild long enough.

So what's the path forward, then?  Just make all efi use linuxefi, like
linux vs linux16?  That's pretty close to what I've got already, except
on arm where it's just "linux" in EFI mode as well.  But we could make
those aliases for the same thing on that platform easily enough.  Or do
you have something else in mind?

Anyway, I've moved the patches that clearly need some more work to a
branch called "for-upstream-wip", and fixed up the ones still in
"for-upstream" as I've mentioned above.

Thanks!

-- 
  Peter