[systemd-devel] [PATCH v4 1/4] bus: StartTransientUnit can have aux unit

[Lennart Poettering]

On Tue, 02.12.14 23:29, WaLyong Cho (walyong.cho at samsung.com) wrote:

> ---
>  src/core/dbus-manager.c | 123 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 105 insertions(+), 18 deletions(-)
> 
> diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
> index 0994d7b..643aa8b 100644
> --- a/src/core/dbus-manager.c
> +++ b/src/core/dbus-manager.c
> @@ -615,6 +615,93 @@ static int method_set_unit_properties(sd_bus *bus, sd_bus_message *message, void
>          return bus_unit_method_set_properties(bus, message, u, error);
>  }
>  
> +static int transient_unit_from_message(
> +                Manager *m,
> +                sd_bus_message *message,
> +                const char *name,
> +                Unit **unit,
> +                sd_bus_error *error) {
> +
> +        Unit *u;
> +        int r;
> +
> +        assert(m);
> +        assert(message);
> +        assert(name);
> +
> +        r = manager_load_unit(m, name, NULL, error, &u);
> +        if (r < 0)
> +                return r;
> +
> +        if (u->load_state != UNIT_NOT_FOUND ||
> +            set_size(u->dependencies[UNIT_REFERENCED_BY]) > 0)
> +                return sd_bus_error_setf(error,
> +                                         BUS_ERROR_UNIT_EXISTS,
> +                                         "Unit %s already exists.",
> +                                         name);

Please do not line-break so eagerly. See CODING_STYLE, we do no follow
a 80ch regime.

> +
> +static int try_aux_units_in_message(
> +                Manager *m,
> +                sd_bus_message *message,
> +                sd_bus_error *error) {

Why call this "try"? Maybe invoke it
"transient_aux_units_from_message()" or so?

>          if (r < 0)
>                  return r;
>          if (r == 0)
> -                return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
> +                /* No authorization for now, but the async polkit
> +                 * stuff will call us again when it has it */
> +                return 1;

Please don't rearrange lines that your patch doesn't really
change. Please don't break lines too eagerly.

>  
>          r = sd_bus_message_read(message, "ss", &name, &smode);
>          if (r < 0)
> @@ -639,34 +728,32 @@ static int method_start_transient_unit(sd_bus *bus, sd_bus_message *message, voi
>  
>          t = unit_name_to_type(name);
>          if (t < 0)
> -                return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid unit type.");
> +                return sd_bus_error_setf(error,
> +                                         SD_BUS_ERROR_INVALID_ARGS,
> +                                         "Invalid unit type.");

Same here...

>  
>          if (!unit_vtable[t]->can_transient)
> -                return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Unit type %s does not support transient units.", unit_type_to_string(t));
> -
> -        mode = job_mode_from_string(smode);
> -        if (mode < 0)
> -                return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Job mode %s is invalid.", smode);
> +                return sd_bus_error_setf(error,
> +                                         SD_BUS_ERROR_INVALID_ARGS,
> +                                         "Unit type %s does not support transient units.",
> +                                         unit_type_to_string(t));

Same here.

>  
>          r = mac_selinux_access_check(message, "start", error);
>          if (r < 0)
>                  return r;
>  
> -        r = manager_load_unit(m, name, NULL, error, &u);
> -        if (r < 0)
> -                return r;
> -
> -        if (u->load_state != UNIT_NOT_FOUND || set_size(u->dependencies[UNIT_REFERENCED_BY]) > 0)
> -                return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, "Unit %s already exists.", name);
> +        mode = job_mode_from_string(smode);
> +        if (mode < 0)
> +                return sd_bus_error_setf(error,
> +                                         SD_BUS_ERROR_INVALID_ARGS,
> +                                         "Job mode %s is invalid.",
> +                                         smode);

Why did you move the parsing of the job mode after the selinux access
check? I think we should validate all args before doing any further checks.

>  
> -        /* OK, the unit failed to load and is unreferenced, now let's
> -         * fill in the transient data instead */
> -        r = unit_make_transient(u);
> +        r = transient_unit_from_message(m, message, name, &u, error);
>          if (r < 0)
>                  return r;
>  
> -        /* Set our properties */
> -        r = bus_unit_set_properties(u, message, UNIT_RUNTIME, false, error);
> +        r = try_aux_units_in_message(m, message, error);
>          if (r < 0)
>                  return r;

Hmm, the unit_load() invocation, isn't that something that should move
into transient_unit_from_message() as well?

Lennart

-- 
Lennart Poettering, Red Hat