[PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Michael Chang]

We don't insert gettext module if message catalog file missing to
prevent error message from being logged.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub.d/00_header.in |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
index bb34ef2..d438d52 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -182,10 +182,14 @@ EOF
 
 # Gettext variables and module
 if [ "x${LANG}" != "xC" ] ; then
+# We don't insert gettext module if message catalog file missing
+# To prevent error message from being logged (bnc#771393)
   cat << EOF
-  set locale_dir=\$prefix/locale
-  set lang=${grub_lang}
-  insmod gettext
+  if [ -f "\$prefix/locale/${grub_lang}.mo" ] ; then
+    set locale_dir=\$prefix/locale
+    set lang=${grub_lang}
+    insmod gettext
+  fi
 EOF
 fi
 
-- 
1.7.3.4

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Mads Kiilerich]

On 09/24/2012 08:51 AM, Michael Chang wrote:
> We don't insert gettext module if message catalog file missing to
> prevent error message from being logged.
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>   util/grub.d/00_header.in |   10 +++++++---
>   1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
> index bb34ef2..d438d52 100644
> --- a/util/grub.d/00_header.in
> +++ b/util/grub.d/00_header.in
> @@ -182,10 +182,14 @@ EOF
>   
>   # Gettext variables and module
>   if [ "x${LANG}" != "xC" ] ; then

Couldn't / sholdn't this check be replaced by the new check you introduce?

> +# We don't insert gettext module if message catalog file missing
> +# To prevent error message from being logged (bnc#771393)

That seems like a reference to some (internal Suse?) bugtracker? To me 
it is https://bugzilla.redhat.com/show_bug.cgi?id=817187 , but I guess 
https://savannah.gnu.org/bugs/?35880 is the best reference.

>     cat << EOF
> -  set locale_dir=\$prefix/locale
> -  set lang=${grub_lang}
> -  insmod gettext
> +  if [ -f "\$prefix/locale/${grub_lang}.mo" ] ; then
> +    set locale_dir=\$prefix/locale
> +    set lang=${grub_lang}
> +    insmod gettext
> +  fi
>   EOF
>   fi

I'm +1 for the principle, but does it really work for real world locales 
like de_DE which will use de.mo on runtime?

I would guess that it also should handle all the logic in gettext.c 
grub_gettext_init_ext() and grub_mofile_open_lang() and how these 
functions are invoked: .gz extension, _CC stripping and 
primary/secondary locale_dir.

(I would prefer if all this processing could be done in mkconfig instead 
of on runtime, but I guess the desire for backward compatibility 
prevents that.)

(It also seems to me like the current system lacks support for fallback 
from ll_CC@unknownvariant to ll_CC and ll. I don't know if that is a 
real problem.)

/Mads

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Andrey Borzenkov]

В Пн., 24/09/2012 в 11:37 +0200, Mads Kiilerich пишет:
> On 09/24/2012 08:51 AM, Michael Chang wrote:
> > We don't insert gettext module if message catalog file missing to
> > prevent error message from being logged.
> >
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >   util/grub.d/00_header.in |   10 +++++++---
> >   1 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
> > index bb34ef2..d438d52 100644
> > --- a/util/grub.d/00_header.in
> > +++ b/util/grub.d/00_header.in
> > @@ -182,10 +182,14 @@ EOF
> >   
> >   # Gettext variables and module
> >   if [ "x${LANG}" != "xC" ] ; then
> 
> Couldn't / sholdn't this check be replaced by the new check you introduce?
> 
> > +# We don't insert gettext module if message catalog file missing
> > +# To prevent error message from being logged (bnc#771393)
> 
> That seems like a reference to some (internal Suse?) bugtracker? To me 
> it is https://bugzilla.redhat.com/show_bug.cgi?id=817187 , but I guess 
> https://savannah.gnu.org/bugs/?35880 is the best reference.
> 
> >     cat << EOF
> > -  set locale_dir=\$prefix/locale
> > -  set lang=${grub_lang}
> > -  insmod gettext
> > +  if [ -f "\$prefix/locale/${grub_lang}.mo" ] ; then
> > +    set locale_dir=\$prefix/locale
> > +    set lang=${grub_lang}
> > +    insmod gettext
> > +  fi
> >   EOF
> >   fi
> 
> I'm +1 for the principle, but does it really work for real world locales 
> like de_DE which will use de.mo on runtime?
> 

