tools/hotplug/Linux/launch-xenstore.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
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
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
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
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.
© 2016 - 2024 Red Hat, Inc.