[PATCH] tools: variables clean-up in libvirt-guests script

[Martin Kletzander]

On Sun, Mar 29, 2020 at 04:32:52AM +0530, Prathamesh Chavan wrote:
>Redeclared variables in script functions marked as local.
>Variables `guest_running` and `guests_shutting_down` in the
>functions 'guest_is_on` and `check_guests_shutdown` were
>untouched, as the functions returned values in these
>variables.
>
>Signed-off-by: Prathamesh Chavan <pc44800 at gmail.com>
>---
>I'm submitting this as my BiteSizedTask for GSoC'20
>I've cc'd Martin Kletzander, as he is mentor for this task,
>and to ensure that the needful was done by me.
>

Thanks for sending the patch.

>After running `make check`, I received  "PASS: 98 and
>SKIP: 7" as the result.
>Travis-ci build report can be found at [1].
>
>For my project, 'Introducing job control to the storage driver',
>right now I'm going through the `/src/qemu/THREADS.txt` as well
>as previous work done by Tucker DiNapoli during GSoC'14, as well
>as the proposal of Taowei Luo for GSoC'15. Their email threads
>are surely helping me get a better picture of the project.
>Apart from this, please let me know if there is anything else,
>which could help me with this project. Thansk!
>

Discussing the project would be a good start.  You're doing well that you're
reading what was done before, but also make sure to check out what is happening
lately.

I'm only quickly looking through the code below, I have nowhere to test it
currently and who knows when I even looked at some libvirt code lately =)

>[1]: https://travis-ci.org/github/pratham-pc/libvirt/builds/668209157
>
> tools/libvirt-guests.sh.in | 104 +++++++++++++++++++------------------
> 1 file changed, 54 insertions(+), 50 deletions(-)
>
>diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
>index a881f6266e..90a18fee49 100644
>--- a/tools/libvirt-guests.sh.in
>+++ b/tools/libvirt-guests.sh.in
>@@ -65,7 +65,7 @@ retval() {
> # If URI is "default" virsh is called without the "-c" argument
> # (using libvirt's default connection)
> run_virsh() {
>-    uri=$1
>+    local uri=$1
>     shift
>
>     if [ "x$uri" = xdefault ]; then
>@@ -86,7 +86,7 @@ run_virsh_c() {
> # check if URI is reachable
> test_connect()
> {
>-    uri=$1
>+    local uri=$1
>
>     if run_virsh "$uri" connect 2>/dev/null; then
>         return 0;
>@@ -103,8 +103,8 @@ test_connect()
> # --transient: list only transient guests
> # [none]: list both persistent and transient guests
> list_guests() {
>-    uri=$1
>-    persistent=$2
>+    local uri=$1
>+    local persistent=$2
>
>     list=$(run_virsh_c "$uri" list --uuid $persistent)

I don't think this variable is supposed to be global.

>     if [ $? -ne 0 ]; then
>@@ -118,8 +118,8 @@ list_guests() {
> # guest_name URI UUID
> # return name of guest UUID on URI
> guest_name() {
>-    uri=$1
>-    uuid=$2
>+    local uri=$1
>+    local uuid=$2
>
>     run_virsh "$uri" domname "$uuid" 2>/dev/null
> }
>@@ -128,8 +128,8 @@ guest_name() {
> # check if guest UUID on URI is running
> # Result is returned by variable "guest_running"
> guest_is_on() {
>-    uri=$1
>-    uuid=$2
>+    local uri=$1
>+    local uuid=$2
>
>     guest_running=false
>     id=$(run_virsh "$uri" domid "$uuid")

It seems you skipped some functions after the first empty line.

>@@ -161,13 +161,13 @@ start() {
>         return 0
>     fi
>
>-    isfirst=true
>-    bypass=
>-    sync_time=false
>+    local isfirst=true
>+    local bypass=
>+    local sync_time=false
>     test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache
>     test "x$SYNC_TIME" = x0 || sync_time=true
>     while read uri list; do
>-        configured=false
>+        local configured=false
>         set -f
>         for confuri in $URIS; do
>             set +f
>@@ -186,7 +186,7 @@ start() {
>
>         eval_gettext "Resuming guests on \$uri URI..."; echo
>         for guest in $list; do
>-            name=$(guest_name "$uri" "$guest")
>+            local name=$(guest_name "$uri" "$guest")

It would be nice to have the info about the variable being local in one place,
at the top of the function.  Most of us are used to our old C ways here, so
keeping this in a similar fashion could be nice.

I understand that this is not an interesting task and it is also one of the
easiest ones, but the idea for that was created when this actually solved a real
bug.  This will probably be a one-off since not much is happening in the script,
but we could have a calmer sleep if the variables are marked properly.

Have a nice day,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200329/900e634e/attachment-0001.sig>