Re: [systemd-devel] [PATCH] swap: use swapon -o

[Tom Gundersen]

Applied. Thanks!

Tom

On Mon, May 25, 2015 at 12:11 PM, Karel Zak  wrote:
> This patch simplify swapon usage in systemd. The command swapon(8)
> since util-linux v2.26 supports "-o ". The idea is exactly the
> same like for mount(8). The -o specifies options in fstab-compatible
> way. For systemd it means that it does not have to care about things
> like "discard" or another swapon specific options.
>
> swapon -o 
>
> For backward compatibility the code cares about "Priority:" swap unit
> field (for a case when Priority: is set, but pri= in the Options: is
> missing).
>
> References: 
> http://lists.freedesktop.org/archives/systemd-devel/2014-October/023576.html
> ---
> V2:
>  - update README
>  - add hint to systemd.swap man page
>  - don't care about pri= in systed-fstab-generator at all
>  - add warning about duplicate priority configuration
>  - use warning rather than notice for non-parsable pri=
>
>  README|  2 +-
>  man/systemd.swap.xml  |  3 ++-
>  src/core/swap.c   | 43 
> +++
>  src/fstab-generator/fstab-generator.c | 28 ---
>  4 files changed, 26 insertions(+), 50 deletions(-)
>
> diff --git a/README b/README
> index 039110e..2b8c68e 100644
> --- a/README
> +++ b/README
> @@ -136,7 +136,7 @@ REQUIREMENTS:
>  During runtime, you need the following additional
>  dependencies:
>
> -util-linux >= v2.25 required
> +util-linux >= v2.26 required
>  dbus >= 1.4.0 (strictly speaking optional, but recommended)
>  dracut (optional)
>  PolicyKit (optional)
> diff --git a/man/systemd.swap.xml b/man/systemd.swap.xml
> index 5016f45..c398677 100644
> --- a/man/systemd.swap.xml
> +++ b/man/systemd.swap.xml
> @@ -177,7 +177,8 @@
>
>  Swap priority to use when activating the swap
>  device or file. This takes an integer. This setting is
> -optional.
> +optional and ignored when priotiry is set by pri= 
> in the
> +Options= option.
>
>
>
> diff --git a/src/core/swap.c b/src/core/swap.c
> index 12ebf84..193c8c3 100644
> --- a/src/core/swap.c
> +++ b/src/core/swap.c
> @@ -717,8 +717,8 @@ fail:
>  }
>
>  static void swap_enter_activating(Swap *s) {
> -_cleanup_free_ char *discard = NULL;
> -int r, priority = -1;
> +_cleanup_free_ char *opts = NULL;
> +int r;
>
>  assert(s);
>
> @@ -726,13 +726,21 @@ static void swap_enter_activating(Swap *s) {
>  s->control_command = s->exec_command + SWAP_EXEC_ACTIVATE;
>
>  if (s->from_fragment) {
> -fstab_filter_options(s->parameters_fragment.options, 
> "discard\0", NULL, &discard, NULL);
> +int priority = -1;
>
> -priority = s->parameters_fragment.priority;
> -if (priority < 0) {
> -r = fstab_find_pri(s->parameters_fragment.options, 
> &priority);
> +r = fstab_find_pri(s->parameters_fragment.options, 
> &priority);
> +if (r < 0)
> +log_warning_errno(r, "Failed to parse swap priority 
> \"%s\", ignoring: %m", s->parameters_fragment.options);
> +else if (r == 1 && s->parameters_fragment.priority >= 0)
> +log_warning("Duplicate swap priority configuration 
> by Priority and Options fields.");
> +
> +if (r <= 0 && s->parameters_fragment.priority >= 0) {
> +if (s->parameters_fragment.options)
> +r = asprintf(&opts, "%s,pri=%i", 
> s->parameters_fragment.options, s->parameters_fragment.priority);
> +else
> +r = asprintf(&opts, "pri=%i", 
> s->parameters_fragment.priority);
>  if (r < 0)
> -log_notice_errno(r, "Failed to parse swap 
> priority \"%s\", ignoring: %m", s->parameters_fragment.options);
> +goto fail;
>  }
>  }
>
> @@ -740,24 +748,9 @@ static void swap_enter_activating(Swap *s) {
>  if (r < 0)
>  goto fail;
>
> -if (priority >= 0) {
> -char p[DECIMAL_STR_MAX(int)];
> -
> -sprintf(p, "%i", priority);
> -r = exec_command_append(s->control_command, "-p", p, NULL);
> -if (r < 0)
> -goto fail;
> -}
> -
> -if (discard && !streq(discard, "none")) {
> -const char *discard_arg;
> -
> -if (streq(discard, "all"))
> -discard_arg = "--discard";
> -else
> -discard_arg = strjoina("--discard=", discard);
> -
> -r = exec_command_append(s->control_command, discard_arg, 
> NULL);
> +if (s->parameters_fragment.options || opts) {
> +

Re: [systemd-devel] [PATCH] swap: use swapon -o

[systemd github import bot]

Patchset imported to github.
Pull request:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] swap: use swapon -o

[Karel Zak]

This patch simplify swapon usage in systemd. The command swapon(8)
since util-linux v2.26 supports "-o ". The idea is exactly the
same like for mount(8). The -o specifies options in fstab-compatible
way. For systemd it means that it does not have to care about things
like "discard" or another swapon specific options.

swapon -o 

For backward compatibility the code cares about "Priority:" swap unit
field (for a case when Priority: is set, but pri= in the Options: is
missing).

References: 
http://lists.freedesktop.org/archives/systemd-devel/2014-October/023576.html
---
V2:
 - update README
 - add hint to systemd.swap man page
 - don't care about pri= in systed-fstab-generator at all
 - add warning about duplicate priority configuration
 - use warning rather than notice for non-parsable pri=

 README|  2 +-
 man/systemd.swap.xml  |  3 ++-
 src/core/swap.c   | 43 +++
 src/fstab-generator/fstab-generator.c | 28 ---
 4 files changed, 26 insertions(+), 50 deletions(-)

diff --git a/README b/README
index 039110e..2b8c68e 100644
--- a/README
+++ b/README
@@ -136,7 +136,7 @@ REQUIREMENTS:
 During runtime, you need the following additional
 dependencies:
 
-util-linux >= v2.25 required
+util-linux >= v2.26 required
 dbus >= 1.4.0 (strictly speaking optional, but recommended)
 dracut (optional)
 PolicyKit (optional)
diff --git a/man/systemd.swap.xml b/man/systemd.swap.xml
index 5016f45..c398677 100644
--- a/man/systemd.swap.xml
+++ b/man/systemd.swap.xml
@@ -177,7 +177,8 @@
 
 Swap priority to use when activating the swap
 device or file. This takes an integer. This setting is
-optional.
+optional and ignored when priotiry is set by pri= in 
the
+Options= option.
   
 
   
diff --git a/src/core/swap.c b/src/core/swap.c
index 12ebf84..193c8c3 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -717,8 +717,8 @@ fail:
 }
 
 static void swap_enter_activating(Swap *s) {
-_cleanup_free_ char *discard = NULL;
-int r, priority = -1;
+_cleanup_free_ char *opts = NULL;
+int r;
 
 assert(s);
 
@@ -726,13 +726,21 @@ static void swap_enter_activating(Swap *s) {
 s->control_command = s->exec_command + SWAP_EXEC_ACTIVATE;
 
 if (s->from_fragment) {
-fstab_filter_options(s->parameters_fragment.options, 
"discard\0", NULL, &discard, NULL);
+int priority = -1;
 
-priority = s->parameters_fragment.priority;
-if (priority < 0) {
-r = fstab_find_pri(s->parameters_fragment.options, 
&priority);
+r = fstab_find_pri(s->parameters_fragment.options, &priority);
+if (r < 0)
+log_warning_errno(r, "Failed to parse swap priority 
\"%s\", ignoring: %m", s->parameters_fragment.options);
+else if (r == 1 && s->parameters_fragment.priority >= 0)
+log_warning("Duplicate swap priority configuration by 
Priority and Options fields.");
+
+if (r <= 0 && s->parameters_fragment.priority >= 0) {
+if (s->parameters_fragment.options)
+r = asprintf(&opts, "%s,pri=%i", 
s->parameters_fragment.options, s->parameters_fragment.priority);
+else
+r = asprintf(&opts, "pri=%i", 
s->parameters_fragment.priority);
 if (r < 0)
-log_notice_errno(r, "Failed to parse swap 
priority \"%s\", ignoring: %m", s->parameters_fragment.options);
+goto fail;
 }
 }
 
@@ -740,24 +748,9 @@ static void swap_enter_activating(Swap *s) {
 if (r < 0)
 goto fail;
 
-if (priority >= 0) {
-char p[DECIMAL_STR_MAX(int)];
-
-sprintf(p, "%i", priority);
-r = exec_command_append(s->control_command, "-p", p, NULL);
-if (r < 0)
-goto fail;
-}
-
-if (discard && !streq(discard, "none")) {
-const char *discard_arg;
-
-if (streq(discard, "all"))
-discard_arg = "--discard";
-else
-discard_arg = strjoina("--discard=", discard);
-
-r = exec_command_append(s->control_command, discard_arg, NULL);
+if (s->parameters_fragment.options || opts) {
+r = exec_command_append(s->control_command, "-o",
+opts ? : s->parameters_fragment.options, NULL);
 if (r < 0)
 goto fail;
 }
diff --git a/src/fstab-generator/fstab-generator.c 
b/src/fstab-generator/fstab-generator.c
index 3

Re: [systemd-devel] [PATCH] swap: use swapon -o

[Lennart Poettering]

On Thu, 21.05.15 18:02, Karel Zak (k...@redhat.com) wrote:

> On Thu, May 21, 2015 at 04:45:44PM +0200, Lennart Poettering wrote:
> > I think it might be a good idea to log a warning if the priority is
> > specified both in s->parameters_frament.priority and in
> > s->parameters_fragment.options.
> 
> This (priority specified by both fields) is standard fstab-generator 
> behaviour.
> Maybe the generator should be also updated to stop generate "Priority:".

Yeah, I agree. It should pass the option string as is, given that we
can do so now, and not parse it in any way and generate secondary
fields from it.

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] swap: use swapon -o

[Karel Zak]

On Thu, May 21, 2015 at 04:45:44PM +0200, Lennart Poettering wrote:
> I think it might be a good idea to log a warning if the priority is
> specified both in s->parameters_frament.priority and in
> s->parameters_fragment.options.

This (priority specified by both fields) is standard fstab-generator behaviour.
Maybe the generator should be also updated to stop generate "Priority:".

Karel

-- 
 Karel Zak  
 http://karelzak.blogspot.com
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] swap: use swapon -o

[Lennart Poettering]

On Wed, 20.05.15 17:54, Karel Zak (k...@redhat.com) wrote:

> This patch simplify swapon usage in systemd. The command swapon(8)
> since util-linux v2.26 supports "-o ". The idea is exactly the
> same like for mount(8). The -o specifies options in fstab-compatible
> way. For systemd it means that it does not have to care about things
> like "discard" or another swapon specific options.

I figure the README should be updated as part of the patch to require
util-linux 2.26 then.

>  static void swap_enter_activating(Swap *s) {
> -_cleanup_free_ char *discard = NULL;
> -int r, priority = -1;
> +_cleanup_free_ char *opts = NULL;
> +int r;
>  
>  assert(s);
>  
> @@ -726,13 +726,19 @@ static void swap_enter_activating(Swap *s) {
>  s->control_command = s->exec_command + SWAP_EXEC_ACTIVATE;
>  
>  if (s->from_fragment) {
> -fstab_filter_options(s->parameters_fragment.options, 
> "discard\0", NULL, &discard, NULL);
> +int priority = -1;
> +
> +r = fstab_find_pri(s->parameters_fragment.options, 
> &priority);
> +if (r < 0)
> +log_notice_errno(r, "Failed to parse swap
> priority \"%s\", ignoring: %m", s->parameters_fragment.options);

So far we logged such non-fatal errors as LOG_WARNING. Could you
upgrade this from log_notice_errno() to log_warning_errno() please?

(I am aware the the previous code used LOG_NOTICE, but this should be
fixed I think)

>  
> -priority = s->parameters_fragment.priority;
> -if (priority < 0) {
> -r = fstab_find_pri(s->parameters_fragment.options, 
> &priority);
> +if (r <= 0 && s->parameters_fragment.priority >= 0) {
> +if (s->parameters_fragment.options)
> +r = asprintf(&opts, "%s,pri=%i", 
> s->parameters_fragment.options, s->parameters_fragment.priority);
> +else
> +r = asprintf(&opts, "pri=%i", 
> s->parameters_fragment.priority);
>  if (r < 0)
> -log_notice_errno(r, "Failed to
> -parse swap priority \"%s\", ignoring: %m",
> -s->parameters_fragment.options);
> +goto fail;
>  }

I think it might be a good idea to log a warning if the priority is
specified both in s->parameters_frament.priority and in
s->parameters_fragment.options.

Otherwise looks good!

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] swap: use swapon -o

[systemd github import bot]

Patchset imported to github.
Pull request:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] swap: use swapon -o

[Karel Zak]

This patch simplify swapon usage in systemd. The command swapon(8)
since util-linux v2.26 supports "-o ". The idea is exactly the
same like for mount(8). The -o specifies options in fstab-compatible
way. For systemd it means that it does not have to care about things
like "discard" or another swapon specific options.

swapon -o 

For backward compatibility the code cares about "Priority:" swap unit
field (for a case when Priority: is set, but pri= in the Options: is
missing).

References: 
http://lists.freedesktop.org/archives/systemd-devel/2014-October/023576.html
---
 src/core/swap.c | 41 -
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/src/core/swap.c b/src/core/swap.c
index 12ebf84..658ac8b 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -717,8 +717,8 @@ fail:
 }
 
 static void swap_enter_activating(Swap *s) {
-_cleanup_free_ char *discard = NULL;
-int r, priority = -1;
+_cleanup_free_ char *opts = NULL;
+int r;
 
 assert(s);
 
@@ -726,13 +726,19 @@ static void swap_enter_activating(Swap *s) {
 s->control_command = s->exec_command + SWAP_EXEC_ACTIVATE;
 
 if (s->from_fragment) {
-fstab_filter_options(s->parameters_fragment.options, 
"discard\0", NULL, &discard, NULL);
+int priority = -1;
+
+r = fstab_find_pri(s->parameters_fragment.options, &priority);
+if (r < 0)
+log_notice_errno(r, "Failed to parse swap priority 
\"%s\", ignoring: %m", s->parameters_fragment.options);
 
-priority = s->parameters_fragment.priority;
-if (priority < 0) {
-r = fstab_find_pri(s->parameters_fragment.options, 
&priority);
+if (r <= 0 && s->parameters_fragment.priority >= 0) {
+if (s->parameters_fragment.options)
+r = asprintf(&opts, "%s,pri=%i", 
s->parameters_fragment.options, s->parameters_fragment.priority);
+else
+r = asprintf(&opts, "pri=%i", 
s->parameters_fragment.priority);
 if (r < 0)
-log_notice_errno(r, "Failed to parse swap 
priority \"%s\", ignoring: %m", s->parameters_fragment.options);
+goto fail;
 }
 }
 
@@ -740,24 +746,9 @@ static void swap_enter_activating(Swap *s) {
 if (r < 0)
 goto fail;
 
-if (priority >= 0) {
-char p[DECIMAL_STR_MAX(int)];
-
-sprintf(p, "%i", priority);
-r = exec_command_append(s->control_command, "-p", p, NULL);
-if (r < 0)
-goto fail;
-}
-
-if (discard && !streq(discard, "none")) {
-const char *discard_arg;
-
-if (streq(discard, "all"))
-discard_arg = "--discard";
-else
-discard_arg = strjoina("--discard=", discard);
-
-r = exec_command_append(s->control_command, discard_arg, NULL);
+if (s->parameters_fragment.options || opts) {
+r = exec_command_append(s->control_command, "-o",
+opts ? : s->parameters_fragment.options, NULL);
 if (r < 0)
 goto fail;
 }
-- 
2.1.0

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel