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?
Lennart
--
Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
