[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