Re: Bugs and tasks for 2.02[~rc1]

[Peter Jones]

On Tue, Mar 08, 2016 at 08:57:16PM +0300, Andrei Borzenkov wrote:
> 07.03.2016 22:00, Peter Jones пишет:
> ...
> > 
> >>> 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.
> >>
> 
> It looks like there are quite a lot of different patches floating
> around. We have one in next branch; I just sent some cleanup against
> this code and would be interested in review. I would also appreciate if
> we all could settle on a single version, hopefully what we (will) have
> in next.

Okay.

>> >>> 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.
> > 
> 
> As long as EFI is the only platform that has notion of "returning to
> menu" what is the value of code bloat for other platforms? We'd need
> then invent exit codes or options etc and it really is not worth it, at
> least now.

I guess that's a fair point, but I still think the semantics of "the
command to exit with an exit code isn't exit" is pretty weird.  Would
you have an objection to two implementations with some #ifdef things
going on to determine which you get at build time, to avoid the bloat on
other platforms?

> >>> e0bb91a Fix bzr's ignore artificats in .gitignore
> > 
> > Did you accidentally skip this one?
> >
> 
> Not really but it is a lot of churn in something I do not really care
> about :) Could you split it in two - alpha sorting and actual changes,
> this makes it easier to see what is changed.

Okay, will do.

> >>> 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? 
> 
> Huh? The about quote is from the first file. How can "err" be unused if
> it is assigned and checked on the next line?

Okay, clearly I got confused there because the other file /was/ the
problem I described.  But the same actually applies to this one as well,
so I'm going to move it out of this branch as well.  Basically I've got
some UI patches I'm carrying from our desktop team that accomplish what
they wanted to visually, but they're not really very good patches, which
is why I didn't push those.  One of them removes the error printing
between initializing locale_dir and secondary_locale_dir.  But that
patch isn't something I think is suitable for upstream as it is written,
and I didn't realize this patch depended on that one.

Sorry about that.  I've folded them together in my tree now, so that
I'll remember to fix that when I'm cleaning those up.

> > 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.
> > 
> 
> There is.
> 
> address@hidden:~/src/systemd$ pkg-config --variable
> completionsdir bash-completion
> /usr/share/bash-completion/completions
> 
> Could you add configure.ac option that checks for it and defaults to
> current value?

Gah, it's in bash-completion not in bash.  Well, anyway: sure thing,
I'll fix up configure.ac, though I'm not that well versed in autoconf.
Currently what I have is here, and I welcome any feedback:
https://github.com/vathpela/grub2-fedora/commit/04844de3eb04f

Note that I've built that version but not actually tested it yet.

> >>> 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
> 
> Currently you can have
> 
> fallback="1 2 3 4"
> 
> Your patch treats full value of "fallback" as a single title.
> 
> > 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.
> 
> We usually accept ids everywhere numbers or titles are accepted; and ids
> can quite sensibly be split in multiple entries.
> 
> >  FWIW the documentation *doesn't* say
> > that it allows multiple entries, but *does* say, more or less by
> > accident, that you can use titles:
> 
> We can also fix documentation :)
> 
> We usually favor ids over names or titles. But that also can become
> pretty much hairy with submenus. So I think this needs some discussion
> about design.

Alright; I've moved it to my -wip branch.  At the same time I've updated
it to address your concern, I think, so if you want to have a look:
https://github.com/vathpela/grub2-fedora/commit/8c93aae4e

> >>> 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?
> 
> This came up before and I personally am fine with generic function that
> checks boolean value (C version is needed as well). Something for post-2.02.

Agreed.

> >>> 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.
> > 
> 
> Not really. Linux UUIDs cannot be used without initrd, so if you have
> monolithic kernel without initrd you cannot really use UUIDs for root.
> So you can disable them in this case.
> 
> It really needs some more details about problem it was intended to solve.

Original bug is here:
https://bugzilla.redhat.com/show_bug.cgi?id=1027833 .  I'm not
personally very invested in this one, except inasmuch as it's in my
tree and I'd like that diff to get smaller over time... :)

-- 
  Peter