Re: merging of gettext branch

[Robert Millan <rmh@aybabtu.com>]

On Sat, Nov 21, 2009 at 10:14:10PM +0000, Carles Pina i Estany wrote:
> @@ -179,7 +185,7 @@
>  pkglib_MODULES += fshelp.mod fat.mod ufs1.mod ufs2.mod ext2.mod ntfs.mod \
>  	ntfscomp.mod minix.mod hfs.mod jfs.mod iso9660.mod xfs.mod	\
>  	affs.mod sfs.mod hfsplus.mod reiserfs.mod cpio.mod tar.mod	\
> -	udf.mod	afs.mod afs_be.mod befs.mod befs_be.mod
> +	udf.mod	afs.mod afs_be.mod befs.mod befs_be.mod gettext.mod
>  
>  # For fshelp.mod.
>  fshelp_mod_SOURCES = fs/fshelp.c
> @@ -610,6 +616,11 @@
>  bufio_mod_CFLAGS = $(COMMON_CFLAGS)
>  bufio_mod_LDFLAGS = $(COMMON_LDFLAGS)
>  
> +# For gettext.mod.
> +gettext_mod_SOURCES = gettext/gettext.c
> +gettext_mod_CFLAGS = $(COMMON_CFLAGS)
> +gettext_mod_LDFLAGS = $(COMMON_LDFLAGS)

I haven't switched the old declarations, but for new modules please use
something like:

pkglib_MODULES += gettext.mod
gettext_mod_SOURCES = gettext/gettext.c
gettext_mod_CFLAGS = $(COMMON_CFLAGS)
gettext_mod_LDFLAGS = $(COMMON_LDFLAGS)

this way it is easier to move declarations around, etc.

> +static grub_file_t grub_mofile_open (const char *name);

We usually skip declarations for static functions (when all its users are
placed after its implementation, of course).

> +#define GETTEXT_MAGIC_NUMBER 0
> +#define GETTEXT_FILE_FORMAT 4
> +#define GETTEXT_NUMBER_OF_STRINGS 8
> +#define GETTEXT_OFFSET_ORIGINAL 12
> +#define GETTEXT_OFFSET_TRANSLATION 16

These would look much prettier with some tabs ;-)

> +  grub_file_seek (fd_mo, offset);
> +  grub_file_read (fd_mo, translation, length);

You do a lot of grub_file_seek + grub_file_read throurough the code.  Maybe
grub_file_pread() would be more appropiate?  (for space saving)

> +          grub_free(current_string);
> +          min=current;
> [...]
> +    current = (max+min)/2;
> [...]
> +  ret = grub_malloc(grub_strlen(orig) + 1);
> +  grub_strcpy(ret,orig);

Missing spaces (between function name and parenthesis, around '=', etc).

I suggest you use indent(1), this will automatically adjust to our coding
style.

> +  /*
> +  Do we want .mo.gz files? Then, the code:
> +  file = grub_gzio_open (io, 0); // 0: transparent
> +  if (! file)
> +    {
> +      grub_printf("Problems opening the file\n");
> +      grub_file_close (io);
> +      return 0;
> +    }
> +  */

Uhm I wonder if we could answer this question before the code is merged; what
does everyone think about .mo.gz?

> +  locale_dir = grub_env_get ("locale_dir");

This needs to be checked for NULL.

> +  // mo_file e.g.: /boot/grub/locale/ca.mo

Please use C-style comments (/* */).

> +  mo_file = grub_malloc (grub_strlen (locale_dir) + sizeof ("/") + grub_strlen (lang) + sizeof(".mo"));

Note that sizeof ("/") equals 2 ('/' + '\0').

> +  grub_dprintf("gettext", "Will try to open file: %s " ,mo_file);
> +
> +  fd_mo = grub_mofile_open(mo_file);

In this case I think error handling would be more useful than debug print (i.e.
if file can't be opened, rise an error).

> +  /* Testing:
> +  grub_register_command ("_", grub_cmd_translate, GRUB_COMMAND_FLAG_BOTH,
> +			 "_", "internalization support trans", 0);
> +  */

We could define this interface unconditionally, but it'd be more consistent
as `gettext' command (like the one in GNU gettext package).

> === added file 'include/grub/i18n_grub.h'
> --- include/grub/i18n_grub.h	1970-01-01 00:00:00 +0000
> +++ include/grub/i18n_grub.h	2009-11-19 21:32:05 +0000
> [...]
> +#ifndef	GRUB_I18N_GRUB_H
> +#define	GRUB_I18N_GRUB_H	1
> +
> +# define _(str) grub_gettext(str)
> +
> +#endif /* GRUB_I18N_GRUB_H */

You can use <grub/i18n.h> for this (in fact it already defines _ to a
noop stub for non-util and would just need to be modified).

> +const char *EXPORT_FUNC(grub_gettext_dummy) (const char *s);
> +extern const char *(*EXPORT_VAR(grub_gettext)) (const char *s);// = grub_gettext_dummy;

This could be in i18n.h too (except the comment ;-)).

Btw, is it necessary to export grub_gettext_dummy symbol?  If we avoid it
it's a few less bytes in kernel :-)

> +# Gettext variables and module
> +cat << EOF
> +set locale_dir=${locale_dir}
> +set lang=${grub_lang}
> +insmod gettext 
> +EOF

We could avoid loading the module at all for non-utf8 capable setups (when
this happens, grub-mkconfig exports LANG=C, so this can be easily detected).

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