On Mon, 2014-11-03 at 08:50 -0500, Anne Mulhern wrote:
----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
> To: anaconda-patches(a)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(a)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