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@xxxxxxxxxxxxxxxxx
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list