WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] fix xendomains service

To: Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] fix xendomains service
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Wed, 8 Jun 2011 09:45:29 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 08 Jun 2011 01:46:21 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <72a098061cbd894976b0.1307480575@xxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <72a098061cbd894976b0.1307480575@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Tue, 2011-06-07 at 22:02 +0100, Zhigang Wang wrote:
> # HG changeset patch
> # User Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
> # Date 1307480010 14400
> # Node ID 72a098061cbd894976b0bf9e0f96cc1e1c2897b5
> # Parent  37c77bacb52aa7795978b994f9d371b979b2cb07
> fix xendomains service
> 
> This patch fixes the following problems of xendomains service:

In general we would prefer each unrelated change in an individual patch.
If your commit message is a list of unrelated issues then that is a good
sign you need to consider splitting them up.

> 
> 1. Bash regex:
> 
>        if [[ "(state -b----)" =~ '(state' ]]; then echo "works"; fi
> 
>    works on bash 4.x, not 3.x.
> 
>    Put it into a variable works for both:
> 
>        state_re="\(state"; if [[ "(state -b----)" =~ $state_re ]]; then echo 
> "works"; fi

This is pretty nasty looking in both cases, but OK, I guess we don't
have anything much better available.

> 2. Change XENDOMAINS_AUTO_ONLY=true. Then on service stop, it will only
>    save or migrate all VMs under XENDOMAINS_AUTO (/etc/xen/auto), but always
>    shutdown all the running VMs.

Why is this change in default behaviour desirable? What makes it better
in the general case than the existing default?

> 3. Fix: "state" as well as "name" and "id", is used but never defined.

OK.

> 4. Remove the incorrect eval.

Which incorrect eval? (this might be obvious if it was a separate
changeset but it is hidden in the rest of the changes in this changes).
How is it incorrect and what makes it correct now? 

