Re: [Xen-devel] [PATCH v2 6/6] RFC: tools/dm_restrict: Enable QEMU sandboxing

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

> 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 ?

> failed="false"

Quotes are not needed here and seem un-idiomatic to me when the RHS is
a simple atom.

> # 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.

> # 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" ||:

> 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#*  })
DTRT.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel