[Ovirt-devel] [PATCH trivial] Create developer appliances under Ubuntu v2

[Jeff Schroeder]

On Sat, Jun 28, 2008 at 1:34 AM, Jim Meyering <jim at meyering.net> wrote:
> "Jeff Schroeder" <jeffschroed at gmail.com> wrote:
>> On Fri, Jun 27, 2008 at 4:13 PM, Jeff Schroeder <jeffschroed at gmail.com> wrote:
>> ...snip...
>>> An updated patch is attached that hopefully addresses your concerns.
>>
>> Ok I changed something after testing the previous patch and flipped
>> the logic for some
>> stupid reason. This makes more sense and fixes the package dependency check.
>>
>> Please ignore the last email and take a look at this patch instead.
>>
>>
>> diff --git a/wui-appliance/create-wui-appliance.sh b/wui-appliance/create-wui-appliance.sh
>> index 18d7983..fdf1206 100755
>> --- a/wui-appliance/create-wui-appliance.sh
>> +++ b/wui-appliance/create-wui-appliance.sh
>> @@ -122,7 +122,7 @@ gen_fake_managed_node() {
>>    <on_reboot>restart</on_reboot>
>>    <on_crash>destroy</on_crash>
>>    <devices>
>> -    <emulator>$KVM_BINARY</emulator>
>> +    <emulator>$KVM</emulator>
>>      <interface type='network'>
>>        <mac address='00:16:3e:12:34:$last_mac'/>
>>        <source network='dummybridge'/>
>> @@ -155,7 +155,7 @@ gen_app() {
>>    <on_reboot>restart</on_reboot>
>>    <on_crash>destroy</on_crash>
>>    <devices>
>> -    <emulator>$KVM_BINARY</emulator>
>> +    <emulator>$KVM</emulator>
>>      <disk type='file' device='disk'>
>>        <source file='$disk'/>
>>        <target dev='hda'/>
>> @@ -180,23 +180,21 @@ fi
>>
>>  # now make sure the packages we need are installed
>>  if [ -e /etc/redhat-release ]; then
>> -    PACKAGES="libvirt kvm virt-manager virt-viewer"
>> -    CHECK=$(rpm $(printf " -q %s " "$PACKAGES")  &> /dev/null; echo $?)
>> -    KVM_BINARY=/usr/bin/qemu-kvm
>> +    PACKAGES='libvirt kvm virt-manager virt-viewer'
>> +    rpm $(printf " -q %s " "$PACKAGES") > /dev/null 2>&1 && DEPS_MET=1
>> +    KVM=/usr/bin/qemu-kvm
>>  elif [ -e /etc/debian_version ]; then
>>      # Works in Ubuntu 8.04. Still needs testing in Debian
>> -    PACKAGES="libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer"
>> -    CHECK=$(dpkg -l $PACKAGES &> /dev/null; echo $?)
>> -    KVM_BINARY=/usr/bin/kvm
>> +    PACKAGES='libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer'
>> +    dpkg -l $PACKAGES > /dev/null 2>&1 && DEPS_MET=1
>
> The idiom I suggested, "; some_var=$?", guaranteed that
> $some_var would be defined.
> Since you've opted to use " && DEPS_MET=1",
> it's your responsibility to define DEPS_MET when they're not met.
> Otherwise, if DEPS_MET=1 happens to be set in the environment
> (yeah, not likely), it will stay that way, even when the dependencies
> are not met.
The point on && was to set it to 1 if the dependencies are actually met.
I misunderstood your previous email. It was just the subshell, not ; ... $?
In doing it this way, "unset DEPS_MET" should have been first done.


>
>> +    KVM=/usr/bin/kvm
>>  else
>>      die "Not a supported system"
>>  fi
>> -
>> -if [ $CHECK -ne 0 ]; then
>> +if [ ${DEPS_MET:-0} -ne 1 ]; then
>
> Then you can use a cleaner comparison:
>
>  if [ $DEPS_MET = 0 ]; then
>
> By the way, please don't use "-ne" for arithmetic comparisons.
> "=" works fine and its shorter and more readable.

You mean this, right? If we set DEPS_MET=$? 0 is a true return and
nonzero is false.
if [ $DEPS_MET != 0 ]; then

-ne, -lt, -gt are all specifically for integer comparison. The purist
in me is going to have to
politely disagree on this one. != is more for string comparison but
most shells use it for
integer comparison also ontop of -ne.

What is wrong with the brace expansion? Hating to pull out the
reference, but brace expansion
is part of the Single Unix Specification:
http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02

>
>>      # one of the previous packages wasn't installed; bail out
>> -    die "Must have the libvirt, kvm, virt-manager, and virt-viewer packages installed"
>> +    die "Missing a package from this list: $PACKAGES"
>>  fi
>> -
>>  if [ $devel = 1 ]; then
>>      NAME=developer
>>      BRIDGENAME=dummybridge
>

Sorry I'm unexpectedly travelling and don't have git available at the
moment. Later this
weekend or monday you'll get a third version + some other small patches.

thanks again for the review.

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com