Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=

2013-07-13 Thread Karol Lewandowski 
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=

2013-07-12 Thread Łukasz Stelmach 
It was 2013-07-12 pią 04:48, when Kyungmin Park wrote:
 On Fri, Jul 12, 2013 at 7:43 AM, Lennart Poettering
 lenn...@poettering.net 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 RD 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=

2013-07-12 Thread Łukasz Stelmach 
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 RD 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=

2013-07-12 Thread Lennart Poettering 
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=

2013-07-12 Thread Lennart Poettering 
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=

2013-07-12 Thread Kay Sievers 
On Fri, Jul 12, 2013 at 3:31 PM, Lennart Poettering
lenn...@poettering.net 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=

2013-07-12 Thread Karol Lewandowski 
On 07/12/2013 03:40 PM, Kay Sievers wrote:
 On Fri, Jul 12, 2013 at 3:31 PM, Lennart Poettering
 lenn...@poettering.net 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=

2013-07-12 Thread Lennart Poettering 
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
  lenn...@poettering.net 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

[systemd-devel] [RFC PATCH] condition: add ConditionFileContains=

2013-07-11 Thread Karol Lewandowski 
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 @@
 
termvarnameConditionDirectoryNotEmpty=/varname/term
 
termvarnameConditionFileNotEmpty=/varname/term
 
termvarnameConditionFileIsExecutable=/varname/term
+
termvarnameConditionFileContains=/varname/term
 
termvarnameConditionKernelCommandLine=/varname/term
 
termvarnameConditionVirtualization=/varname/term
 
termvarnameConditionSecurity=/varname/term
@@ -932,6 +933,13 @@
 exists, is a regular file and marked
 executable./para
 
+paravarnameConditionFileContains=/varname
+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./para
+
 paraSimilar,
 varnameConditionKernelCommandLine=/varname
 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,

Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=

2013-07-11 Thread Dave Reisner 
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 @@
  
 termvarnameConditionDirectoryNotEmpty=/varname/term
  
 termvarnameConditionFileNotEmpty=/varname/term
  
 termvarnameConditionFileIsExecutable=/varname/term
 +
 termvarnameConditionFileContains=/varname/term
  
 termvarnameConditionKernelCommandLine=/varname/term
  
 termvarnameConditionVirtualization=/varname/term
  
 termvarnameConditionSecurity=/varname/term
 @@ -932,6 +933,13 @@
  exists, is a regular file and marked
  executable./para
  
 +
 paravarnameConditionFileContains=/varname
 +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./para
 +
  paraSimilar,
  
 varnameConditionKernelCommandLine=/varname
  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

Re: [systemd-devel] [RFC PATCH] condition: add ConditionFileContains=

2013-07-11 Thread Lennart Poettering 
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=

2013-07-11 Thread Lennart Poettering 
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=

2013-07-11 Thread Kyungmin Park 
On Fri, Jul 12, 2013 at 7:43 AM, Lennart Poettering
lenn...@poettering.net 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