On 09/24/2018 12:21 PM, Ian Jackson wrote:
> Apropos of our conversation on IRC, I looked at the checker script in
> detail.
>
>> #!/bin/bash
>>
>> domain="$1"
>
> Just noticed this, but: OMG no `set -e'.
> You probably want `set -o pipefail' too.
`set -e` never made any sense to me -- that's not the way I code in any
other language; why would scripting be any different? What's the
advantage of doing that in the current script?
>> dmpid=$(xenstore-read /local/domain/$domid/image/device-model-pid
>> 2>/dev/null)
>> if [[ -z "$dmpid" ]] ; then
>> echo "xenstore-read failed"
>> exit 1
>> fi
>
> Why do you throw away the stderr from xenstore-read ?
That's left over from a previous version of the script, where I didn't
check to see whether $1 was numeric, but rather tried to interpret it as
numeric and if it failed, then ran `xl domid` on it. I can take that out.
>
>> failed="false"
>
> Quotes are not needed here and seem un-idiomatic to me when the RHS is
> a simple atom.
Sure.
>> # TEST: Process / group id
> ...
>> # FIXME: deal with other UID configurations?
>
> Since the test will fail if you don't do this, I think this is very
> sub-critical and you could drop the fixme.
This was really a question for the reviewer; probably it should have
been RFC rather than FIXME.
The question is: should the script handle the case where
`xen-qemuuser-shared` is defined instead of `xen-qemuuser-range-base`?
>> # Example input:
>> # Uid: 1193 1193 1193 1193
>> input=$(grep Uid /proc/$dmpid/status)
>
> I have commented on this grep, and the subsequent regexpery, already.
>
> But also: you check the uid and the gid, but by duplicating the code.
> Surely this could be a shell function.
>
>> echo -n "Process UID: "
>
> If this had been me, I would have written
> begin_test "Process UID"
> and then
>
>> result="PASSED"
>
> test_passed
>
>> for i in {1..4}; do
>> if [[ "${BASH_REMATCH[$i]}" != "$tgt_uid" ]] ; then
>> result="FAILED"
>> failed="true"
>
> In particular, you open-code setting `failed' everywhere. If you miss
> one then that could hide a test failure. So
> test_failed
> But you want to print a reason so
> test_failed "got $uidorgid $actual_id wanted $tgt_id"
>
> As a bonus, doing this now means you can fix the test output format to
> be more parseable by changing the code in one place.
>
>> if [[ "$root" != "$tgt_chroot" ]] ; then
>> echo "FAILED"
>
> You could introduce
> test_condition 'root directory' "$root" != "$tgt_chroot"
> which calls test_passed or test_failed as appropriate.
>
> If you have it return the same exit status as the test, you can use it
> for the uids where you would say
> test_condition "one $uidorgid" $actual_id = $tgt_id || break
> and the rest of the time you would have to write
> test_condition 'root directory' "$root" != "$tgt_chroot" ||:
I'll take a look at doing this if we decide to stick with bash.
>> function check_rlimit() {
>> limit_name=$1
>> limit_string=$2
>> tgt=$3
>>
>> echo -n "rlimit $limit_name: "
>> input=$(grep "^$limit_string" /proc/$dmpid/limits)
> ...
>> if [[ "$input" =~
>> ^$limit_string[[:space:]]*([^[:space:]]+)[[:space:]]*([^[:space:]]+)[[:space:]]*[^[:space:]]+
>> ]] ; then
>
> Because of the unfortunate format of /proc/PID/limits, you do can't
> just do the
> fields=($input)
> trick but
> fields=(${input#* })
What will this do?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
