Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, Jul 12, 2013 at 09:19:58PM +0200, Lennart Poettering wrote: > On Fri, 12.07.13 20:42, Karol Lewandowski (k.lewando...@samsung.com) wrote: ... > > - software raid (md) status - /proc/mdstat > > Not sure what this is really doing... /etc/init.d/hdparm seems to be bailing out if md-raid is not fully operational and /etc/init.d/halt doesn't pass "-h" to final halt(8) if md is in active state. > > Every such case could be handled by generic "built-in grep" > > instead of dozen of flags like ConditionCPUFeature=, > > ConditionMDStatus=, ... > > I am pretty sure we cover most of these cases with some other way too. > > I mean, I am generally willing to add this, but if there's no strict > need for it, I'd avoid it. Let's avoid it for now as our use case can be solved by udev rule (as Kay suggested). I'll take closer into Tizen boot sequence next monday to see if there are other cases where feature like this might come useful (hopefully I won't find any). Thanks ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, 12.07.13 20:42, Karol Lewandowski (k.lewando...@samsung.com) wrote: > > On Fri, Jul 12, 2013 at 3:31 PM, Lennart Poettering > > wrote: > > > ConditionFileContains=/sys/module/sn/parameters/enabled:1 > > > >> To make this clear: I am not keen on adding this. I can see the > >> usefulness, and the thing is still simple enough so that it would be OK > >> to add this, but if you guys don't actually need that I'd prefer to keep > >> our codebase smaller... > > We can workaround this one way or another - be it shell script, > udev rule or, in some cases, generator. Patch that started this > discussion is yet another option. > > I have looked into /etc/init.d on my Debian system and found > following examples of 'grepping' files used to decide if given > service should be started or not: > > - kernel option specified - /proc/cmdline We have ConditionKernelCommandLine= for this case, and it is a bit more powerful. > - filesystem available - /proc/filesystems This sounds like something one could also do with ConditionPathExists= checking for some module in /sys/module/ or so. > - file contains non commented out lines - /etc/exports It's probably a better idea to simply not ship that file by default. That's how we handle the rc.boot case. > - cpu features available - /proc/cpuinfo We have CPU feature based kernel module auto-loading these days, which makes pretty much all of these cases where this was necessary redundant. If the modules are auto-loaded where needed it is afterrwards sufficient to check for /sys/module/ for the functionality... > - software raid (md) status - /proc/mdstat Not sure what this is really doing... > - ... > > Every such case could be handled by generic "built-in grep" > instead of dozen of flags like ConditionCPUFeature=, > ConditionMDStatus=, ... I am pretty sure we cover most of these cases with some other way too. I mean, I am generally willing to add this, but if there's no strict need for it, I'd avoid it. Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On 07/12/2013 03:40 PM, Kay Sievers wrote: > On Fri, Jul 12, 2013 at 3:31 PM, Lennart Poettering > wrote: > ConditionFileContains=/sys/module/sn/parameters/enabled:1 > >> To make this clear: I am not keen on adding this. I can see the >> usefulness, and the thing is still simple enough so that it would be OK >> to add this, but if you guys don't actually need that I'd prefer to keep >> our codebase smaller... We can workaround this one way or another - be it shell script, udev rule or, in some cases, generator. Patch that started this discussion is yet another option. I have looked into /etc/init.d on my Debian system and found following examples of 'grepping' files used to decide if given service should be started or not: - kernel option specified - /proc/cmdline - filesystem available - /proc/filesystems - file contains non commented out lines - /etc/exports - cpu features available - /proc/cpuinfo - software raid (md) status - /proc/mdstat - ... Every such case could be handled by generic "built-in grep" instead of dozen of flags like ConditionCPUFeature=, ConditionMDStatus=, ... Having said that I fully understand that the line has to be drawn somewhere. [1] Such condition should probably support globs so we would end up with something like ConditionFileContentsGlob= > Just a note: > The above example should be triggered by a udev rule anyway. Agreed. Thanks ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, Jul 12, 2013 at 10:29 PM, Lennart Poettering wrote: > On Fri, 12.07.13 11:48, Kyungmin Park (kmp...@infradead.org) wrote: > >> It's another question. >> the main goal of this patch is that supports conditional execution >> If some services are executed with given condition, it should check >> some conditions and execute different flow. >> if [ condition is 1 ]; then >> launch A service >> else if [ condition is 2 ]; then >> launch B service >> else >> launch C service >> >> current systemd scheme we have to prepare three service file. so can >> we make it only one service with some unit extension? IOW multipul >> unit support? >> >> [Unit] >> ... >> ConditionBlahBlah=1 >> >> [Unit] >> ... >> ConditionBlahBlah=2 >> >> [Unit] >> ... >> ConditionBlahBlah=!1 && !2 or empty line > > Unit files are not supposed to be a Turing complete programming > language, and conditions are suggested only as simple checks for > optimizing start-up of some services. Their configuration files are > independent of each other so that we can load/unload them separately. > Okay, I knew it's not systemd goal. I just asked does it have plan for these requirements. we will create different service files for each purpose. Thank you, Kyungmin Park ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, Jul 12, 2013 at 3:31 PM, Lennart Poettering wrote: >> > ConditionFileContains=/sys/module/sn/parameters/enabled:1 > To make this clear: I am not keen on adding this. I can see the > usefulness, and the thing is still simple enough so that it would be OK > to add this, but if you guys don't actually need that I'd prefer to keep > our codebase smaller... Just a note: The above example should be triggered by a udev rule anyway. During bootup: udevadm trigger --type=subsystems will coldplug all modules, and udev rules can catch the content of a module parameter and pull-in a service on demand. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, 12.07.13 10:16, Łukasz Stelmach (l.stelm...@samsung.com) wrote:
> It was <2013-07-11 czw 19:18>, when Karol Lewandowski wrote:
> > Add ability to test if given file contains specified value.
> > File and expected value are given as one argument separated
> > by colon (":"), i.e.
> >
> > ConditionFileContains=/sys/module/sn/parameters/enabled:1
> >
> > ---
> > As above example suggests we use it to conditionally
> > start service based on kernel module parameter value.
> > This can be (ab)used for other/regular files too.
> >
> > RFC
>
> I am confused a bit. Although I can see (hardly though) the point but
> I am not sure, yet another condition in unit files is required. IMHO
> systemd can check if files required by a service exist (and some other
> tests which more or less cover test(1) functions) and not lauch the
> service if they does not but, it is up to the service to check the
> files' contents. No matter if it is a single character or a 24Mpix jpeg
> file.
>
> Technically, except for the notes already given by others, the patch
> does not look bad.
To make this clear: I am not keen on adding this. I can see the
usefulness, and the thing is still simple enough so that it would be OK
to add this, but if you guys don't actually need that I'd prefer to keep
our codebase smaller...
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, 12.07.13 11:48, Kyungmin Park (kmp...@infradead.org) wrote: > It's another question. > the main goal of this patch is that supports conditional execution > If some services are executed with given condition, it should check > some conditions and execute different flow. > if [ condition is 1 ]; then > launch A service > else if [ condition is 2 ]; then > launch B service > else > launch C service > > current systemd scheme we have to prepare three service file. so can > we make it only one service with some unit extension? IOW multipul > unit support? > > [Unit] > ... > ConditionBlahBlah=1 > > [Unit] > ... > ConditionBlahBlah=2 > > [Unit] > ... > ConditionBlahBlah=!1 && !2 or empty line Unit files are not supposed to be a Turing complete programming language, and conditions are suggested only as simple checks for optimizing start-up of some services. Their configuration files are independent of each other so that we can load/unload them separately. Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
It was <2013-07-11 czw 19:18>, when Karol Lewandowski wrote:
> Add ability to test if given file contains specified value.
> File and expected value are given as one argument separated
> by colon (":"), i.e.
>
> ConditionFileContains=/sys/module/sn/parameters/enabled:1
>
> ---
> As above example suggests we use it to conditionally
> start service based on kernel module parameter value.
> This can be (ab)used for other/regular files too.
>
> RFC
I am confused a bit. Although I can see (hardly though) the point but
I am not sure, yet another condition in unit files is required. IMHO
systemd can check if files required by a service exist (and some other
tests which more or less cover test(1) functions) and not lauch the
service if they does not but, it is up to the service to check the
files' contents. No matter if it is a single character or a 24Mpix jpeg
file.
Technically, except for the notes already given by others, the patch
does not look bad.
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
It was <2013-07-12 pią 04:48>, when Kyungmin Park wrote: > On Fri, Jul 12, 2013 at 7:43 AM, Lennart Poettering > wrote: >> On Thu, 11.07.13 13:37, Dave Reisner (d...@falconindy.com) wrote: >> >>> > +p = strchr(path, ':'); >>> >>> This is going to fail for a file:value pair such as /foo:/bar/baz:value. >>> You could use strrchr(), but then you have to be concerned about >>> matching values with a colon. >> >> This might become a problem, but then again, I think it is OK if some >> files cannot be checked with this. I'd prefer using a space or "=" as >> separator as a better choice though, as that's probably less frequent >> than ":" in the names of files one would check with this condition >> setting. >> >> Lennart >> > > It's another question. > the main goal of this patch is that supports conditional execution > If some services are executed with given condition, it should check > some conditions and execute different flow. > if [ condition is 1 ]; then > launch A service > else if [ condition is 2 ]; then > launch B service > else > launch C service IMHO this looks too complicated to fit into systemd units. To support such (multiple choice) situation (which seems like the the conditions are not properly stated) we can use a generator to link the desired service to the proper target. -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Fri, Jul 12, 2013 at 7:43 AM, Lennart Poettering wrote: > On Thu, 11.07.13 13:37, Dave Reisner (d...@falconindy.com) wrote: > >> > +p = strchr(path, ':'); >> >> This is going to fail for a file:value pair such as /foo:/bar/baz:value. >> You could use strrchr(), but then you have to be concerned about >> matching values with a colon. > > This might become a problem, but then again, I think it is OK if some > files cannot be checked with this. I'd prefer using a space or "=" as > separator as a better choice though, as that's probably less frequent > than ":" in the names of files one would check with this condition > setting. > > Lennart > It's another question. the main goal of this patch is that supports conditional execution If some services are executed with given condition, it should check some conditions and execute different flow. if [ condition is 1 ]; then launch A service else if [ condition is 2 ]; then launch B service else launch C service current systemd scheme we have to prepare three service file. so can we make it only one service with some unit extension? IOW multipul unit support? [Unit] ... ConditionBlahBlah=1 [Unit] ... ConditionBlahBlah=2 [Unit] ... ConditionBlahBlah=!1 && !2 or empty line Thank you, Kyungmin Park ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Thu, 11.07.13 13:37, Dave Reisner (d...@falconindy.com) wrote: > > +p = strchr(path, ':'); > > This is going to fail for a file:value pair such as /foo:/bar/baz:value. > You could use strrchr(), but then you have to be concerned about > matching values with a colon. This might become a problem, but then again, I think it is OK if some files cannot be checked with this. I'd prefer using a space or "=" as separator as a better choice though, as that's probably less frequent than ":" in the names of files one would check with this condition setting. Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Thu, 11.07.13 19:18, Karol Lewandowski (k.lewando...@samsung.com) wrote:
> Add ability to test if given file contains specified value.
> File and expected value are given as one argument separated
> by colon (":"), i.e.
>
> ConditionFileContains=/sys/module/sn/parameters/enabled:1
Hmm, I don't like the naming. If this is called "Contains" I'd always
assume that it does substring matching or so?
Maybe ConditionFileContentsIs= or so?
I don#t really like the : as separator. Either "=" or a space sounds better.
> +path = strdup(parameter);
> +if (!path) {
> +log_oom();
> +return false;
> +}
> +
> +p = strchr(path, ':');
> +if (!p)
> +return false;
> +
> +*(p++) = '\0';
I'd prefer if we'd have a path_is_absolute() check here, and immeidately
fail with a warning if the path isn't absolute.
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
On Thu, Jul 11, 2013 at 07:18:35PM +0200, Karol Lewandowski wrote:
> Add ability to test if given file contains specified value.
> File and expected value are given as one argument separated
> by colon (":"), i.e.
>
> ConditionFileContains=/sys/module/sn/parameters/enabled:1
>
> ---
> As above example suggests we use it to conditionally
> start service based on kernel module parameter value.
> This can be (ab)used for other/regular files too.
>
> RFC
>
> Thanks!
> ---
> man/systemd.unit.xml |8
> src/core/condition.c | 34
> +
> src/core/condition.h |1 +
> src/core/load-fragment-gperf.gperf.m4 |1 +
> 4 files changed, 44 insertions(+)
>
> diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml
> index 4f0bd64..d26bd23 100644
> --- a/man/systemd.unit.xml
> +++ b/man/systemd.unit.xml
> @@ -841,6 +841,7 @@
>
> ConditionDirectoryNotEmpty=
>
> ConditionFileNotEmpty=
>
> ConditionFileIsExecutable=
> +
> ConditionFileContains=
>
> ConditionKernelCommandLine=
>
> ConditionVirtualization=
>
> ConditionSecurity=
> @@ -932,6 +933,13 @@
> exists, is a regular file and marked
> executable.
>
> +
> ConditionFileContains=
> +may be used to check whether a given
> +file contains specified value. The
> +argument consists of file path to be
> +tested, separator (":"), and the
> +expected value.
> +
> Similar,
>
> ConditionKernelCommandLine=
> may be used to check whether a
> diff --git a/src/core/condition.c b/src/core/condition.c
> index 427aa08..7c62d11 100644
> --- a/src/core/condition.c
> +++ b/src/core/condition.c
> @@ -124,6 +124,36 @@ static bool test_kernel_command_line(const char
> *parameter) {
> return found;
> }
>
> +static bool test_file_contains(const char *parameter) {
> +int r;
> +char *p;
> +_cleanup_free_ char *path = NULL;
> +_cleanup_free_ char *line = NULL;
> +
> +assert(parameter);
> +
> +path = strdup(parameter);
> +if (!path) {
> +log_oom();
> +return false;
> +}
> +
> +p = strchr(path, ':');
This is going to fail for a file:value pair such as /foo:/bar/baz:value.
You could use strrchr(), but then you have to be concerned about
matching values with a colon.
> +if (!p)
> +return false;
> +
> +*(p++) = '\0';
> +
> +r = read_one_line_file(path, &line);
> +
> +if (r < 0) {
> +log_warning("Failed to read %s, ignoring: %s", path,
> strerror(-r));
> +return false;
> +}
> +
> +return !!streq(line, p);
> +}
> +
> static bool test_virtualization(const char *parameter) {
> int b;
> Virtualization v;
> @@ -307,6 +337,9 @@ bool condition_test(Condition *c) {
> return (S_ISREG(st.st_mode) && (st.st_mode & 0111)) ==
> !c->negate;
> }
>
> +case CONDITION_FILE_CONTAINS:
> +return test_file_contains(c->parameter) == !c->negate;
> +
> case CONDITION_KERNEL_COMMAND_LINE:
> return test_kernel_command_line(c->parameter) == !c->negate;
>
> @@ -392,6 +425,7 @@ static const char* const
> condition_type_table[_CONDITION_TYPE_MAX] = {
> [CONDITION_DIRECTORY_NOT_EMPTY] = "ConditionDirectoryNotEmpty",
> [CONDITION_FILE_NOT_EMPTY] = "ConditionFileNotEmpty",
> [CONDITION_FILE_IS_EXECUTABLE] = "ConditionFileIsExecutable",
> +[CONDITION_FILE_CONTAINS] = "ConditionFileContains",
> [CONDITION_KERNEL_COMMAND_LINE] = "ConditionKernelCommandLine",
> [CONDITION_VIRTUALIZATION] = "ConditionVirtualization",
> [CONDITION_SECURITY] = "ConditionSecurity",
> diff --git a/src/core/condition.h b/src/core/condition.h
> index 50ed955..2bf26bd 100644
> --- a/src/core/condition.h
> +++ b/src/core/condition.h
> @@ -35,6 +35,7 @@ typedef enum ConditionType {
> CONDITION_DIRECTORY_NOT_EMPTY,
> CONDITION_FILE_NOT_EMPTY,
> CONDITION_FILE_IS_EXECUTABLE,
> +CONDITION_FILE_CONTAINS,
> CONDITION_KERNEL_COMMAND_LINE,
> CONDITION_VIRTUALIZATION,
> CONDITION_SECURITY,
> diff --git a/src/core/load-fragment-gperf.gperf.m4
> b/src/core/load-fragment-gperf.gperf.m4
> index 2325d6a..310e1c5 100644
> --- a/src/core/load-fragment-gperf
[systemd-devel] [RFC PATCH] condition: add ConditionFileContains=
Add ability to test if given file contains specified value.
File and expected value are given as one argument separated
by colon (":"), i.e.
ConditionFileContains=/sys/module/sn/parameters/enabled:1
---
As above example suggests we use it to conditionally
start service based on kernel module parameter value.
This can be (ab)used for other/regular files too.
RFC
Thanks!
---
man/systemd.unit.xml |8
src/core/condition.c | 34 +
src/core/condition.h |1 +
src/core/load-fragment-gperf.gperf.m4 |1 +
4 files changed, 44 insertions(+)
diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml
index 4f0bd64..d26bd23 100644
--- a/man/systemd.unit.xml
+++ b/man/systemd.unit.xml
@@ -841,6 +841,7 @@
ConditionDirectoryNotEmpty=
ConditionFileNotEmpty=
ConditionFileIsExecutable=
+
ConditionFileContains=
ConditionKernelCommandLine=
ConditionVirtualization=
ConditionSecurity=
@@ -932,6 +933,13 @@
exists, is a regular file and marked
executable.
+ConditionFileContains=
+may be used to check whether a given
+file contains specified value. The
+argument consists of file path to be
+tested, separator (":"), and the
+expected value.
+
Similar,
ConditionKernelCommandLine=
may be used to check whether a
diff --git a/src/core/condition.c b/src/core/condition.c
index 427aa08..7c62d11 100644
--- a/src/core/condition.c
+++ b/src/core/condition.c
@@ -124,6 +124,36 @@ static bool test_kernel_command_line(const char
*parameter) {
return found;
}
+static bool test_file_contains(const char *parameter) {
+int r;
+char *p;
+_cleanup_free_ char *path = NULL;
+_cleanup_free_ char *line = NULL;
+
+assert(parameter);
+
+path = strdup(parameter);
+if (!path) {
+log_oom();
+return false;
+}
+
+p = strchr(path, ':');
+if (!p)
+return false;
+
+*(p++) = '\0';
+
+r = read_one_line_file(path, &line);
+
+if (r < 0) {
+log_warning("Failed to read %s, ignoring: %s", path,
strerror(-r));
+return false;
+}
+
+return !!streq(line, p);
+}
+
static bool test_virtualization(const char *parameter) {
int b;
Virtualization v;
@@ -307,6 +337,9 @@ bool condition_test(Condition *c) {
return (S_ISREG(st.st_mode) && (st.st_mode & 0111)) ==
!c->negate;
}
+case CONDITION_FILE_CONTAINS:
+return test_file_contains(c->parameter) == !c->negate;
+
case CONDITION_KERNEL_COMMAND_LINE:
return test_kernel_command_line(c->parameter) == !c->negate;
@@ -392,6 +425,7 @@ static const char* const
condition_type_table[_CONDITION_TYPE_MAX] = {
[CONDITION_DIRECTORY_NOT_EMPTY] = "ConditionDirectoryNotEmpty",
[CONDITION_FILE_NOT_EMPTY] = "ConditionFileNotEmpty",
[CONDITION_FILE_IS_EXECUTABLE] = "ConditionFileIsExecutable",
+[CONDITION_FILE_CONTAINS] = "ConditionFileContains",
[CONDITION_KERNEL_COMMAND_LINE] = "ConditionKernelCommandLine",
[CONDITION_VIRTUALIZATION] = "ConditionVirtualization",
[CONDITION_SECURITY] = "ConditionSecurity",
diff --git a/src/core/condition.h b/src/core/condition.h
index 50ed955..2bf26bd 100644
--- a/src/core/condition.h
+++ b/src/core/condition.h
@@ -35,6 +35,7 @@ typedef enum ConditionType {
CONDITION_DIRECTORY_NOT_EMPTY,
CONDITION_FILE_NOT_EMPTY,
CONDITION_FILE_IS_EXECUTABLE,
+CONDITION_FILE_CONTAINS,
CONDITION_KERNEL_COMMAND_LINE,
CONDITION_VIRTUALIZATION,
CONDITION_SECURITY,
diff --git a/src/core/load-fragment-gperf.gperf.m4
b/src/core/load-fragment-gperf.gperf.m4
index 2325d6a..310e1c5 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -134,6 +134,7 @@ Unit.ConditionPathIsReadWrite,
config_parse_unit_condition_path, CONDITION_P
Unit.ConditionDirectoryNotEmpty, config_parse_unit_condition_path,
CONDITION_DIRECTORY_NOT_EMPTY, 0
Unit.ConditionFileNotEmpty, config_parse_unit_condition_path,
CONDITION_FILE_NOT_EMPTY, 0
Unit.ConditionFileIsExecutable, config_parse_unit_condition_path,
CONDITION_FILE_IS_EXECUTABLE, 0
+Unit.ConditionFileContains, config_par
