[systemd-devel] [PATCH] gentoo: vconsole-setup support.

[Lennart Poettering]

On Sun, 19.09.10 21:23, Gustavo Sverzut Barbieri (barbieri at profusion.mobi) wrote:

Heya,

Looks pretty good, but a few requests:

> +#ifdef TARGET_GENTOO
> +/* Gentoo uses sys-apps/kbd that installs to /usr and have fonts in CamelCase */
> +#define KBD_LOADKEYS "/usr/bin/loadkeys"
> +#define KBD_SETFONT "/usr/bin/setfont"
> +#define DEFAULT_FONT "LatArCyrHeb-16"
> +#else
> +#define KBD_LOADKEYS "/bin/loadkeys"
> +#define KBD_SETFONT "/bin/setfont"
> +#define DEFAULT_FONT "latarcyrheb-sun16"
> +#endif

I think it would be nicer if the path and filename configuration would
happen in the Makefile, instead of in the sources. Could you move this
to Makefile.am?

> +#elif defined(TARGET_GENTOO)
> +        if ((r = parse_env_file("/etc/rc.conf", NEWLINE,
> +                                "unicode", &vc_unicode,
> +                                NULL)) < 0) {
> +                if (r != -ENOENT)
> +                        log_warning("Failed to read /etc/rc.conf: %s", strerror(-r));
> +        }
> +        if (vc_unicode) {
> +                int rc_unicode;
> +
> +                if (strcasecmp(vc_unicode, "yes") == 0 ||
> +                    strcasecmp(vc_unicode, "on") == 0 ||
> +                    strcasecmp(vc_unicode, "true") == 0 ||
> +                    strcmp(vc_unicode, "1") == 0)
> +                        rc_unicode = 1;

Please use parse_boolean() here, which does pretty much exactly what you
want to do here, but is much nicer to read.

Otherwise looks good to me!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.