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

[WaLyong Cho]

On 12/04/2014 03:00 AM, Lennart Poettering wrote:
> 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?

The main transient unit should have UNIT_REFERENCED_BY dependency to the
aux unit. So the aux unit should be loaded first. To keep this order,
unit_load() can not be move into transient_unit_from_message().

WaLyong
> 
> Lennart
>