[PATCH 2/2] Use one thread for payload setup.
Brian C. Lane
bcl at redhat.com
Fri Aug 22 22:19:32 UTC 2014
On Fri, Aug 22, 2014 at 12:25:38PM -0400, David Shea wrote:
This looks really nice, just a few comments below:
> + def addListener(self, event_id, func):
> + """Add a listener for an event.
> +
> + :param int event_id: The event to listen for, one of the EVENT_* constants
> + :param function func: An object to call when the event is reached
> + """
> +
> + # Check that the event_id is valid
> + assert isinstance(event_id, int)
> + assert event_id <= self.STATE_FINISHED
> + assert event_id >= self.STATE_ERROR
> +
> + # Add the listener inside the lock in case we need to run immediately,
> + # to make sure the listener isn't triggered twice
> + with self._event_lock:
> + self._event_listeners[event_id].append(func)
> +
> + # If an error event was requested, run it if currently in an error state
> + if event_id == self.STATE_ERROR:
> + if event_id == self._thread_state:
> + func()
> + # Otherwise, run if the requested event has already occurred
> + elif event_id <= self._thread_state:
> + func()
I think you can drop the first block of this if, since the elif is <=
it's going to run when the current state == event_id
> + def restartThread(self, storage, ksdata, payload, fallback=False, checkmount=True):
> + """Start or restart the payload thread.
> +
> + If a payload thread is already running, this method will block
> + until the previous thread finishes. This method doesn't lock on
> + anything, so don't try to start two at once.
> +
> + :param blivet.Blivet storage: The blivet storage instance
> + :param kickstart.AnacondaKSHandler ksdata: The kickstart data instance
> + :param packaging.Payload payload: The payload instance
> + :param installclass.BaseInstallClass instClass: The install class instance
> + """
You already commented on IRC that you noticed these didn't match the
code.
> def _initialize(self):
> - hubQ.send_message(self.__class__.__name__, _("Downloading package metadata..."))
> -
> threadMgr.wait(constants.THREAD_PAYLOAD)
>
> - hubQ.send_message(self.__class__.__name__, _("Downloading group metadata..."))
> -
> self.environment = self.data.packages.environment
Is this still needed here? _payload_finished also does this.
> + def _payload_error(self):
> + self._error = True
> + hubQ.send_message(self.__class__.__name__, payloadMgr.error)
> + gtk_call_once(self.set_warning, _("Failed to set up installation source; check the repo url"))
> + hubQ.send_ready(self.__class__.__name__, False)
This should probably keep the check for proxy and error string
mentioning it.
> def _initialize(self):
> """ Private initialize. """
> - threadMgr.wait(THREAD_STORAGE)
> threadMgr.wait(THREAD_PAYLOAD)
Why drop the wait on storage here? I don't have a specific reason to
keep it, just wondering why.
--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
More information about the anaconda-patches
mailing list