No. It does not, except for zh_CN and zh_TW.

> I would guess that it also should handle all the logic in gettext.c 
> grub_gettext_init_ext() and grub_mofile_open_lang() and how these 
> functions are invoked: .gz extension, _CC stripping and 
> primary/secondary locale_dir.
> 

What about removing this error message altogether? Under OS gettext does
not complaint when catalog does not exist; why should it do it here?
Having English interface is enough indication that message catalog was
not found.

-andrey

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Michael Chang]

2012/9/24 Mads Kiilerich <mads@kiilerich.com>:
> On 09/24/2012 08:51 AM, Michael Chang wrote:
>>
>> We don't insert gettext module if message catalog file missing to
>> prevent error message from being logged.
>>
>> Signed-off-by: Michael Chang <mchang@suse.com>
>> ---
>>   util/grub.d/00_header.in |   10 +++++++---
>>   1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
>> index bb34ef2..d438d52 100644
>> --- a/util/grub.d/00_header.in
>> +++ b/util/grub.d/00_header.in
>> @@ -182,10 +182,14 @@ EOF
>>     # Gettext variables and module
>>   if [ "x${LANG}" != "xC" ] ; then
>
>
> Couldn't / sholdn't this check be replaced by the new check you introduce?

Yes. It should be done in mkconfig, which is in general better than in
run-time. The problem is that I don't know how to get $prefix
environment grub2 set implicitly at runtime. (It seems to be retrieved
somewhere from grub module header but I can't tell in exact).

>
>
>> +# We don't insert gettext module if message catalog file missing
>> +# To prevent error message from being logged (bnc#771393)
>
>
> That seems like a reference to some (internal Suse?) bugtracker? To me it is
> https://bugzilla.redhat.com/show_bug.cgi?id=817187 , but I guess
> https://savannah.gnu.org/bugs/?35880 is the best reference.

The bug is public report against openSUSE 12.2 :
https://bugzilla.novell.com/show_bug.cgi?id=771393

>
>
>>     cat << EOF
>> -  set locale_dir=\$prefix/locale
>> -  set lang=${grub_lang}
>> -  insmod gettext
>> +  if [ -f "\$prefix/locale/${grub_lang}.mo" ] ; then
>> +    set locale_dir=\$prefix/locale
>> +    set lang=${grub_lang}
>> +    insmod gettext
>> +  fi
>>   EOF
>>   fi
>
>
> I'm +1 for the principle, but does it really work for real world locales
> like de_DE which will use de.mo on runtime?

No it doesn't. :(

>
> I would guess that it also should handle all the logic in gettext.c
> grub_gettext_init_ext() and grub_mofile_open_lang() and how these functions
> are invoked: .gz extension, _CC stripping and primary/secondary locale_dir.

You're right. All logics in gettext.c should be considered and
implemented in the patch, otherwise it would cause regressions.

>
> (I would prefer if all this processing could be done in mkconfig instead of
> on runtime, but I guess the desire for backward compatibility prevents
> that.)
>
> (It also seems to me like the current system lacks support for fallback from
> ll_CC@unknownvariant to ll_CC and ll. I don't know if that is a real
> problem.)

Thank you for your great review here, and please forgive my ignorant
not checking the gettext.c before working the patch. :)

Regards,
Michael

>
> /Mads
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Michael Chang]

