Re: [libvirt] [PATCH] output status information during guest shutdown again
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(a)cactusamerica.com