From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1NBz4r-0000YL-HC for mharc-grub-devel@gnu.org; Sat, 21 Nov 2009 18:09:13 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NBz4p-0000Xs-Ee for grub-devel@gnu.org; Sat, 21 Nov 2009 18:09:11 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NBz4l-0000XJ-KB for grub-devel@gnu.org; Sat, 21 Nov 2009 18:09:11 -0500 Received: from [199.232.76.173] (port=35585 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NBz4l-0000XF-HQ for grub-devel@gnu.org; Sat, 21 Nov 2009 18:09:07 -0500 Received: from xvm-190-8.ghst.net ([217.70.190.8]:53570 helo=aybabtu.com) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NBz4l-0001mT-3i for grub-devel@gnu.org; Sat, 21 Nov 2009 18:09:07 -0500 Received: from [192.168.10.10] (helo=thorin) by aybabtu.com with esmtp (Exim 4.69) (envelope-from ) id 1NBz4j-0002a0-CV for grub-devel@gnu.org; Sun, 22 Nov 2009 00:09:05 +0100 Received: from rmh by thorin with local (Exim 4.69) (envelope-from ) id 1NBz4i-0007nP-S8 for grub-devel@gnu.org; Sun, 22 Nov 2009 00:09:04 +0100 Date: Sun, 22 Nov 2009 00:09:04 +0100 From: Robert Millan To: The development of GNU GRUB Message-ID: <20091121230904.GA29740@thorin> References: <20091121212712.GA25254@pina.cat> <20091121220054.GA29354@thorin> <20091121221410.GA26442@pina.cat> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091121221410.GA26442@pina.cat> Organization: free as in freedom X-Message-Flag: Worried about Outlook viruses? Switch to Thunderbird! www.mozilla.com/thunderbird X-Debbugs-No-Ack: true User-Agent: Mutt/1.5.18 (2008-05-17) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: Re: merging of gettext branch X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Nov 2009 23:09:11 -0000 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 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."