[systemd-devel] [PATCH] add Mandriva distribution support

[Lennart Poettering]

On Fri, 04.03.11 19:52, Andrey Borzenkov (arvidjaar at gmail.com) wrote:

> This merges several separate patches that I carry as part of
> Mandriva systemd RPM. They touch those parts that are very
> unlikely to be changed in near future and do not impose any
> functionality change for systemd core. I also think it is
> useful for troubleshooting to have real distribution name in
> system logs, espicially when someone reports problem upstream.
> 
> The patch looks bigger than sum of replaced patches because
> 
> - previous patches were applied on top of distro=fedora, now
> I need to add all those bits for distro=mandriva as well
> 
> - part of patch was done as spec file magic, but it seems more
> logical to ship all these bits together
> 
> Signed-off-by: Andrey Borzenkov <arvidjaar at gmail.com>
> Acked-By: Eugeni Dodonov <eugeni at mandriva.com>

(We don't really do S-o-b nor A-B, please leave this out of patchs,
since we don't want to create the impression the data was valid.

> +#ifdef TARGET_MANDRIVA
> +                "exim",                 SPECIAL_MAIL_TRANSFER_AGENT_TARGET,
> +                "masqmail",             SPECIAL_MAIL_TRANSFER_AGENT_TARGET,
> +                "postfix",              SPECIAL_MAIL_TRANSFER_AGENT_TARGET,
> +                "sendmail",             SPECIAL_MAIL_TRANSFER_AGENT_TARGET,
> +#endif
>          };

Grr, I am not sure I like this much. This hardcodes the names of
specific implementations in systemd, and I am really not sure I want to
start with doing this. Can we find another solution?

>  
> +#elif defined(TARGET_MANDRIVA)
> +
> +	if (!pretty_name) {
> +		char *s, *v = NULL, *p;
> +
> +		if ((r = read_one_line_file("/etc/mandriva-release", &s) < 0))
> +			log_warning("Failed to read
> /etc/mandriva-release: %s", strerror(-r));

Hmm, I want to have an easy upgrade path for everybody to drom those
/etc/*-release files, so please explicitly ignore ENOENT here, like the
others do that as well.

> +		else {
> +			p = strstr(s, " release ");
> +			if (p) {
> +				*p = '\0';
> +				v = p += 9;
> +				while (*p && *p != ' ')
> +				p++;

Please fix indenting of p++;

> +				*p = '\0';

I'd just shorten the last three lines to:

    p[strcspn(p, WHITESPACE)] = 0

Please do not use tabs, btw.

> +
> +				/* This corresponds to standard rc.sysinit */
> +				asprintf(&pretty_name,
> +					"\x1B[1;36m%s\x1B[0;39m %s", s,
> v);

Needs OOM checking.

> +				/* and fake it so colour is right */
> +				const_color = "0";

Hmm, please set "1:36" here and drop it from the pretty name, so that new
need no "fakes" and the variable name actually explains what is in it.

> +			} else
> +				log_warning("Failed to parse /etc/mandriva-release");
> +			free(s);
> +		}
> +	}
> +
>  #endif
>  
>          if (!pretty_name && !const_pretty)
> diff --git a/src/vconsole-setup.c b/src/vconsole-setup.c
> index 1952dfb..f142e8e 100644
> --- a/src/vconsole-setup.c
> +++ b/src/vconsole-setup.c
> @@ -147,6 +147,9 @@ int main(int argc, char **argv) {
>  #ifdef TARGET_GENTOO
>          char *vc_unicode = NULL;
>  #endif
> +#ifdef TARGET_MANDRIVA
> +	char *vc_keytable = NULL;
> +#endif
>          int fd = -1;
>          bool utf8;
>          int r = EXIT_FAILURE;
> @@ -345,6 +348,58 @@ int main(int argc, char **argv) {
>                          if (r != -ENOENT)
>                                  log_warning("Failed to read /etc/conf.d/keymaps: %s", strerror(-r));
>                  }
> +
> +#elif defined(TARGET_MANDRIVA)
> +
> +                if ((r = parse_env_file("/etc/sysconfig/i18n", NEWLINE,
> +                                        "SYSFONT", &vc_font,
> +                                        "SYSFONTACM", &vc_font_map,
> +                                        "UNIMAP", &vc_font_unimap,
> +                                        NULL)) < 0) {
> +
> +                        if (r != -ENOENT)
> +                                log_warning("Failed to read /etc/sysconfig/i18n: %s", strerror(-r));
> +                }
> +
> +		if ((r = parse_env_file("/etc/sysconfig/keyboard", NEWLINE,
> +					"KEYTABLE", &vc_keytable,
> +					"KEYMAP", &vc_keymap,
> +					"UNIKEYTABLE", &vc_keymap,
> +					"GRP_TOGGLE", &vc_keymap_toggle,
> +					NULL)) < 0) {
> +
> +			if (r != -ENOENT)
> +				log_warning("Failed to read /etc/sysconfig/i18n: %s", strerror(-r));
> +		}
> +
> +		if (vc_keytable) {
> +			if (vc_keymap)
> +				free(vc_keymap);
> +			if (utf8) {
> +				if (endswith(vc_keytable, ".uni") || strstr(vc_keytable, ".uni."))
> +					vc_keymap = strdup(vc_keytable);
> +				else {
> +					char *s;
> +					if ((s = strstr(vc_keytable, ".map")))
> +						vc_keytable[s-vc_keytable+1] = '\0';
> +					vc_keymap = strappend(vc_keytable, ".uni");
> +				}
> +			} else
> +				vc_keymap = strdup(vc_keytable);

OOM checking all over the place please!

> +m4_ifdef(`TARGET_MANDRIVA',
> +`# These dependencies are in original SysV script that starts display manager
> +After=network.target acpid.service lircmd.service fs.service haldaemon.service
> +# Hide SysV script
> +Names=dm.service')

Do we really need those? I fail to see why acpid or lircmd or any of those
really should be ordered before X. Can't we just drop them?

If not, then I'd prefer to simple use two seperate prefdm files in
fedora/prefdm.service and mandriva/prefdm.service.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.