Re: Bugs and tasks for 2.02[~rc1]

[Andrei Borzenkov]

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.

...
>>
>>> 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

I screwed something up, sorry. Looks sane.

...
> 
>>> 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.

...

> 
>>> 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.


>>> 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?

> 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?

>>> 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.
...
>>
>>> 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.

> 
>>> 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.

> 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.
> 
...