Re: Build failures on Ubuntu due to gettext
Hi, On Dec/10/2009, Robert Millan wrote: On Tue, Dec 08, 2009 at 12:10:34AM +, Colin Watson wrote: Even if we're only discussing a change in five places, I assume we're going to find this situation in lots of calls throurough GRUB code. If this is so, it really calls for a solution that doesn't make GCC generate two memory references and pass two arguments. So first of all, how many times are we going to find similar calls that very rush numbers, based on my non-commited pending to improve patch to change commands/*: car...@pinux:~/grub2/grub/po$ grep -A 3 \#\: commands\/ grub.pot | grep ^msgid | grep -c -v % 86 need to be adjusted in some way? If we're going to find many of them (as I expect), I think this warrants adding a new function (which, btw, will also make GRUB slightly faster). Plus about 10 in normal/* (aprox, 5 that Colin did and some more in another pending patch I guess) I haven't looked in other code yet. I think that Vladimir proposed to do it in a macro now and then properly count and implement as a function or not later. From: Vladimir Date: Tue, 08 Dec 2009 01:15:30 +0100 Then it's not worth it. But again we haven't gettext'ized whole grub2 yet to know for sure. Perhaps a macro so we can change it later if necessary? So, for about 86 in commands/* and probably some more in normal/*: function now? Now a macro and then we review? Colin: for me you can commit your patch some way or another so you could compile in Ubuntu and I can commit soon more things. Thanks, -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi Vladimir, Colin,
On Dec/08/2009, Vladimir '??-coder/phcoder' Serbinenko wrote:
My patch made the following transformation:
- grub_printf (_(literal string));
+ grub_printf (%s, _(literal string));
This was only necessary in five places, so I doubt that a function is
worth it.
Then it's not worth it. But again we haven't gettext'ized whole grub2
yet to know for sure. Perhaps a macro so we can change it later if
necessary?
Vladimir, Colin: see the attached patch with the Colin changes but
macrofied.
Should compile fine in Ubuntu, but I have not tried.
Are we happy with grub_put_ function name?
Colin: could you apply this patch or similar?
I don't want to push more gettext strings before setting up the basic
infrastructure to avoid working twice.
Thanks,
--
Carles Pina i Estany
http://pinux.info
=== modified file 'include/grub/misc.h'
--- include/grub/misc.h 2009-12-08 00:08:52 +
+++ include/grub/misc.h 2009-12-09 23:51:07 +
@@ -34,6 +34,8 @@
/* XXX: If grub_memmove is too slow, we must implement grub_memcpy. */
#define grub_memcpy(d,s,n) grub_memmove ((d), (s), (n))
+#define grub_put_(str) grub_printf(%s, (str))
+
void *EXPORT_FUNC(grub_memmove) (void *dest, const void *src, grub_size_t n);
char *EXPORT_FUNC(grub_strcpy) (char *dest, const char *src);
char *EXPORT_FUNC(grub_strncpy) (char *dest, const char *src, int c);
=== modified file 'normal/menu_entry.c'
--- normal/menu_entry.c 2009-12-08 00:08:52 +
+++ normal/menu_entry.c 2009-12-09 23:46:36 +
@@ -1000,7 +1000,7 @@ run (struct screen *screen)
grub_cls ();
grub_printf ( );
- grub_printf_ (N_(Booting a command list));
+ grub_put_ (N_(Booting a command list));
grub_printf (\n\n);
@@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
grub_print_error ();
grub_errno = GRUB_ERR_NONE;
grub_putchar ('\n');
- grub_printf_ (N_(Press any key to continue...));
+ grub_put_ (N_(Press any key to continue...));
(void) grub_getkey ();
}
=== modified file 'normal/menu_text.c'
--- normal/menu_text.c 2009-12-08 00:08:52 +
+++ normal/menu_text.c 2009-12-09 23:47:04 +
@@ -40,7 +40,7 @@ void
grub_wait_after_message (void)
{
grub_putchar ('\n');
- grub_printf_ (N_(Press any key to continue...));
+ grub_put_ (N_(Press any key to continue...));
(void) grub_getkey ();
grub_putchar ('\n');
}
@@ -206,7 +206,7 @@ entry is highlighted.);
if (nested)
{
grub_printf (\n);
- grub_printf_ (N_(ESC to return previous menu.));
+ grub_put_ (N_(ESC to return previous menu.));
}
}
}
@@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
grub_errno = GRUB_ERR_NONE;
}
grub_printf (\n );
- grub_printf_ (N_(Failed to boot default entries.\n));
+ grub_put_ (N_(Failed to boot default entries.\n));
grub_wait_after_message ();
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, As Vladimir spotted: On Dec/09/2009, Carles Pina i Estany wrote: This...: +#define grub_put_(str) grub_printf(%s, (str)) should be: #define grub_put_(str) grub_printf(N_ (%s), (str)) If you Colin commit it don't propagate my mistake. -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, On Dec/10/2009, Carles Pina i Estany wrote: As Richard commented (thanks): #define grub_put_(str) grub_printf(N_ (%s), (str)) #define grub_put_(str) grub_printf(%s, N (str)) I should not be sending patches when too tired / without properly testing with the .po that I've just deleted doing some other tests :-) Now should be fine and after it's committed next gettext patches will use it when needed. Cheers, -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Tue, Dec 08, 2009 at 12:10:34AM +, Colin Watson wrote: My patch made the following transformation: - grub_printf (_(literal string)); + grub_printf (%s, _(literal string)); This was only necessary in five places, so I doubt that a function is worth it. I think we all agree that it's safer to avoid gettextizing the template; furthemore I agree with Colin that this seems to be a correct approach, but our case is unusual in that we're very pressed for size. Even if we're only discussing a change in five places, I assume we're going to find this situation in lots of calls throurough GRUB code. If this is so, it really calls for a solution that doesn't make GCC generate two memory references and pass two arguments. So first of all, how many times are we going to find similar calls that need to be adjusted in some way? If we're going to find many of them (as I expect), I think this warrants adding a new function (which, btw, will also make GRUB slightly faster). -- Robert Millan The DRM opt-in fallacy: Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Build failures on Ubuntu due to gettext
Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
(IMO rightly!) complain about constructs such as this:
grub_printf (_(foo));
... because it's all too easy for a translator to (usually accidentally)
insert % sequences which would cause printf to behave incorrectly. This
should instead be:
grub_printf (%s, _(foo));
Patch follows. I can't help thinking that this would be easier with a
grub_puts, but perhaps that isn't worth it given the relatively small
number of occurrences here?
Also, should the line in notify_execution_failure instead be:
- grub_printf (_(Failed to boot default entries.\n));
+ grub_printf (%s\n, _(Failed to boot default entries.));
... to get rid of the unsightly \n in this translated string?
2009-12-07 Colin Watson cjwat...@ubuntu.com
* normal/menu_entry.c (run): Don't pass the result of gettext as
the first argument to grub_printf, appeasing -Wformat-security.
(grub_menu_entry_run): Likewise.
* normal/menu_text.c (grub_wait_after_message): Likewise.
(print_message): Likewise.
(notify_execution_failure): Likewise.
=== modified file 'normal/menu_entry.c'
--- normal/menu_entry.c 2009-12-05 11:25:07 +
+++ normal/menu_entry.c 2009-12-07 14:02:20 +
@@ -1000,7 +1000,7 @@ run (struct screen *screen)
grub_cls ();
grub_printf ( );
- grub_printf (_(Booting a command list));
+ grub_printf (%s, _(Booting a command list));
grub_printf (\n\n);
@@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
grub_print_error ();
grub_errno = GRUB_ERR_NONE;
grub_putchar ('\n');
- grub_printf (_(Press any key to continue...));
+ grub_printf (%s, _(Press any key to continue...));
(void) grub_getkey ();
}
=== modified file 'normal/menu_text.c'
--- normal/menu_text.c 2009-12-05 11:25:07 +
+++ normal/menu_text.c 2009-12-07 14:02:45 +
@@ -40,7 +40,7 @@ void
grub_wait_after_message (void)
{
grub_putchar ('\n');
- grub_printf (_(Press any key to continue...));
+ grub_printf (%s, _(Press any key to continue...));
(void) grub_getkey ();
grub_putchar ('\n');
}
@@ -206,7 +206,7 @@ entry is highlighted.);
if (nested)
{
grub_printf (\n);
- grub_printf (_(ESC to return previous menu.));
+ grub_printf (%s, _(ESC to return previous menu.));
}
}
}
@@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
grub_errno = GRUB_ERR_NONE;
}
grub_printf (\n );
- grub_printf (_(Failed to boot default entries.\n));
+ grub_printf (%s, _(Failed to boot default entries.\n));
grub_wait_after_message ();
}
Thanks,
--
Colin Watson [cjwat...@ubuntu.com]
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hello, On Dec/07/2009, Colin Watson wrote: Ubuntu's GCC enables -Wformat-security by default. This causes GCC to (IMO rightly!) complain about constructs such as this: grub_printf (_(foo)); I see... (actually some weeks ago I thought about gettext security implications, and I thought that if someone can change the .mo files there is some bigger problem for the user... but I understand the point) ... because it's all too easy for a translator to (usually accidentally) insert % sequences which would cause printf to behave (side and non-relevant note: if the translator wants to do some damage on purpose he doesn't need to play with %s and %d, it's as easy as changing strings from Press C to Cancel and D to delete to something like (in another language) Press D to Cancel and C to delete) incorrectly. This should instead be: grub_printf (%s, _(foo)); I like the general idea but I'm not 100% convinced of the implementation: a) It's a bit of false security because it's not fixing the case of file normal/menu_text.c, line 191 (search the string Use the %C and %C keys to) or menu_text.c line 374 (search for The highlighted entry) (I agree that it's better than before... but not very solid) b) How would you translate and handle: grub_printf (_(Hello %s), name); The translator really needs %s because in other languages can be %s hello (not the best example but maybe you get the point) c) I'm thinking to implement grub_printf_ (str). I will send a patch later to see if everybody likes. Then the call for simple strings would be: grub_printf_ (%s, N_(Hello)); Not much different from: grub_printf_ (N_(Hello)); But it's getting hard to print a string :-) d) (important, even if it's the last one): How it prevents mistakes from the current msgfmt checks? For example, and using the option -c in msgfmt: #: normal/misc.c:67 #, c-format msgid test %s t msgstr test %d test2 /usr/bin/msgfmt -c --statistics -o po/ca.mo po/ca.po po/ca.po:1183: format specifications in 'msgid' and 'msgstr' for argument 1 are not the same /usr/bin/msgfmt: found 1 fatal error 26 translated messages, 213 untranslated messages. Using any different number of %X from msgid and msgstr ishalting msgfmt (so, if msgid contains 1 %s and 2 %d, msgstr has to contain the same) We are talking from _( ), I see that -Wformat-security is for string came from untrusted input and contains when .mo are trusted. Are they? How are other projectes implementing it? Specially the dynamic strings. Patch follows. I can't help thinking that this would be easier with a grub_puts, but perhaps that isn't worth it given the relatively small number of occurrences here? We could replace grub_puts with grub_printf... or some other idea. Also, should the line in notify_execution_failure instead be: - grub_printf (_(Failed to boot default entries.\n)); + grub_printf (%s\n, _(Failed to boot default entries.)); ... to get rid of the unsightly \n in this translated string? I really like that this fix the \n discussion that we had :-) I like the idea and I understand that it's easy to make mistakes translating strings. Actually I'm surprised that msgfmt is not giving any warning if the -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Mon, Dec 07, 2009 at 08:14:08PM +, Carles Pina i Estany wrote: On Dec/07/2009, Colin Watson wrote: Ubuntu's GCC enables -Wformat-security by default. This causes GCC to (IMO rightly!) complain about constructs such as this: grub_printf (_(foo)); I see... (actually some weeks ago I thought about gettext security implications, and I thought that if someone can change the .mo files there is some bigger problem for the user... but I understand the point) ... because it's all too easy for a translator to (usually accidentally) insert % sequences which would cause printf to behave (side and non-relevant note: if the translator wants to do some damage on purpose he doesn't need to play with %s and %d, it's as easy as changing strings from Press C to Cancel and D to delete to something like (in another language) Press D to Cancel and C to delete) incorrectly. This should instead be: grub_printf (%s, _(foo)); I like the general idea but I'm not 100% convinced of the implementation: a) It's a bit of false security because it's not fixing the case of file normal/menu_text.c, line 191 (search the string Use the %C and %C keys to) or menu_text.c line 374 (search for The highlighted entry) (I agree that it's better than before... but not very solid) Forget security, in this case; let's just think of it as a way to avoid translators accidentally shooting themselves (and their users) in the foot. I have seen this happen and it's best to prevent it by static checking. b) How would you translate and handle: grub_printf (_(Hello %s), name); -Wformat-security doesn't interfere with that. As its documentation says: At present, this warns about calls to `printf' and `scanf' functions where the format string is not a string literal and there are no format arguments, as in `printf (foo);'. c) I'm thinking to implement grub_printf_ (str). I will send a patch later to see if everybody likes. Then the call for simple strings would be: grub_printf_ (%s, N_(Hello)); Not much different from: grub_printf_ (N_(Hello)); But it's getting hard to print a string :-) grub_puts would be nice and would match the POSIX API (or maybe grub_fputs or grub_putstr or something, since POSIX puts is kind of odd in writing a trailing newline). I don't really like significant trailing underscores in function names myself, although I see why you chose it here. d) (important, even if it's the last one): How it prevents mistakes from the current msgfmt checks? For example, and using the option -c in msgfmt: #: normal/misc.c:67 #, c-format msgid test %s t msgstr test %d test2 /usr/bin/msgfmt -c --statistics -o po/ca.mo po/ca.po po/ca.po:1183: format specifications in 'msgid' and 'msgstr' for argument 1 are not the same /usr/bin/msgfmt: found 1 fatal error 26 translated messages, 213 untranslated messages. Using any different number of %X from msgid and msgstr ishalting msgfmt (so, if msgid contains 1 %s and 2 %d, msgstr has to contain the same) That only works for strings with the c-format attribute, which xgettext only applies (and should only apply) to msgids that contain %. It helps for the string you mentioned earlier with %C in the msgid, but it's no use for _(literal string with no percent characters). We are talking from _( ), I see that -Wformat-security is for string came from untrusted input and contains when .mo are trusted. Are they? I would recommend against considering .mo as entirely trusted, unless you really think that developers will read every submission line-by-line. I realise that as you say there are plenty of more subtle avenues of attack; as I said above, I really think of this as foot-shooting prevention more than security. How are other projectes implementing it? Specially the dynamic strings. Every competent project I've seen that uses gettext in C is in line with the patch I sent, although some use an equivalent of printf(%s, ...) while some use an equivalent of fputs(..., stdout). I like the idea and I understand that it's easy to make mistakes translating strings. Actually I'm surprised that msgfmt is not giving any warning if the In general it can only do this for appropriately tagged strings. For an arbitrary literal string it often isn't actually wrong to translate it with text containing a % sign - it's just that it crashes the program. Thus, it's the program's responsibility to avoid crashing. -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Colin Watson wrote:
Ubuntu's GCC enables -Wformat-security by default. This causes GCC to
(IMO rightly!) complain about constructs such as this:
grub_printf (_(foo));
... because it's all too easy for a translator to (usually accidentally)
insert % sequences which would cause printf to behave incorrectly. This
should instead be:
grub_printf (%s, _(foo));
Patch follows. I can't help thinking that this would be easier with a
grub_puts, but perhaps that isn't worth it given the relatively small
number of occurrences here?
Also, should the line in notify_execution_failure instead be:
- grub_printf (_(Failed to boot default entries.\n));
+ grub_printf (%s\n, _(Failed to boot default entries.));
... to get rid of the unsightly \n in this translated string?
This warning is simply wrong in this context. And silencing it is
against gettext manual. Read
http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html
http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag
2009-12-07 Colin Watson cjwat...@ubuntu.com
* normal/menu_entry.c (run): Don't pass the result of gettext as
the first argument to grub_printf, appeasing -Wformat-security.
(grub_menu_entry_run): Likewise.
* normal/menu_text.c (grub_wait_after_message): Likewise.
(print_message): Likewise.
(notify_execution_failure): Likewise.
=== modified file 'normal/menu_entry.c'
--- normal/menu_entry.c 2009-12-05 11:25:07 +
+++ normal/menu_entry.c 2009-12-07 14:02:20 +
@@ -1000,7 +1000,7 @@ run (struct screen *screen)
grub_cls ();
grub_printf ( );
- grub_printf (_(Booting a command list));
+ grub_printf (%s, _(Booting a command list));
grub_printf (\n\n);
@@ -1182,6 +1182,6 @@ grub_menu_entry_run (grub_menu_entry_t e
grub_print_error ();
grub_errno = GRUB_ERR_NONE;
grub_putchar ('\n');
- grub_printf (_(Press any key to continue...));
+ grub_printf (%s, _(Press any key to continue...));
(void) grub_getkey ();
}
=== modified file 'normal/menu_text.c'
--- normal/menu_text.c2009-12-05 11:25:07 +
+++ normal/menu_text.c2009-12-07 14:02:45 +
@@ -40,7 +40,7 @@ void
grub_wait_after_message (void)
{
grub_putchar ('\n');
- grub_printf (_(Press any key to continue...));
+ grub_printf (%s, _(Press any key to continue...));
(void) grub_getkey ();
grub_putchar ('\n');
}
@@ -206,7 +206,7 @@ entry is highlighted.);
if (nested)
{
grub_printf (\n);
- grub_printf (_(ESC to return previous menu.));
+ grub_printf (%s, _(ESC to return previous menu.));
}
}
}
@@ -655,7 +655,7 @@ notify_execution_failure (void *userdata
grub_errno = GRUB_ERR_NONE;
}
grub_printf (\n );
- grub_printf (_(Failed to boot default entries.\n));
+ grub_printf (%s, _(Failed to boot default entries.\n));
grub_wait_after_message ();
}
Thanks,
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
signature.asc
Description: OpenPGP digital signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, On Dec/07/2009, Colin Watson wrote: On Mon, Dec 07, 2009 at 08:14:08PM +, Carles Pina i Estany wrote: a) It's a bit of false security because it's not fixing the case of file normal/menu_text.c, line 191 (search the string Use the %C and %C keys to) or menu_text.c line 374 (search for The highlighted entry) (I agree that it's better than before... but not very solid) Forget security, in this case; let's just think of it as a way to avoid Ok, I'll forget about security (even when the name is not helping to forget about security :-) ) b) How would you translate and handle: grub_printf (_(Hello %s), name); -Wformat-security doesn't interfere with that. As its documentation says: At present, this warns about calls to `printf' and `scanf' functions where the format string is not a string literal and there are no format arguments, as in `printf (foo);'. I see now (not before) Using any different number of %X from msgid and msgstr ishalting msgfmt (so, if msgid contains 1 %s and 2 %d, msgstr has to contain the same) That only works for strings with the c-format attribute, which xgettext only applies (and should only apply) to msgids that contain %. It helps for the string you mentioned earlier with %C in the msgid, right. Actually I was expecting that all strings in a C file would has c-format, but it's not the case: #: normal/menu_entry.c:840 #, c-format msgid Possible %s are: #: normal/menu_entry.c:1003 msgid Booting a command list And confirmed: if the translator adds %C in the second string (Booting a command list) msgfmt -c is not complaining. So I would apply your patch, after understanding that it's only for strings that doesn't have any %C (and in my opinion xgettext could add the c-format if it's in a C file, but we will not discuss it here now :-) , I guess that in some cases would not be correct) Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_) but it's not a big problem. If you commit before I would adapt my one, else I would adapt your one. -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Mon, Dec 07, 2009 at 11:04:52PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote: Colin Watson wrote: Ubuntu's GCC enables -Wformat-security by default. This causes GCC to (IMO rightly!) complain about constructs such as this: grub_printf (_(foo)); ... because it's all too easy for a translator to (usually accidentally) insert % sequences which would cause printf to behave incorrectly. This should instead be: grub_printf (%s, _(foo)); Patch follows. I can't help thinking that this would be easier with a grub_puts, but perhaps that isn't worth it given the relatively small number of occurrences here? Also, should the line in notify_execution_failure instead be: - grub_printf (_(Failed to boot default entries.\n)); + grub_printf (%s\n, _(Failed to boot default entries.)); ... to get rid of the unsightly \n in this translated string? This warning is simply wrong in this context. I disagree. I have seen many practical problems caused by this coding style. And silencing it is against gettext manual. Read http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html I see nothing relevant in that link. (I am familiar with gettext already, having been using it for many years in other free software projects.) http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag That suggests one available option for tweaking the way gettext handles such strings, but it does not prescribe any particular solution to the problem. Indeed, it says Of course one would normally use fputs, which is just as valid a solution as adding the c-format flag, and quite possibly more valid since it's a heck of a lot easier to maintain. We don't have fputs in GRUB right now, so grub_printf (%s, _(...)) is perfectly valid too, in just the same way that fputs is. IME, forcing no-c-format is much more useful than forcing c-format. In short, it is not true that silencing it is against gettext manual, but thank you for providing me with links that if anything support my position. ;-) Vladimir, please can you be a little less terse in your objections in future? For example, it is often helpful to quote specific text rather than vague URLs. If you would like arbitration on whether my approach is correct as per the gettext maintainers, I'm happy to bring it up with them. Thanks, -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, On Dec/07/2009, Vladimir '??-coder/phcoder' Serbinenko wrote: Colin Watson wrote: Ubuntu's GCC enables -Wformat-security by default. This causes GCC to (IMO rightly!) complain about constructs such as this: grub_printf (_(foo)); ... because it's all too easy for a translator to (usually accidentally) insert % sequences which would cause printf to behave incorrectly. This should instead be: grub_printf (%s, _(foo)); Patch follows. I can't help thinking that this would be easier with a grub_puts, but perhaps that isn't worth it given the relatively small number of occurrences here? Also, should the line in notify_execution_failure instead be: - grub_printf (_(Failed to boot default entries.\n)); + grub_printf (%s\n, _(Failed to boot default entries.)); ... to get rid of the unsightly \n in this translated string? This warning is simply wrong in this context. And silencing it is against gettext manual. Read http://www.gnu.org/software/hello/manual/gettext/Preparing-Strings.html http://www.gnu.org/software/hello/manual/gettext/c_002dformat-Flag.html#c_002dformat-Flag other solution that I like even more (but I have a problem to implement): use --flag=_:1:pass-c-format . So all strings will be c-format and msgfmt will check the number of parameters (even if the string doesn't have %C, will be c-format and msgfmt should complain if msgstr has a new %C) -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Carles Pina i Estany wrote: And confirmed: if the translator adds %C in the second string (Booting a command list) msgfmt -c is not complaining. So I would apply your patch, after understanding that it's only for strings that doesn't have any %C (and in my opinion xgettext could add the c-format if it's in a C file, but we will not discuss it here now :-) , I guess that in some cases would not be correct) Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_) but it's not a big problem. If you commit before I would adapt my one, else I would adapt your one. Your N_ patch superseeds Colin's patch. Feel free to use printf_ where necessary. It solves format-security problems -- Regards Vladimir 'φ-coder/phcoder' Serbinenko signature.asc Description: OpenPGP digital signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, On Dec/07/2009, Carles Pina i Estany wrote: So I would apply your patch, after understanding that it's only for one more thing Colin. What do you think to not apply your patch and apply the attached patch? It force that the argument 1 of _ and N_ is a c-format (in this way appears in grub.pot). This is, IMO, correct approach. And then msgfmt -c checks the number of arguments. If other projects are not using this approach they could have some reason, like _(File) is a menu label and not a C format string. In this cases, if this happens in Grub it's not making things worse. Using it we should have all checks about the number of arguments and we are not programming warning oriented. If it's a linguism and wide accepted to do in the other way I personally don't have any strong objection. -- Carles Pina i Estany http://pinux.info === modified file 'Makefile.in' --- Makefile.in 2009-11-30 01:25:57 + +++ Makefile.in 2009-12-07 23:28:49 + @@ -477,7 +477,7 @@ genkernsyms.sh: genkernsyms.sh.in config $(SHELL) ./config.status $(srcdir)/po/$(PACKAGE).pot: po/POTFILES po/POTFILES-shell - cd $(srcdir) $(XGETTEXT) --from-code=utf-8 -o $@ -f $ --keyword=_ --keyword=N_ + cd $(srcdir) $(XGETTEXT) --from-code=utf-8 -o $@ -f $ --keyword=_ --keyword=N_ --flag=N_:1:c-format --flag=_:1:c-format cd $(srcdir) $(XGETTEXT) --from-code=utf-8 -o $@ -f po/POTFILES-shell -j --language=Shell $(foreach lang, $(LINGUAS), $(srcdir)/po/$(lang).po): po/$(PACKAGE).pot ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Furthermore, the style I suggested is used in many other GNU projects. Here are just a few examples: coreutils/src/du.c:994: error (0, 0, %s, _(invalid zero-length file name)); diffutils/src/sdiff.c:853: fprintf (stderr, %s, _(\ diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\ glibc/inet/rcmd.c:178: __fxprintf(NULL, %s, _(\ glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n)); gnulib/lib/xalloc-die.c:34: error (exit_failure, 0, %s, _(memory exhausted)); Indeed, here's a commit from the gettext author using this style: http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=d5d1ae3b3c3ae2f837740e5f5d65326197ccdb98 (Look for close_stdout_status in lib/closeout.c.) -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Tue, Dec 08, 2009 at 12:25:01AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote: Your N_ patch superseeds Colin's patch. Feel free to use printf_ where necessary. It solves format-security problems It only works around the compiler warning if functions have not been given the correct function attributes. It certainly doesn't solve the underlying issue. -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Mon, Dec 07, 2009 at 11:38:04PM +, Carles Pina i Estany wrote: On Dec/07/2009, Carles Pina i Estany wrote: So I would apply your patch, after understanding that it's only for one more thing Colin. What do you think to not apply your patch and apply the attached patch? It force that the argument 1 of _ and N_ is a c-format (in this way appears in grub.pot). This is, IMO, correct approach. And then msgfmt -c checks the number of arguments. I really think that this is incorrect, I'm afraid. I've certainly never seen any other project do this. There's no reason to restrict _ and N_ only to be used by printf-a-likes. Cheers, -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Colin Watson wrote: Furthermore, the style I suggested is used in many other GNU projects. Here are just a few examples: coreutils/src/du.c:994: error (0, 0, %s, _(invalid zero-length file name)); diffutils/src/sdiff.c:853: fprintf (stderr, %s, _(\ diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\ glibc/inet/rcmd.c:178: __fxprintf(NULL, %s, _(\ glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n)); gnulib/lib/xalloc-die.c:34: error (exit_failure, 0, %s, _(memory exhausted)); I have nothing against defining grub_putl_ (str); equivalent to grub_printf (%s\n, str); (defined as function if it's used extensively for space reasons). Just from your previous mails it seemed that you proposed to transform strings like grub_printf (_(Moving file %s to %s.), f1, f2); to grub_printf (%s %s %s %s., _(Moving file), f1, _(to), f2); to avoid translating format-strings which would be completely untranslatable. Indeed, here's a commit from the gettext author using this style: http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=d5d1ae3b3c3ae2f837740e5f5d65326197ccdb98 (Look for close_stdout_status in lib/closeout.c.) -- Regards Vladimir 'φ-coder/phcoder' Serbinenko signature.asc Description: OpenPGP digital signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Mon, Dec 07, 2009 at 10:46:30PM +, Carles Pina i Estany wrote: Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_) but it's not a big problem. If you commit before I would adapt my one, else I would adapt your one. Please go ahead with your patch after due discussion, and I'll adjust mine afterwards. It's too confusing to try to discuss both at the same time. -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
On Tue, Dec 08, 2009 at 12:59:28AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote: Colin Watson wrote: Furthermore, the style I suggested is used in many other GNU projects. Here are just a few examples: coreutils/src/du.c:994: error (0, 0, %s, _(invalid zero-length file name)); diffutils/src/sdiff.c:853: fprintf (stderr, %s, _(\ diffutils/src/sdiff.c-854-ed:\tEdit then use both versions, each decorated with a header.\n\ glibc/inet/rcmd.c:178: __fxprintf(NULL, %s, _(\ glibc/inet/rcmd.c-179-rcmd: socket: All ports in use\n)); gnulib/lib/xalloc-die.c:34: error (exit_failure, 0, %s, _(memory exhausted)); I have nothing against defining grub_putl_ (str); equivalent to grub_printf (%s\n, str); (defined as function if it's used extensively for space reasons). Just from your previous mails it seemed that you proposed to transform strings like grub_printf (_(Moving file %s to %s.), f1, f2); to grub_printf (%s %s %s %s., _(Moving file), f1, _(to), f2); to avoid translating format-strings which would be completely untranslatable. What on earth?! I said nothing of the kind! That would obviously be completely insane. How did you arrive at that impression? My patch made the following transformation: - grub_printf (_(literal string)); + grub_printf (%s, _(literal string)); This was only necessary in five places, so I doubt that a function is worth it. Thanks, -- Colin Watson [cjwat...@ubuntu.com] ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Hi, On Dec/08/2009, Colin Watson wrote: On Mon, Dec 07, 2009 at 10:46:30PM +, Carles Pina i Estany wrote: Your patch conflicts with my one (Subject: gettext: grub_printf_ and N_) but it's not a big problem. If you commit before I would adapt my one, else I would adapt your one. Please go ahead with your patch after due discussion, and I'll adjust mine afterwards. It's too confusing to try to discuss both at the same time. I just pushed my one since I understand that it's all right (using grub_vprintf). And it's complementary with your one. If you push your one (for which one I agree) I will adjust my pending patches to follow the coding sytle. Thanks, -- Carles Pina i Estany http://pinux.info ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Build failures on Ubuntu due to gettext
Colin Watson wrote: What on earth?! I said nothing of the kind! That would obviously be completely insane. Sorry for having misunderstood you. How did you arrive at that impression? Extrapolation of proposed changes since a lot of patches in gettext threads were samples. My patch made the following transformation: - grub_printf (_(literal string)); + grub_printf (%s, _(literal string)); This was only necessary in five places, so I doubt that a function is worth it. Then it's not worth it. But again we haven't gettext'ized whole grub2 yet to know for sure. Perhaps a macro so we can change it later if necessary? Thanks, -- Regards Vladimir 'φ-coder/phcoder' Serbinenko signature.asc Description: OpenPGP digital signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