> Signed-off-by: Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
> 
> diff -r 37c77bacb52a -r 72a098061cbd 
> tools/hotplug/Linux/init.d/sysconfig.xendomains
> --- a/tools/hotplug/Linux/init.d/sysconfig.xendomains Mon May 23 17:38:28 
> 2011 +0100
> +++ b/tools/hotplug/Linux/init.d/sysconfig.xendomains Tue Jun 07 16:53:30 
> 2011 -0400
> @@ -103,7 +103,7 @@ XENDOMAINS_RESTORE=true
>  XENDOMAINS_AUTO=/etc/xen/auto
>  
>  ## Type: boolean
> -## Default: false
> +## Default: true
>  # 
>  # If this variable is set to "true", only the domains started via config 
>  # files in XENDOMAINS_AUTO will be treated according to XENDOMAINS_SYSRQ,
> @@ -111,7 +111,7 @@ XENDOMAINS_AUTO=/etc/xen/auto
>  # all running domains will be. 
>  # Note that the name matching is somewhat fuzzy.
>  # 
> -XENDOMAINS_AUTO_ONLY=false
> +XENDOMAINS_AUTO_ONLY=true
>  
>  ## Type: integer
>  ## Default: 300
> diff -r 37c77bacb52a -r 72a098061cbd tools/hotplug/Linux/init.d/xendomains
> --- a/tools/hotplug/Linux/init.d/xendomains   Mon May 23 17:38:28 2011 +0100
> +++ b/tools/hotplug/Linux/init.d/xendomains   Tue Jun 07 16:53:30 2011 -0400
> @@ -204,15 +204,21 @@ rdnames()
>  
>  parseln()
>  {
> -    if [[ "$1" =~ '(domain' ]]; then
> +    domain_re="\(domain"
> +    name_re="\(name"
> +    domid_re="\(domid"
> +    state_re="\(state"
> +    if [[ "$1" =~ $domain_re ]]; then
>          name=;id=
> -    else if [[ "$1" =~ '(name' ]]; then
> +    else if [[ "$1" =~ $name_re ]]; then
>          name=$(echo $1 | sed -e 's/^.*(name \(.*\))$/\1/')
> -    else if [[ "$1" =~ '(domid' ]]; then
> +    else if [[ "$1" =~ $domid_re ]]; then
>          id=$(echo $1 | sed -e 's/^.*(domid \(.*\))$/\1/')
> -    fi; fi; fi
> +    else if [[ "$1" =~ $state_re ]]; then
> +        state=$(echo $1 | sed -e 's/^.*(state \(.*\))$/\1/')
> +    fi; fi; fi; fi
>  
> -    [ -n "$name" -a -n "$id" ] && return 0 || return 1
> +    [ -n "$name" -a -n "$id" -a -n "$state" ] && return 0 || return 1
>  }
>  
>  is_running()
> @@ -228,7 +234,7 @@ is_running()
>               RC=0
>               ;;
>       esac
> -    done < <($CMD list -l | grep '(\(domain\|domid\|name\)')
> +    done < <($CMD list -l | grep '(\(domain\|domid\|name\|state\)')
>      return $RC
>  }
>  
> @@ -243,8 +249,6 @@ start()
>      if [ "$XENDOMAINS_RESTORE" = "true" ] &&
>         contains_something "$XENDOMAINS_SAVE"
>      then
> -     mkdir -p $(dirname "$LOCKFILE")
> -     touch $LOCKFILE
>       echo -n "Restoring Xen domains:"
>       saved_domains=`ls $XENDOMAINS_SAVE`
>          for dom in $XENDOMAINS_SAVE/*; do
> @@ -270,7 +274,6 @@ start()
>  
>      if contains_something "$XENDOMAINS_AUTO"
>      then
> -     touch $LOCKFILE
>       echo -n "Starting auto Xen domains:"
>       # We expect config scripts for auto starting domains to be in
>       # XENDOMAINS_AUTO - they could just be symlinks to files elsewhere
> @@ -299,6 +302,8 @@ start()
>           fi
>       done
>      fi
> +     mkdir -p $(dirname "$LOCKFILE")
> +     touch $LOCKFILE
>  }
>  
>  all_zombies()
> @@ -310,7 +315,7 @@ all_zombies()
>       if test "$state" != "-b---d" -a "$state" != "-----d"; then
>           return 1;
>       fi
> -    done < <($CMD list -l | grep '(\(domain\|domid\|name\)')
> +    done < <($CMD list -l | grep '(\(domain\|domid\|name\|state\)')
>      return 0
>  }
>  
> @@ -359,17 +364,15 @@ stop()
>       if test $id = 0; then continue; fi
>       echo -n " $name"
>       if test "$XENDOMAINS_AUTO_ONLY" = "true"; then
> -         eval "
> -         case \"\$name\" in
> -             ($NAMES)
> +         case "$name" in
> +             $NAMES)
>                   # nothing
>                   ;;
> -             (*)
> +             *)
>                   echo -e '(skip)'
>                   continue
>                   ;;
>           esac
> -         "
>       fi
>       # XENDOMAINS_SYSRQ chould be something like just "s" 
>       # or "s e i u" or even "s e s i u o"
> @@ -441,7 +444,7 @@ stop()
>           fi
>           kill $WDOG_PID >/dev/null 2>&1
>       fi
> -    done < <($CMD list -l | grep '(\(domain\|domid\|name\)')
> +    done < <($CMD list -l | grep '(\(domain\|domid\|name\|state\)')
>  
>      # NB. this shuts down ALL Xen domains (politely), not just the ones in
>      # AUTODIR/*
> @@ -478,7 +481,7 @@ check_domain_up()
>               return 0
>               ;;
>       esac
> -    done < <($CMD list -l | grep '(\(domain\|domid\|name\)')
> +    done < <($CMD list -l | grep '(\(domain\|domid\|name\|state\)')
>      return 1
>  }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>