On 02/20/2012 06:32 AM, Peter Krempa wrote:
This patch modifies the libvirt-guests init script to enable parallel shiutdown on guest machines and gets rid of error messages if the client
s/shiutdown/shutdown/
is unable connect to the URI specified in the config file. --- Simultaneous shutdown of machines may speed up the shutdown process as the shutdown sequence of guests often consists of timeouts and storage un-intensive tasks. Simultaneous resume of machines was already supported, although not documented well enough.
This patch also checks if connection to the URI can be done, and prints a error message if it's not the case. This get's rid of unrelevant and repeated error messages
s/unrelevant/irrelevant/ (Stupid English, for being inconsistent on whether un- or ir- negates a word :)
if the URI is unreachable.
The last improvement is while using managed-save. Transient domains are excluded from the save sequence to get rid of error messages and a list of domains that are left behind is printed.
Please tell me your suggestions how to improve this, as I'm not a bash "native speaker".
Actually, we want this to be stricter than bash, since people are using libvirt-guests on debian-based systems where /bin/sh is dash. But no fears - I'm fluent in shell.
+test_connect()
I find it helpful to document ALL shell functions; it makes maintenance easier down the road (it doesn't help that we haven't been doing this in the rest of the file). Here, it looks like you are doing: # test_connect URI # check if URI is reachable test_connect() {
+{ + uri=$1 + + run_virsh "$uri" connect 2>/dev/null
Any reason for the two spaces?
+ if [ $? -ne 0 ]; then + eval_gettext "Can't connect to \$uri. Skipping." + echo + return 1 + fi +} + +# "persistent" argument options: +# yes: list only persistent guests +# no: list only transient guests +# all: list both persistent and transient guests list_guests() {
Here, I would use: # list_guests URI PERSISTENT prior to the documentation of valid PERSISTENT values.
uri=$1 + persistent=$2
list=$(run_virsh_c "$uri" list) if [ $? -ne 0 ]; then @@ -89,12 +106,19 @@ list_guests() {
uuids= for id in $(echo "$list" | awk 'NR > 2 {print $1}'); do - uuid=$(run_virsh_c "$uri" dominfo "$id" | awk '/^UUID:/{print $2}')
<aside> Remind me again why we are doing an inefficient shell loop? In fact, why were we using 'dominfo | awk' to convert id to uuid? It's faster to use 'run_virsh_c "$uri" virsh domuuid $id" to get the same information. Taking it one step further, I'd rather see us enhance 'virsh list' to give us what we want up front. That is, I think it would be nice to have 'virsh list { [--table] | --uuid | --name | --id } ...', where --table is default (for back-compat), but listing --uuid, --name, or --id drops the header line, and lists just one identifier per line rather than a full table. Also, I'd also like to see 'virsh list { [--running] | --all | --inactive | --persistent | --transient } ...'; that is, we ought to have virsh list fine-tune which domains it lists, by adding new options such as --persistent and --transient. By improving virsh, we could skip out on the awk loop altogether, and have the initial virsh list give us what we want in the first place. </aside>
- if [ -z "$uuid" ]; then + dominfo=$(run_virsh_c "$uri" dominfo "$id") + uuid=$(echo "$dominfo" | awk '/^UUID:/{print $2}') + dompersist=$(echo "$dominfo" | awk '/^Persistent:/{print $2}')
This sets $dompersist to yes or no,...
+ + if [ -z "$uuid" ] || [ -z "$dompersist" ]; then RETVAL=1 return 1 fi - uuids="$uuids $uuid" + + if [ "$persistent" == "$dompersist" ] ||
For portability to dash, you must use '=', not '==', inside [].
+ [ "$persistent" == "all" ]; then + uuids="$uuids $uuid"
...if persistent is 'all', then you wasted an awk call above for a result you don't care about, since you plan on using the uuid anyway. Again, more reason to have virsh list give us what we want in the first place.
+shutdown_guest_async() +{ + uri=$1 + guest=$2 + + name=$(guest_name "$uri" "$guest") + label=$(eval_gettext "Starting shutdown on guest: \$name") + echo $label
Missing quotes around $label. Probably simpler to just: name=$(guest_name "$uri" "$guest") eval_gettext "Starting shutdown on guest: \$name" echo
+ retval run_virsh "$uri" shutdown "$guest" >/dev/null || return
No need for the ||return - since this is the last statement in the function, you will return anyway, with the same status.
+} + +set_add() +{ + item=$1 + items=$2 + + echo "$items $item"
Should you be checking for duplicates before adding into a set? I'm not sure without reviewing further to see how this is used. Also, this particular usage requires that the user capture the output of the shell function using $(), which wastes a subshell; there are tricks for passing the name of a variable which holds a set, where you can then modify the variable in place with fewer forks, but I don't think we're at the point where optimizing a few forks will help matters.
+} + +set_remove() +{ + item=$1 + items=$2 + + newitems= + for nit in $items; do + if [ "$nit" != "$domain" ]; then
Where did "$domain" come from? Don't you instead mean to be comparing against "$item"?
+ newitems="$newitems $nit"
The resulting $newitems will always have a leading space, even if $items on entry did not. Is that intentional?
+ fi + done + + echo "$newitems" +} + +set_count() +{ + items=$1 + + count="0" + for item in $items; do + count=$((count+1))
It's slightly more portable to do $(($count+1)). But even that is slow; why not let the shell do it for you: set_count() { set -- $1 echo $# }
+ done + + echo $count +} + +set_head() +{ + items=$1 + + for item in $items; do + echo $item + return 0 + done +}
I sure hope that the sets you are using have no whitespace or glob characters (that is, a set of domain ids or uuids is safe, a set of domain names or of URIs is not).
+ +remove_shutdown_domains() +{ + uri=$1 + domains=$2 + + newlist= + for dom in $domains; do + guest_is_on "$uri" "$dom" 2>&1 > /dev/null || return
Do you really want to break out of the entire for loop on the first guest where you encounter failure? Also, this redirection looks fishy - it says 'make the error output of guest_is_on go to my stdout, and discard the normal output of guest_is_on'. Did you mean '>/dev/null 2>&1' which says to discard both output and error messages from guest_is_on?
+ if "$guest_running"; then + newlist="$newlist $dom" + fi + done + + echo "$newlist" +}
I ran out of time to finish my review today, but hope this gives you some things to think about. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org