[PATCH 2/2] Use one thread for payload setup.

David Shea dshea at redhat.com
Mon Aug 25 13:34:17 UTC 2014


On 08/22/2014 06:19 PM, Brian C. Lane wrote:
> 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

The first block is there because STATE_ERROR is -1, and if you add a 
error handler you don't want it running if the thread is in a normal, 
positive number state. I don't know that there's any way around handling 
the error state specially. If it were a big number instead of a small 
one there would need to be a check that installing handlers for the 
non-error states aren't triggered by a payload thread in an error state.

>
>> +    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.

Yeah. instClass doesn't belong in the rhel7 version, and I've copied the 
fallback and checkmount comments line into the rhel7 patch.

>
>>       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.

It is not.

>
>> +    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.

Oops.
>
>>       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.
>

I don't think it was ever needed, actually. Waiting on THREAD_STORAGE 
was always part of THREAD_PAYLOAD.


More information about the anaconda-patches mailing list