[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