Re: [Qemu-devel] [RFC PATCH v3 03/11] Add a script to extract VSS SDK headers on POSIX system

[Tomoki Sekiyama]

On 5/21/13 12:48 , "Eric Blake" <address@hidden> wrote:
>On 05/21/2013 09:33 AM, Tomoki Sekiyama wrote:
>> VSS SDK(*) setup.exe is only runnable on Windows. This adds a script
>> to extract VSS SDK headers on POSIX-systems using msitools.
>> 
>>   * http://www.microsoft.com/en-us/download/details.aspx?id=23490
>> 
>> From: Paolo Bonzini <address@hidden>
>> Signed-off-by: Tomoki Sekiyama <address@hidden>
>> ---
>>  scripts/extract-vsssdk-headers |   25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100755 scripts/extract-vsssdk-headers
>> 
>
>> +#! /bin/bash
>
>Since you are using bash...
>
>> +
>> +# extract-vsssdk-headers
>> +# Author: Paolo Bonzini <address@hidden>
>> +
>> +set -e
>> +if test $# = 0 || ! test -f "$1"; then
>
>Why reject 0 arguments but not 2?  Shouldn't the first check be test $#
>!= 1?

I agree, will fix this.

>> +  echo 'Usage: extract-vsssdk-headers /path/to/setup.exe'
>> +  exit 1
>> +fi
>> +
>> +# Extract .MSI file in the .exe, looking for the OLE compound
>> +# document signature.  Extra data at the end does not matter.
>> +export LC_ALL=C
>> +MAGIC=$'\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1'
>> +offset=`grep -abom1 "$MAGIC" "$1" | sed -n 's/:/\n/; P' `
>
>...I'd prefer $() over ``.

OK.

>> +(dd of=/dev/null skip=$offset bs=1 count=0; cat) < "$1" > vsssdk.msi
>> +
>> +# Now extract the files.
>> +tmpdir=tmp$$
>> +mkdir $tmpdir
>
>...also, this name is rather predictable; adding a $RANDOM might help
>improve its quality.

I will add $RANDOM here.

>> +msiextract -C $tmpdir vsssdk.msi
>> +mv "$tmpdir/Program Files/Microsoft/VSSSDK72/inc" inc
>> +rm -rf $tmpdir vsssdk.msi
>
>What, no trap, to guarantee clean up of the temp directory even if you
>Ctrl-C the script in the middle of a potentially long-running msiextract?

OK, I will add some trap here.

Maybe it also should have an additional check and a message to install
Msitools for the case msiextract isn't exectable.

Thanks,
-- 
Tomoki Sekiyama