Re: [PATCH] iotests: Quote $cmd in _send_qemu_cmd

[Max Reitz <mreitz@redhat.com>]

On 15.12.20 12:43, Kevin Wolf wrote:
> Am 14.12.2020 um 18:58 hat Max Reitz geschrieben:
>> With bash 5.1, the output of the following script (which creates an
>> array with a single element, then takes a single-element slice from that
>> array, and echos the result) changes:
>>
>>    a=("double  space")
>>    a=${a[@]:0:1}
>>    echo "$a"
>>
>> from "double space" to "double  space", i.e. all white space is
>> preserved as-is.  This is probably what we actually want here (judging
>> from the "...to accommodate pathnames with spaces" comment), but before
>> 5.1, we have to quote the ${} slice to get the same behavior.
>>
>> In any case, without quoting, the reference output of many iotests is
>> different between bash 5.1 and pre-5.1, which is not very good.  We
>> should fix it by quoting here, and then adjusting all the reference
>> output of all iotests using _send_qemu_cmd accordingly.  (The only
>> ones we do not need to change are those that do not have multiple
>> consecutive whitespaces in their _send_qemu_cmd parameters.)
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Alternatively, we could explicitly collapse all whitespace sequences
>> into single spaces, but I believe that would defeat the purpose of
>> "accomodat[ing] pathnames with spaces".
> 
> I agree that quoting is the better alternative.
> 
> The whole thing shouldn't be needed if callers propertly quote
> filenames. If I understand correctly, these lines just try to merge
> back filenames that have been split into multiple words because the
> caller wasn't careful. But we can't reliably do that because we don't
> know which and how much whitespace was removed.

No, without quoting here, the slice operation will actually collapse 
spaces inside of array elements, as shown in the commit message
(a=("x  y") creates an array with a single element that just has two 
spaces in it).  If the caller didn’t quote, then quoting in 
_send_qemu_cmd wouldn’t help either, because the whitespace would no 
longer be present in $@.

Most callers of _send_qemu_cmd actually do something like this:

_send_qemu_cmd \
     $QEMU_HANDLE \
     "{ 'execute': 'something',
        'arguments': ... }" \
     'return'

So they do quote all of their arguments, and actually only pass three 
(or four, with success_or_failure=y), so basically the slicing is 
generally unnecessary and we’d just need cmd=$1 (not $2, because 
_send_qemu_cmd likes to invoke “shift”).

In fact, if you do that on top of this patch (cmd=$1 instead of the 
fancy slice), you’ll see that the only test that then doesn’t pass is 
102.  I think that’s because it has this one _send_qemu_cmd invocation 
that forgot to pass $QEMU_HANDLE...?

So, alternatively to quoting the slice, we could also just set cmd=$1 
and fix 102.

>> I used this script to verify that the git-diff only changes whitespace
>> (except for the hunk in common.qemu):
>>
>>    https://gist.github.com/XanClic/41cfcc2ac4619421883e8a97790f5c9e
> 
> 'git diff --word-diff' is good enough for me.

Learn a new thing every day.

> This seems to be based on your branch (312.out doesn't exist yet for
> me), so instead of merging, you can just have my:
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

In any case (because perhaps we just want cmd=$1, I don’t know), thanks. :)

Max