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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list