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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/25/2018 12:02 PM, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH v2 6/6] RFC: tools/dm_restrict: Enable QEMU 
> sandboxing"):
>> On 09/24/2018 12:21 PM, Ian Jackson wrote:
>>> 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?
> 
> ?
> 
> Many modern languages throw exceptions.  set -e is a bit like that.
> If you don't say set -e then you need to wrap every everything in your
> entire script with an error check.

I'm not sure you can call such languages "modern" anymore. :-)  Neither
golang nor rust have exceptions, for example, by deliberate choice. I've
read golang's rationale for why not, and I tend to agree with them.
People complain about the fact that you always have to check errors in
golang, but it generally prods you to think about what would actually
happen if something went wrong at each point and do something sensible.
Rust seems to be following a similar example.

I suppose the difference between bash and golang / rust is that the
compiler prods you to actually do the checking, whereas in bash nothing
prods you to do the check; so `set -e` is actually somewhat safer: if
you forget to check a critical success / failure, exiting is probably
going to cause less havoc than blindly continuing.

> For example, you write
> 
>>>> 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.
> 
> but with set -e you can write only
> 
>     dmpid=$(xenstore-read /local/domain/$domid/image/device-model-pid)
> 
> and the subsequent if is not needed.

I think you misunderstood what I meant.  The original code looked
something like this:

# See if $domain is a domid
dmpid=$(xenstore-read /local/domain/$domain/... >2 /dev/null )
if [[ -z "$dmpid" ]] ; then
  # That didn't work; maybe it was a domain name instead
  domid=$(xl domid $domain)
  dmpid=$(xenstore-read /local/domain/$domid/...)

  # ... more error handling
fi

Without the pipe to /dev/null, if a user uses a domain name (i.e.,
'c6-01'), then the script will succeed, but there will be a spurious
error message when the first xenstore-read fails.

With `set -e`, I'd not only have to pipe to null, I'd also have to add
in "|| true" or something to keep the shell from exiting on an error.

In the current code, printing the error message is obviously better than
throwing it away.  But I still feel better checking the result and
giving a sensible follow-up error message than having a failure silently
exit, because I prefer to explicitly think about what happens when
things fail (and remind the people reading the code to do so as well).

>> The question is: should the script handle the case where
>> `xen-qemuuser-shared` is defined instead of `xen-qemuuser-range-base`?
> 
> I think it is fine if it doesn't.  If someone wants that feature it is
> easy to add it.

Ack

>>>> 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?
> 
> The expression
>   ${input#*  }
> is the contents of input with the shortest prefix matching `*  '
> stripped off the front.  That will eat all the words at the start,
> which are separated by one space each, and find the first pair of
> adjacent spaces.  So if input is
>   'Max processes             63603                63603                
> processes 
> then "${input#*  }" is
>   '           63603                63603                processes 
> 
> Expanding it without the surrounding " " causes it to be word-split in
> the usual way, producing
>   63603 63603 processes
> and then this is assigned to a new bash array variable `fields'
> which can then be indexed to find the values.

That's an idea.  I'm not sure I like it much better than using a regexp
(now that the runes have been written), but I'll think about it.

 -George

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