[PATCH] Add timeout to callbacks waiting for enough entropy (#1073679)
Vratislav Podzimek
vpodzime at redhat.com
Mon Nov 3 14:14:57 UTC 2014
On Mon, 2014-11-03 at 08:50 -0500, Anne Mulhern wrote:
>
>
>
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: anaconda-patches at lists.fedorahosted.org
> > Sent: Monday, November 3, 2014 7:04:34 AM
> > Subject: [PATCH] Add timeout to callbacks waiting for enough entropy (#1073679)
> >
> > On some systems there might never be enough entropy so instead of making the
> > installation stuck forever we should just continue after some time.
> >
> > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > ---
> > pyanaconda/constants.py | 3 +++
> > pyanaconda/ui/gui/spokes/lib/entropy_dialog.glade | 25 +++++++++++++++---
> > pyanaconda/ui/gui/spokes/lib/entropy_dialog.py | 25 +++++++++++++++---
> > pyanaconda/ui/lib/entropy.py | 32
> > +++++++++++++++++------
> > 4 files changed, 71 insertions(+), 14 deletions(-)
> >
> > diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> > index 45b2ca8..46c9e00 100644
> > --- a/pyanaconda/constants.py
> > +++ b/pyanaconda/constants.py
> > @@ -154,3 +154,6 @@ TAR_SUFFIX = (".tar", ".tbz", ".tgz", ".txz", ".tar.bz2",
> > "tar.gz", "tar.xz")
> > # screenshots
> > SCREENSHOTS_DIRECTORY = "/tmp/anaconda-screenshots"
> > SCREENSHOTS_TARGET_DIRECTORY = "/root/anaconda-screenshots"
> > +
> > +# for how long (in minutes) we try to wait for enough entropy for LUKS
> > +MAX_ENTROPY_WAIT = 10
> > diff --git a/pyanaconda/ui/gui/spokes/lib/entropy_dialog.glade
> > b/pyanaconda/ui/gui/spokes/lib/entropy_dialog.glade
> > index d0f7e41..3cb530e 100644
> > --- a/pyanaconda/ui/gui/spokes/lib/entropy_dialog.glade
> > +++ b/pyanaconda/ui/gui/spokes/lib/entropy_dialog.glade
> > @@ -24,7 +24,7 @@
> > <property name="margin_top">6</property>
> > <property name="margin_bottom">6</property>
> > <property name="orientation">vertical</property>
> > - <property name="spacing">6</property>
> > + <property name="spacing">3</property>
> > <child>
> > <object class="GtkLabel" id="messageLabel">
> > <property name="visible">True</property>
> > @@ -32,7 +32,7 @@
> > <property name="valign">center</property>
> > <property name="vexpand">True</property>
> > <property name="xalign">0</property>
> > - <property name="label" translatable="yes">The system needs
> > better quality of random data, you can improve it by typing randomly on
> > keyboard and moving your mouse</property>
> > + <property name="label" translatable="yes">The system needs
> > better quality of random data, you can improve it by typing randomly on
> > keyboard and moving your mouse. The installation will continue automatically
> > regardless of random data quality when time runs out.</property>
> > <property name="wrap">True</property>
> > <property name="width_chars">60</property>
> > <property name="max_width_chars">60</property>
> > @@ -44,6 +44,25 @@
> > </packing>
> > </child>
> > <child>
> > + <object class="GtkLabel" id="headerLabel">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + <property name="valign">center</property>
> > + <property name="vexpand">True</property>
> > + <property name="xalign">0</property>
> > + <property name="label" translatable="yes">Random data
> > quality:</property>
> > + <property name="wrap">True</property>
> > + <property name="width_chars">60</property>
> > + <property name="max_width_chars">60</property>
> > + <property name="margin_top">3</property>
> > + </object>
> > + <packing>
> > + <property name="expand">False</property>
> > + <property name="fill">True</property>
> > + <property name="position">1</property>
> > + </packing>
> > + </child>
> > + <child>
> > <object class="GtkProgressBar" id="progressBar">
> > <property name="visible">True</property>
> > <property name="can_focus">False</property>
> > @@ -52,7 +71,7 @@
> > <packing>
> > <property name="expand">False</property>
> > <property name="fill">True</property>
> > - <property name="position">1</property>
> > + <property name="position">2</property>
> > </packing>
> > </child>
> > </object>
> > diff --git a/pyanaconda/ui/gui/spokes/lib/entropy_dialog.py
> > b/pyanaconda/ui/gui/spokes/lib/entropy_dialog.py
> > index d04b08d..74eb94a 100644
> > --- a/pyanaconda/ui/gui/spokes/lib/entropy_dialog.py
> > +++ b/pyanaconda/ui/gui/spokes/lib/entropy_dialog.py
> > @@ -23,6 +23,8 @@ import time
> >
> > from gi.repository import Gtk, GLib
> >
> > +from pyanaconda.i18n import P_
> > +from pyanaconda.constants import MAX_ENTROPY_WAIT
> > from pyanaconda.ui.gui import GUIObject
> > from pyanaconda.ui.gui.utils import gtk_action_wait
> > from blivet.util import get_current_entropy
> > @@ -36,6 +38,8 @@ def run_entropy_dialog(ksdata, desired_entropy):
> > dialog = EntropyDialog(ksdata, desired_entropy)
> > dialog.run()
> >
> > + return dialog.force_cont
> >
>
> The method docstring needs to be updated to include
> information about return value and type.
It is a (tiny) function and not its docstring doesn't need an update.
>
> > class EntropyDialog(GUIObject):
> > builderObjects = ["entropyDialog"]
> > mainWidgetName = "entropyDialog"
> > @@ -46,9 +50,16 @@ class EntropyDialog(GUIObject):
> > self._desired_entropy = desired_entropy
> > self._progress_bar = self.builder.get_object("progressBar")
> > self._terminate = False
> > + self._started = 0
> > + self.force_cont = False
> >
> > def run(self):
> > self.window.show_all()
> > +
> > + # XXX: Is it better to rely on Gtk running the self._update_progress
> > + # method every ~250msec or rely on NTP not changing system time
> > right
> > + # now and use time.time()?
> > + self._250msec_loops = 0
>
> This is an instance variable, so it should be set in the constructor.
>
> Also, to have a more useful name in case the Glib.timeout() value changes,
> it should be something like, self._num_timeouts.
>
> > GLib.timeout_add(250, self._update_progress)
>
> 250 should be turned into a named constant, say TIMEOUT.
We use 250 msec in many other places in Anaconda, but okay.
>
> > Gtk.main()
> > self.window.destroy()
> > @@ -61,13 +72,21 @@ class EntropyDialog(GUIObject):
> > # remove the method from idle queue
> > return False
> > else:
> > + self._250msec_loops += 1
> > current_entropy = get_current_entropy()
> > current_fraction = min(float(current_entropy) /
> > self._desired_entropy, 1.0)
> > + remaining = MAX_ENTROPY_WAIT - int(self._250msec_loops / (4 *
> > 60))
>
> To make this robust to any change in that 250 value,
> which should be a named constant, it would be better to
> calculate remaining as
>
> remaining = MAX_ENTROPY_WAIT * 60 - self._num_timeouts * (TIMEOUT / 1000.0)
>
> instead of having precomputed that 4 which is implicitly based on the TIMEOUT value,
>
> and then divide remaining by 60 or 60.0 depending on
> interest in displaying fractional versus whole quantities of minutes.
>
> > +
> > self._progress_bar.set_fraction(current_fraction)
> > + self._progress_bar.set_text("%(pct)d %% (%(rem)d %(min)s
> > remaining)" % {"pct": (int(current_fraction * 100)),
> > +
> > "rem":
> > remaining,
> > +
> > "min":
> > P_("minute", "minutes", remaining)})
> > +
> > + # if we have enough our time ran out, terminate the dialog, but
> > let
> > + # the progress_bar refresh in the main loop
> > + self._terminate = (current_entropy >= self._desired_entropy) or
> > (remaining <= 0)
> >
> > - # if we have enough, terminate the dialog, but let the
> > progress_bar
> > - # refresh in the main loop
> > - self._terminate = current_entropy >= self._desired_entropy
> > + self.force_cont = (remaining <= 0)
> >
> > # keep updating
> > return True
> > diff --git a/pyanaconda/ui/lib/entropy.py b/pyanaconda/ui/lib/entropy.py
> > index 1fe5634..65b5a24 100644
> > --- a/pyanaconda/ui/lib/entropy.py
> > +++ b/pyanaconda/ui/lib/entropy.py
> > @@ -30,10 +30,11 @@ import sys
> > import termios
> >
> > from pyanaconda.progress import progress_message
> > +from pyanaconda.constants import MAX_ENTROPY_WAIT
> > from pykickstart.constants import DISPLAY_MODE_GRAPHICAL
> > from blivet.util import get_current_entropy
> >
> > -from pyanaconda.i18n import _
> > +from pyanaconda.i18n import _, P_
> >
> > def wait_for_entropy(msg, desired_entropy, ksdata):
> > """
> > @@ -51,15 +52,17 @@ def wait_for_entropy(msg, desired_entropy, ksdata):
> > # in some cases
> > from pyanaconda.ui.gui.spokes.lib.entropy_dialog import
> > run_entropy_dialog
> > progress_message(_("The system needs more random data entropy"))
> > - run_entropy_dialog(ksdata, desired_entropy)
> > + return run_entropy_dialog(ksdata, desired_entropy)
> > else:
> > - _tui_wait(msg, desired_entropy)
> > + return _tui_wait(msg, desired_entropy)
>
> Method docstring should be updated to document return value and type.
Fixing locally.
>
> >
> > def _tui_wait(msg, desired_entropy):
> > """Tell user we are waiting for entropy"""
>
> Method docstring should be updated here too, for all params and return value.
I really think these are all pretty self-documenting because of their
names.
>
> >
> > print(msg)
> > print(_("Entropy can be increased by typing randomly on keyboard"))
> > + print(_("After %d minutes, the installation will continue regardless of
> > the "
> > + "amount of available entropy") % MAX_ENTROPY_WAIT)
> > fd = sys.stdin.fileno()
> > old = termios.tcgetattr(fd)
> > new = termios.tcgetattr(fd)
> > @@ -67,20 +70,31 @@ def _tui_wait(msg, desired_entropy):
> > new[6][termios.VMIN] = 1
> > termios.tcsetattr(fd, termios.TCSANOW, new)
> >
> > - # wait for the entropy to become high enough
> > + # wait for the entropy to become high enough or running out of time
>
> "or running out of time" better as "time has run out".
Fixing locally.
>
> > cur_entr = get_current_entropy()
> > - while cur_entr < desired_entropy:
> > - print(_("Available entropy: %(av_entr)s, Required entropy:
> > %(req_entr)s [%(pct)d %%]")
> > + secs = 0
>
> secs is completely redundant here. You might as well do the loop as:
It improves readability of the code (at least for me).
>
> remaining = MAX_ENTROPY_WAIT * 60
> while cur_entr < desired_entropy and remaining >= 0:
> # etc,
>
> # divide remaining by 60.0 so user can see partial minutes
> # or by 60 if that's not desired for print statement.
>
> # decrement remaining by 1 at bottom of loop
>
>
> > + while cur_entr < desired_entropy and secs <= MAX_ENTROPY_WAIT * 60:
> > + remaining = MAX_ENTROPY_WAIT - (secs / 60)
> > + print(_("Available entropy: %(av_entr)s, Required entropy:
> > %(req_entr)s [%(pct)d %%] (%(rem)d %(min)s remaining)")
> > % {"av_entr": cur_entr, "req_entr": desired_entropy,
> > - "pct": int((float(cur_entr) / desired_entropy) * 100)})
> > + "pct": int((float(cur_entr) / desired_entropy) * 100),
> > + "min": P_("minute", "minutes", remaining),
> > + "rem": remaining })
> > time.sleep(1)
> > cur_entr = get_current_entropy()
> > + secs += 1
> >
>
> In any case, it seems like it would be better to abstract this timeout
> activity, something vaguely like:
Do we have other places to use such function? It would at least need to
return something more than just True/False to be more generic and then
we would get to the same old discussion with None as an error indication
vs. value, etc., etc.
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
More information about the anaconda-patches
mailing list