2012/9/25 Andrey Borzenkov <arvidjaar@gmail.com>:
> В Пн., 24/09/2012 в 11:37 +0200, Mads Kiilerich пишет:
>> On 09/24/2012 08:51 AM, Michael Chang wrote:
>> > We don't insert gettext module if message catalog file missing to
>> > prevent error message from being logged.
>> >
>> > Signed-off-by: Michael Chang <mchang@suse.com>
>> > ---
>> >   util/grub.d/00_header.in |   10 +++++++---
>> >   1 files changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
>> > index bb34ef2..d438d52 100644
>> > --- a/util/grub.d/00_header.in
>> > +++ b/util/grub.d/00_header.in
>> > @@ -182,10 +182,14 @@ EOF
>> >
>> >   # Gettext variables and module
>> >   if [ "x${LANG}" != "xC" ] ; then
>>
>> Couldn't / sholdn't this check be replaced by the new check you introduce?
>>
>> > +# We don't insert gettext module if message catalog file missing
>> > +# To prevent error message from being logged (bnc#771393)
>>
>> That seems like a reference to some (internal Suse?) bugtracker? To me
>> it is https://bugzilla.redhat.com/show_bug.cgi?id=817187 , but I guess
>> https://savannah.gnu.org/bugs/?35880 is the best reference.
>>
>> >     cat << EOF
>> > -  set locale_dir=\$prefix/locale
>> > -  set lang=${grub_lang}
>> > -  insmod gettext
>> > +  if [ -f "\$prefix/locale/${grub_lang}.mo" ] ; then
>> > +    set locale_dir=\$prefix/locale
>> > +    set lang=${grub_lang}
>> > +    insmod gettext
>> > +  fi
>> >   EOF
>> >   fi
>>
>> I'm +1 for the principle, but does it really work for real world locales
>> like de_DE which will use de.mo on runtime?
>>
>
> No. It does not, except for zh_CN and zh_TW.
>
>> I would guess that it also should handle all the logic in gettext.c
>> grub_gettext_init_ext() and grub_mofile_open_lang() and how these
>> functions are invoked: .gz extension, _CC stripping and
>> primary/secondary locale_dir.
>>
>
> What about removing this error message altogether? Under OS gettext does
> not complaint when catalog does not exist; why should it do it here?
> Having English interface is enough indication that message catalog was
> not found.

I agree with you. IMHO the problem is it's not emitted directly from
gettext module but from common underlying fs level, removing it would
lead to other message which is fatal be ignored as well.

Another question is : Is there any other good method offered by grub2
to check the files existence that we could test before calling
grub_file_open() in gettext.c ? Anybody has good idea on this ?

Thanks,
Michael

>
> -andrey
>
>

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Andrey Borzenkov]

В Вт., 25/09/2012 в 12:46 +0800, Michael Chang пишет:
> 2012/9/25 Andrey Borzenkov <arvidjaar@gmail.com>:
> > В Пн., 24/09/2012 в 11:37 +0200, Mads Kiilerich пишет:
> >> On 09/24/2012 08:51 AM, Michael Chang wrote:
> >> > We don't insert gettext module if message catalog file missing to
> >> > prevent error message from being logged.
[...]
> >
> > What about removing this error message altogether? Under OS gettext does
> > not complaint when catalog does not exist; why should it do it here?
> > Having English interface is enough indication that message catalog was
> > not found.
> 
> I agree with you. IMHO the problem is it's not emitted directly from
> gettext module but from common underlying fs level, removing it would
> lead to other message which is fatal be ignored as well.
> 

As far as I can tell it is emitted explicitly when setting "lang":

grub-core/gettext/gettext.c:grub_gettext_env_write_lang()

  grub_err_t err;
  err = grub_gettext_init_ext (&main_context, val, grub_env_get
("locale_dir"),
                               grub_env_get ("prefix"));
  if (err)
    grub_print_error ();

and later. There are some more places which also try to reload catalog.

May be this messages can be turned into debugging message instead of
error.

-andrey

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

В Вт., 25/09/2012 в 19:24 +0400, Andrey Borzenkov пишет:
> В Вт., 25/09/2012 в 12:46 +0800, Michael Chang пишет:
> > 2012/9/25 Andrey Borzenkov <arvidjaar@gmail.com>:
> > > В Пн., 24/09/2012 в 11:37 +0200, Mads Kiilerich пишет:
> > >> On 09/24/2012 08:51 AM, Michael Chang wrote:
> > >> > We don't insert gettext module if message catalog file missing to
> > >> > prevent error message from being logged.
> [...]
> > >
> > > What about removing this error message altogether? Under OS gettext does
> > > not complaint when catalog does not exist; why should it do it here?
> > > Having English interface is enough indication that message catalog was
> > > not found.
> > 
> > I agree with you. IMHO the problem is it's not emitted directly from
> > gettext module but from common underlying fs level, removing it would
> > lead to other message which is fatal be ignored as well.
> > 
> 
> As far as I can tell it is emitted explicitly when setting "lang":
> 
> grub-core/gettext/gettext.c:grub_gettext_env_write_lang()
> 
>   grub_err_t err;
>   err = grub_gettext_init_ext (&main_context, val, grub_env_get
> ("locale_dir"),
>                                grub_env_get ("prefix"));
>   if (err)
>     grub_print_error ();
> 
> and later. There are some more places which also try to reload catalog.
> 

Attached is prototype patch (tested) which suppresses error return from
grub_text_init_ext(). Proper patch would need to also change function
prototype, as its return value is now useless.

