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

[Andrey Borzenkov]

On Mon, Mar 7, 2011 at 3:33 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> 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.
>

removed

>> +#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?
>

I removed it for now. It does not look like
SPECIAL_MAIL_TRANSFER_AGENT_TARGET is actually used anywhere inside
systemd itself; and we better wait for real demand to emerge.

>>
>> +#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.
>

Done

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

Done

>> +                             *p = '\0';
>
> I'd just shorten the last three lines to:
>
>    p[strcspn(p, WHITESPACE)] = 0
>

Done

> Please do not use tabs, btw.
>

Done

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

Done

>> +                             /* 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.
>

OK, makes sense.

>> +             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!
>

Actually it became just one place, consolidated. Done.

>> +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?
>

Digging in SVN, lircmd seems to be some typo when SVN was resurrected
after crash. At least it appears from nowhere without a single
comment.

X still tries to contact acpid; funnily enough it fails though, do not know why:

[   116.539] (WW) Open ACPI failed (/var/run/acpid.socket) (Connection refused)

may be race condition (acpid is still not actually started at this point?).

hald can likely be dropped.

we still ship X font server, so if user happens to have it enabled, it
is expected to still work.

Regarding network - this is done due to possible network
authentication as well as complaints that KDM (or was it GDM, do not
remember) does not show correct host name. This is the best
approximation I can get today; I have to think about it.

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

OK, so let's do it separate unit (and Makefile section) so we have
flexibility. I dropped lircmd which seems bogus but left others for
now.

Version with your comments applied attached.

Thank you!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mandriva.patch
Type: text/x-patch
Size: 18947 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20110308/9845ebce/attachment-0001.bin>