Re: [systemd-devel] [PATCH v6 3/3] run: introduce timer support option
On Tue, 09.12.14 00:03, WaLyong Cho (walyong@samsung.com) wrote:
} else {
log_error(Unknown assignment %s., assignment);
return -EINVAL;
diff --git a/src/run/run.c b/src/run/run.c
index 85eb052..e145784 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -30,6 +30,7 @@
#include env-util.h
#include path-util.h
#include bus-error.h
+#include calendarspec.h
static bool arg_scope = false;
static bool arg_remain_after_exit = false;
@@ -47,27 +48,45 @@ static int arg_nice = 0;
static bool arg_nice_set = false;
static char **arg_environment = NULL;
static char **arg_property = NULL;
+static bool with_timer = false;
Hmm, maybe we should turn this boolean into a function? I mean, static
variables are something we should keep at a minimum. We do them for
command line arguments, since they are kinda global anyway, but I
think we should otherwise avoid them, especially if they are
effectively redundant anyway.
Hence, my suggestions would be to turn this into a call:
static bool with_timer(void) {
return arg_on_active || arg_on_boot || arg_on_startup ||
arg_on_unit_active || arg_on_unit_inactive || arg_on_calendar;
}
Or so?
+static int start_transient_service(
+sd_bus *bus,
+char **argv,
+sd_bus_error *error) {
+
+_cleanup_bus_message_unref_ sd_bus_message *m = NULL;
+_cleanup_free_ char *service = NULL;
+char *state = NULL;
+int r;
+
+assert(bus);
+assert(argv);
+
+if (arg_unit) {
+service = unit_name_mangle_with_suffix(arg_unit,
MANGLE_NOGLOB, .service);
+if (!service)
+return log_oom();
+
+if (get_unit_state_by_name(bus, service, state) == 0) {
+log_error(Unit %s is already exist with %s state.,
service, state);
+return -EEXIST;
+}
What's the rationale for checking the unit state here? I mean, PID 1
should fail anyway in this case, do we realy need to check this name
in advance? I mean, such a check is necessarily racy, since the unit
might appear while between our client-side check and the actual
execution of the service, no?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v6 3/3] run: introduce timer support option
On 12/09/2014 12:38 AM, Lennart Poettering wrote:
On Tue, 09.12.14 00:03, WaLyong Cho (walyong@samsung.com) wrote:
} else {
log_error(Unknown assignment %s., assignment);
return -EINVAL;
diff --git a/src/run/run.c b/src/run/run.c
index 85eb052..e145784 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -30,6 +30,7 @@
#include env-util.h
#include path-util.h
#include bus-error.h
+#include calendarspec.h
static bool arg_scope = false;
static bool arg_remain_after_exit = false;
@@ -47,27 +48,45 @@ static int arg_nice = 0;
static bool arg_nice_set = false;
static char **arg_environment = NULL;
static char **arg_property = NULL;
+static bool with_timer = false;
Hmm, maybe we should turn this boolean into a function? I mean, static
variables are something we should keep at a minimum. We do them for
command line arguments, since they are kinda global anyway, but I
think we should otherwise avoid them, especially if they are
effectively redundant anyway.
Hence, my suggestions would be to turn this into a call:
static bool with_timer(void) {
return arg_on_active || arg_on_boot || arg_on_startup ||
arg_on_unit_active || arg_on_unit_inactive || arg_on_calendar;
}
Or so?
Yes, it should be.
+static int start_transient_service(
+sd_bus *bus,
+char **argv,
+sd_bus_error *error) {
+
+_cleanup_bus_message_unref_ sd_bus_message *m = NULL;
+_cleanup_free_ char *service = NULL;
+char *state = NULL;
+int r;
+
+assert(bus);
+assert(argv);
+
+if (arg_unit) {
+service = unit_name_mangle_with_suffix(arg_unit,
MANGLE_NOGLOB, .service);
+if (!service)
+return log_oom();
+
+if (get_unit_state_by_name(bus, service, state) == 0) {
+log_error(Unit %s is already exist with %s
state., service, state);
+return -EEXIST;
+}
What's the rationale for checking the unit state here? I mean, PID 1
should fail anyway in this case, do we realy need to check this name
in advance? I mean, such a check is necessarily racy, since the unit
might appear while between our client-side check and the actual
execution of the service, no?
I had plan to generate transient timer unit for already existing
service. To do that I needed get_unit_state_by_name() and I used that
also here.
with_aux = get_unit_state_by_name(bus, service, state)
0 ? true : false;
Above is to determine that. Honestly, some of codes are removed during
rebase. I will rework for that.
Anyway, as you said, it is role of server side not client. I will remove
on here.
WaLyong
Lennart
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v6 3/3] run: introduce timer support option
On Tue, 09.12.14 01:13, WaLyong Cho (walyong@samsung.com) wrote: What's the rationale for checking the unit state here? I mean, PID 1 should fail anyway in this case, do we realy need to check this name in advance? I mean, such a check is necessarily racy, since the unit might appear while between our client-side check and the actual execution of the service, no? I had plan to generate transient timer unit for already existing service. This might make sense to add one day, but I'd really leave that out for now. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
