[systemd-devel] [PATCH 5/6] Add trivial templated args to [Install] WantedBy and RequiredBy fields

[Lennart Poettering]

On Sat, 03.11.12 21:52, Oleksii Shevchuk (alxchk at gmail.com) wrote:

I think this makes sense. 

> +#include "specifier.h"
>  
>  typedef struct {
>          char *name;
> @@ -51,6 +52,30 @@ typedef struct {
>          Hashmap *have_installed;
>  } InstallContext;
>  
> +char *name_printf(char * prefix, char * instance, char* format) {

We don't need to export this, do we?

> +
> +        /*
> +         * This will use the passed string as format string and
> +         * replace the following specifiers, if any:
> +         *
> +         * %p: the unit prefix                         (foo)
> +         * %i: the instance                            (bar)
> +         */
> +
> +        char empty_prefix[]   = "%p";
> +        char empty_instance[] = "%i";
> +
> +        const Specifier table[] = {
> +                { 'p', specifier_string, prefix   ? : empty_prefix},
> +                { 'i', specifier_string, instance ? : empty_instance},
> +                { 0, NULL, NULL }
> +        };

Hmm, specifier_string() already handles NULL strings, and returns the
empty string for them. Isn't that good enough for this case? Is
returning the unreplaced %p, resp. %i really nicer in that case?

> +
> +        assert(format);
> +
> +        return specifier_printf(format, table, NULL);
> +}
> +
>  static int lookup_paths_init_from_scope(LookupPaths *paths, UnitFileScope scope) {
>          assert(paths);
>          assert(scope >= 0);
> @@ -1271,13 +1296,21 @@ static int install_info_symlink_wants(
>  
>          STRV_FOREACH(s, i->wanted_by) {
>                  char *path;
> +                char *instance = NULL;
> +                char *prefix   = NULL;
> +                char *dst      = NULL;
> +
> +                unit_name_to_instance(i->name, &instance);

We always need to check for OOM for calls like this.

Otherwise looks quite OK...

Lennart

-- 
Lennart Poettering - Red Hat, Inc.