[PATCH] tools: fix oom setting of xenstored

[PATCH] tools: fix oom setting of xenstored

[Juergen Gross]

Commit f282182af32939 ("tools/xenstore: set oom score for xenstore
daemon on Linux") introduced a regression when not setting the oom
value in the xencommons file. Fix that.

Fixes: f282182af32939 ("tools/xenstore: set oom score for xenstore daemon on Linux")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/hotplug/Linux/launch-xenstore.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 8438af9977..2b99b98896 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -60,7 +60,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 		echo "No xenstored found"
 		exit 1
 	}
-	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
+	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] && XENSTORED_OOM_MEM_THRESHOLD=50
 	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))
 
 	[ "$XENSTORED_MAX_OPEN_FDS" = "unlimited" ] || {
-- 
2.26.2

Re: [PATCH] tools: fix oom setting of xenstored

[Jan Beulich]

On 19.10.2021 06:41, Juergen Gross wrote:
> --- a/tools/hotplug/Linux/launch-xenstore.in
> +++ b/tools/hotplug/Linux/launch-xenstore.in
> @@ -60,7 +60,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>  		echo "No xenstored found"
>  		exit 1
>  	}
> -	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
> +	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] && XENSTORED_OOM_MEM_THRESHOLD=50

Is resilience against "set -e" being in effect of interest? If so I
think this would want to be

	[ -n "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50

>  	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))

Alternatively, how about dropping the line above and using

 	XS_OOM_SCORE=-$((${XENSTORED_OOM_MEM_THRESHOLD:-50} * 10))

here?

Jan

Re: [PATCH] tools: fix oom setting of xenstored

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1546 bytes --]

On 19.10.21 08:54, Jan Beulich wrote:
> On 19.10.2021 06:41, Juergen Gross wrote:
>> --- a/tools/hotplug/Linux/launch-xenstore.in
>> +++ b/tools/hotplug/Linux/launch-xenstore.in
>> @@ -60,7 +60,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>>   		echo "No xenstored found"
>>   		exit 1
>>   	}
>> -	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
>> +	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] && XENSTORED_OOM_MEM_THRESHOLD=50
> 
> Is resilience against "set -e" being in effect of interest? If so I
> think this would want to be

I don't think set -e would have a negative effect on above line. The
bash man-page tells me that:

   The shell does not exit if the command that fails is part of the
   command list immediately following a while or until keyword, part of
   the test following the if, ...

And I believe that "[ ... ]" is treated like an "if". A short test
showed that bash indeed does not exit in this case:

   ( set -e; [ -z "" ] && xx=okay; echo $xx; )

This will print "okay", so bash didn't exit.

> 
> 	[ -n "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
> 
>>   	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))
> 
> Alternatively, how about dropping the line above and using
> 
>   	XS_OOM_SCORE=-$((${XENSTORED_OOM_MEM_THRESHOLD:-50} * 10))
> 
> here?

This would be an option, yes.

Unless a maintainer wants me to send another patch with this change I'll
keep it as it is now.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] tools: fix oom setting of xenstored

[Jan Beulich]

On 19.10.2021 09:31, Juergen Gross wrote:
> On 19.10.21 08:54, Jan Beulich wrote:
>> On 19.10.2021 06:41, Juergen Gross wrote:
>>> --- a/tools/hotplug/Linux/launch-xenstore.in
>>> +++ b/tools/hotplug/Linux/launch-xenstore.in
>>> @@ -60,7 +60,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>>>   		echo "No xenstored found"
>>>   		exit 1
>>>   	}
>>> -	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
>>> +	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] && XENSTORED_OOM_MEM_THRESHOLD=50
>>
>> Is resilience against "set -e" being in effect of interest? If so I
>> think this would want to be
> 
> I don't think set -e would have a negative effect on above line. The
> bash man-page tells me that:
> 
>    The shell does not exit if the command that fails is part of the
>    command list immediately following a while or until keyword, part of
>    the test following the if, ...
> 
> And I believe that "[ ... ]" is treated like an "if".

I don't think so - "[ ... ]" is an equivalent of "test ...", i.e.
unrelated to whether that's an operand of "if". The question is
what effect && has, i.e. the behavior is due to what you've
hidden by using ... in your quotation: "..., and is not a part of
an AND or OR list, ...".

I think I recall constructs like the one you use not working with
"set -e" on at least some bash versions, though. Apparently this
was due to a bash bug then (or I'm misremembering, but that's not
overly likely since some of my long used scripts specifically
avoid using && in such situations).

> A short test
> showed that bash indeed does not exit in this case:
> 
>    ( set -e; [ -z "" ] && xx=okay; echo $xx; )
> 
> This will print "okay", so bash didn't exit.

Of course, because the left side of && succeeds. You'd need

   ( set -e; [ -z "xxx" ] && xx=okay; echo xx=$xx; )

and observe "xx=" getting printed. Which indeed I do observe on
the one bash version I've tried to double check. But that one's
surely newer than what I think I saw such problems on.

Jan

Re: [PATCH] tools: fix oom setting of xenstored

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] tools: fix oom setting of xenstored"):
> On 19.10.2021 09:31, Juergen Gross wrote:
> > I don't think set -e would have a negative effect on above line. The
> > bash man-page tells me that:
> > 
> >    The shell does not exit if the command that fails is part of the
> >    command list immediately following a while or until keyword, part of
> >    the test following the if, ...
> > 
> > And I believe that "[ ... ]" is treated like an "if".
> 
> I don't think so - "[ ... ]" is an equivalent of "test ...", i.e.
> unrelated to whether that's an operand of "if". The question is
> what effect && has, i.e. the behavior is due to what you've
> hidden by using ... in your quotation: "..., and is not a part of
> an AND or OR list, ...".

"[ ... ]" is precisely equivalent to "test ...".  But neither of these
is eqivalent to an "if".  When the docs say "part of the test
following the if" they are using the word "test" informally.

> I think I recall constructs like the one you use not working with
> "set -e" on at least some bash versions, though. Apparently this
> was due to a bash bug then (or I'm misremembering, but that's not
> overly likely since some of my long used scripts specifically
> avoid using && in such situations).

I agree that we should avoid this && construct.  Can we not just use
conditional assignment of a default value or an if statement ?

> >    ( set -e; [ -z "" ] && xx=okay; echo $xx; )
> > 
> > This will print "okay", so bash didn't exit.
> 
> Of course, because the left side of && succeeds. You'd need
> 
>    ( set -e; [ -z "xxx" ] && xx=okay; echo xx=$xx; )
> 
> and observe "xx=" getting printed. Which indeed I do observe on
> the one bash version I've tried to double check. But that one's
> surely newer than what I think I saw such problems on.

I think this particular && and || usage is not an idiomatic way of
spelling what would normally be a conditional in shell.

I think
  try_this || try_that
is fine but
  variable_nonempty || variable=value
is strange.

I would use ${param:=default_setting} or ${param:-default}
(or perhaps the colon-less variants).

Ian.