> May be this messages can be turned into debugging message instead of
> error.
> 

That I do not know how to do.

[-- Attachment #2: grub2-no-error-on-missing-message-catalog.patch --]
[-- Type: text/x-patch, Size: 639 bytes --]

Index: b/grub-core/gettext/gettext.c
===================================================================
--- a/grub-core/gettext/gettext.c
+++ b/grub-core/gettext/gettext.c
@@ -395,6 +395,9 @@ grub_gettext_init_ext (struct grub_gette
   if (!part1 || part1[0] == 0)
     return 0;
 
+  /* It is not an error if message catalog does not exist */
+  grub_error_push ();
+
   err = grub_mofile_open_lang (ctx, part1, part2, locale);
 
   /* ll_CC didn't work, so try ll.  */
@@ -412,7 +415,9 @@ grub_gettext_init_ext (struct grub_gette
 
       grub_free (lang);
     }
-  return err;
+
+  grub_error_pop ();
+  return 0;
 }
 
 static char *

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Colin Watson]

On Mon, Sep 24, 2012 at 02:51:49PM +0800, Michael Chang wrote:
> We don't insert gettext module if message catalog file missing to
> prevent error message from being logged.

I already posted a patch against the gettext module itself with a
similar rationale, which I think is a better place for this:

  https://savannah.gnu.org/bugs/?35880

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Colin Watson]

On Tue, Sep 25, 2012 at 12:46:15PM +0800, Michael Chang wrote:
> I agree with you. IMHO the problem is it's not emitted directly from
> gettext module but from common underlying fs level, removing it would
> lead to other message which is fatal be ignored as well.

That's incorrect; the gettext module is perfectly able to disregard
errors from lower-level methods that it doesn't care about.

> Another question is : Is there any other good method offered by grub2
> to check the files existence that we could test before calling
> grub_file_open() in gettext.c ? Anybody has good idea on this ?

This is the wrong approach; you should try it and then handle errors,
not look-before-you-leap.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Michael Chang]

2012/9/26 Colin Watson <cjwatson@ubuntu.com>:
> On Tue, Sep 25, 2012 at 12:46:15PM +0800, Michael Chang wrote:
>> I agree with you. IMHO the problem is it's not emitted directly from
>> gettext module but from common underlying fs level, removing it would
>> lead to other message which is fatal be ignored as well.
>
> That's incorrect; the gettext module is perfectly able to disregard
> errors from lower-level methods that it doesn't care about.

I'd say I'm too careless in understanding how grub2 message logging. I
see grub_error(...) and think it was logged to screen immediately
after that function returns, which is completely wrong.

The error message and number are staging in the buffer and it's up to
the module to use grub_print_error (...) to flush all pending messages
to the screen.  Nevertheless you could stack errors with push|pop,
which is extremely useful as grub2 could be stacked by modules ..

Glad to know one more good and flexible design of grub2. :)

Thanks,
Michael

>
>> Another question is : Is there any other good method offered by grub2
>> to check the files existence that we could test before calling
>> grub_file_open() in gettext.c ? Anybody has good idea on this ?
>
> This is the wrong approach; you should try it and then handle errors,
> not look-before-you-leap.
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Colin Watson]

On Wed, Sep 26, 2012 at 03:44:31PM +0800, Michael Chang wrote:
> The error message and number are staging in the buffer and it's up to
> the module to use grub_print_error (...) to flush all pending messages
> to the screen.  Nevertheless you could stack errors with push|pop,
> which is extremely useful as grub2 could be stacked by modules ..

That is one way to do it, but it's also possible to clear errors you're
not interested in without that.  There's an "Error Handling" section in
docs/grub-dev.texi which you may find useful.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] suppress error message "/grub2/locale/en.mo.gz not found"

[Andrey Borzenkov]

В Tue, 25 Sep 2012 22:52:29 +0100
Colin Watson <cjwatson@ubuntu.com> пишет:

> On Mon, Sep 24, 2012 at 02:51:49PM +0800, Michael Chang wrote:
> > We don't insert gettext module if message catalog file missing to
> > prevent error message from being logged.
> 
> I already posted a patch against the gettext module itself with a
> similar rationale, which I think is a better place for this:
> 
>   https://savannah.gnu.org/bugs/?35880
> 

Anything against applying this? Was used successfully in openSUSE for
quite some time.