On 12/04/2014 03:00 AM, Lennart Poettering wrote:
> On Tue, 02.12.14 23:29, WaLyong Cho (walyong....@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
>
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
