[systemd-devel] [PATCH 1/2] Allow disable of SysV init/rcN.d support at compile time

[Lennart Poettering]

On Mon, 20.09.10 16:33, Fabiano Fidencio (fidencio at profusion.mobi) wrote:

>  static DEFINE_CONFIG_PARSE_ENUM(config_parse_kill_mode, kill_mode, KillMode, "Failed to parse kill mode");
>  
> @@ -1446,7 +1448,9 @@ static void dump_items(FILE *f, const ConfigItem *items) {
>                  { config_parse_exec,             "PATH [ARGUMENT [...]]" },
>                  { config_parse_service_type,     "SERVICETYPE" },
>                  { config_parse_service_restart,  "SERVICERESTART" },
> +#ifdef HAVE_SYSV_COMPAT
>                  { config_parse_sysv_priority,    "SYSVPRIORITY" },
> +#endif
>                  { config_parse_kill_mode,        "KILLMODE" },
>                  { config_parse_kill_signal,      "SIGNAL" },
>                  { config_parse_listen,           "SOCKET [...]" },
> @@ -1597,7 +1601,9 @@ static int load_from_path(Unit *u, const char *path) {
>                  { "PermissionsStartOnly",   config_parse_bool,            &u->service.permissions_start_only,              "Service" },
>                  { "RootDirectoryStartOnly", config_parse_bool,            &u->service.root_directory_start_only,           "Service" },
>                  { "RemainAfterExit",        config_parse_bool,            &u->service.remain_after_exit,                   "Service" },
> +#ifdef HAVE_SYSV_COMPAT
>                  { "SysVStartPriority",      config_parse_sysv_priority,   &u->service.sysv_start_priority,                 "Service" },
> +#endif
>                  { "NonBlocking",            config_parse_bool,            &u->service.exec_context.non_blocking,           "Service" },
>                  { "BusName",                config_parse_string_printf,   &u->service.bus_name,                            "Service" },
>                  { "NotifyAccess",           config_parse_notify_access,   &u->service.notify_access,                       "Service" },

Hmpf, as mentioned in my earlier mail I blieve it is really important
that we still parse unit files correctly that have been written with syv
in mind even on a system where sysv is disabled. This change would
generate parse failures in this case.

Could you add config_parse_warn_compat() which would return successful,
but print one log_debug() messages warning that the requested setting is
not available because the feature was disabled at compile time? This
should be done generic enough so that we later on can use it in mmore
cases like this.

> +#ifdef HAVE_SYSV_COMPAT
>                  case SIGTERM:
>                          if (m->running_as == MANAGER_SYSTEM) {
>                                  /* This is for compatibility with the
> @@ -1967,6 +1968,7 @@ static int manager_process_signal_fd(Manager *m) {
>                                  m->exit_code = MANAGER_REEXECUTE;
>                                  break;
>                          }
> +#endif

Nah, this please keep around. SIGTERM is SIGTERM. There's no point in
making it a NOP in non-sysv builds, as reexecution makes sense both with
and without sysv.

> +#ifdef HAVE_SYSV_COMPAT
>          .check_gc = service_check_gc,
> +#else
> +        .check_gc = NULL,
> +#endif

You can leave the NULL assignment out since everything not set
explicitly here defaults to NULL anyway.

>          .check_snapshot = service_check_snapshot,
>  
>          .sigchld_event = service_sigchld_event,
> @@ -3009,5 +3031,9 @@ const UnitVTable service_vtable = {
>          .bus_message_handler = bus_service_message_handler,
>          .bus_invalidating_properties =  bus_service_invalidating_properties,
>  
> +#ifdef HAVE_SYSV_COMPAT
>          .enumerate = service_enumerate
> +#else
> +        .enumerate = NULL
> +#endif

Same here.

> @@ -117,19 +117,21 @@ struct Service {
>          bool bus_name_good:1;
>          bool forbid_restart:1;
>          bool got_socket_fd:1;
> +#ifdef HAVE_SYSV_COMPAT
>          bool sysv_has_lsb:1;
>          bool sysv_enabled:1;
> -
> -        int socket_fd;
>          int sysv_start_priority;
>  
>          char *sysv_path;
>          char *sysv_runlevels;
> +#endif
>  
>          char *bus_name;
>  
>          char *status_text;
>  
> +        int socket_fd;
> +

Grmbl. ints are 32bit. On 64bit systems this would hence create 4 byte
hole that is unnecessary, if you put it here. Please move it up, next to
after the pid_t definitions. (Yes, i am a pahole nazi).

> -                        if ((c = exit_status_to_string(p->status, i->is_sysv ? EXIT_STATUS_LSB : EXIT_STATUS_SYSTEMD)))
> +#ifdef HAVE_SYSV_COMPAT
> +                        if ((c = exit_status_to_string(p->status, EXIT_STATUS_LSB)))
> +#else
> +                        if ((c = exit_status_to_string(p->status, EXIT_STATUS_SYSTEMD)))
> +#endif

Hey, this is quite a change. the HAVE_SYSV_COMPAT branch still needs to
check i->is_sysv!

> -                                        if ((c = exit_status_to_string(i->exit_status, i->is_sysv ? EXIT_STATUS_LSB : EXIT_STATUS_SYSTEMD)))
> +#ifdef HAVE_SYSV_COMPAT
> +                                        if ((c = exit_status_to_string(i->exit_status, EXIT_STATUS_LSB)))
> +#else
> +                                        if ((c = exit_status_to_string(i->exit_status, EXIT_STATUS_SYSTEMD)))
> +#endif

Same here.

Otherwise looks fine. I'll definitely merge it if you fix the few issues
left. Promised!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.