[libvirt] [PATCH] output status information during guest shutdown again

[Gerd v. Egidy]

Hi Eric,

thanks for the in-depth review. Sorry for answering so late but I had some 
urgent stuff popping up here.

I'll send an updated version of my patch in the next mail.

> >  tools/libvirt-guests.service.in |  1 +
> >  2 files changed, 51 insertions(+), 15 deletions(-)
> >  mode change 100644 => 100755 tools/libvirt-guests.init.sh
> 
> The mode change is spurious, and should not be part of this patch.

This was accidental. Removed.
 
> > -    printf %s "$label"
> > +    printf '%s%-12s\n' "$label" "..."
> 
> Why are you printing trailing whitespace?  You are left-justifying the
> 3-byte ..., which means you now always have 9 bytes of trailing space
> Also, the "..." doesn't technically need quoting, although I guess it
> doesn't hurt.  Would it be worth folding the ... into the format string
> itself, as in:
> 
> printf '%s...\n' "$label"
>
> If we were using \r, then I would understand the trailing space as a
> means of blanking out longer text printed on a previous loop, but you
> are abandoning \r.

done as you suggested. It was a leftover from the previous code with \r.

> > +        slept=$(($slept + 1))
> > +        if [ $(($slept%5)) -eq 0 ]; then
> 
> Consistency argues that we should have spaces on both sides of '%'

done everywhere.
 
> > +            progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null
> > | \ +                    awk '/^Data processed:/{print $3, $4}')
> > +            if [ -n "$progress" ]; then
> > +                printf '%s%12s\n' "$label" "$progress"
> > +            else
> > +                printf '%s%-12s\n' "$label" "..."
> > +            fi
> > 
> >          fi
> 
> Because you now print only every five seconds, you have lost out on the
> final printing when progress is non-empty if that happens on 4 of the 5
> iterations.  I think you want the logic to look more like:
> 
> slept=$(($slept + 1))
> progress=$(...)
> if [ -n "$progress" ]; then
>   printf  ... label progress
> elif [ $(($slept % 5)) -eq 0 ]; then
>   printf ... label...
> fi

for the final printing I have the "done" printf a few lines below. Or did I 
misunderstand you there?

I think we should still print a line every 5 seconds even if we don't have any 
progress information to tell. The reason is that otherwise the user will miss 
what is going on when using systemd: a lot of stuff is happening in parallel at 
first, then only the suspend/shutdown of guests is left because it usually 
takes the longest time. The shutdown notice is lost between tons of other 
stuff.

> > +        format=$(eval_gettext "Waiting for guest %s to shut down, %4d
> > seconds left\n")
> Why the padding for the seconds left?  If timeout is 60, I think this
> looks awkward:
> 
> Waiting for guest foo to shut down,    60 seconds left

you are right. This padding stuff is a leftover from \r, there it was nice to 
have constant positions on the screen, here it doesn't matter. removed 
everywhere.

 
> > +                printf "$format" "$name"
> 
> This is new output that was not present beforehand.  Is the intent that
> when there is no timeout, you want to show that the process is still
> alive waiting for the guest?

exactly, see above.
 
> Overall, it looks like this patch is headed in the right direction.

very good.

> Did
> you also check that on F16, where we still used sysvinit, that the
> output there is still reasonable?

I see, F16 uses systemd but libvirt is still accessed via sysvinit. 

I just set up a box with F16 and compiled the current libvirt rpm from F17 
with my patch applied: it works, the shutdown messages are shown as soon as 
you switch off the quiet and rhgb kernel parameters. 

But they appear twice. I think this is a systemd issue because messages from 
other sysvinit-scripts appear twice on the console too.

Kind regards,

Gerd

-- 
Address (better: trap) for people I really don't want to get mail from:
jonas at cactusamerica.com