Create the DASD module #1507
Create the DASD module #1507
Conversation
poncovka
force-pushed
the
modularization-devel-dasd_module
branch
2 times, most recently
from
d8f21be
to
1c267b9
Compare
| @@ -32,3 +33,13 @@ class DasdModule(KickstartBaseModule): | |||
| def publish(self): | |||
| """Publish the module.""" | |||
| DBus.publish_object(DASD.object_path, DasdInterface(self)) | |||
|
|
|||
| def discover_with_task(self, device_number): | |||
There was a problem hiding this comment.
When I see it used now. What about naming via instead of with? I know this is late but maybe not too late?
There was a problem hiding this comment.
We have decided to use with.
|
|
||
| from pyanaconda.core.regexes import DASD_DEVICE_NUMBER | ||
| from pyanaconda.modules.common.task import Task | ||
| from pyanaconda.modules.common.errors.configuration import DiscoveryError |
There was a problem hiding this comment.
I think you should inherit this exceptions and make something like StorageDiscoveryError. All "main" modules should use their variant instead of the general one.
There was a problem hiding this comment.
The inheritance doesn't work for DBus errors, so we would have to map every exception to a new DBus error. Also, I don't see a point in this, because the storage module obviously returns only the storage related discovery errors.
| # Check the format of the given device name. | ||
| self._check_device_number(self._device_number) | ||
|
|
||
| # Get a full device number. |
There was a problem hiding this comment.
Remove this comment it is redundant.
| # Get a full device number. | ||
| full_device_number = self._get_full_device_number(self._device_number) | ||
|
|
||
| # Try to discover the device. |
|
|
||
| @staticmethod | ||
| def _check_device_number(device_number): | ||
| """Check the format of the given device name.""" |
There was a problem hiding this comment.
Why the name of the function is _check_device_number but the doc tells you that it is checked if format is correct? Please make comments and code consistent.
|
|
||
| if callback: | ||
| callback(task_proxy) | ||
|
|
There was a problem hiding this comment.
What processing do you mean? This is a synchronous call so I think it should be just blocked until task is finished. When you want to react on that than the asynchronous processing should be used instead. This way you basically doing asynchronous call from synchronous call.
Do you have some use-case for this because I can't think of any?
There was a problem hiding this comment.
This functionality is used here: https://github.com/rhinstaller/anaconda/pull/1507/files#diff-dc67ecb027c0c13b20bd1bd133953322R146
The callback is used to show progress of a task that is running synchronously. However, there might be a better solution.
There was a problem hiding this comment.
I didn't find a better solution.
Create a new storage module on s390x architectures for discovering and formatting DASD devices.
Create a new task for discovering DASD devices by their number.
The logic for discovering DASDs in UI can be removed. We can run the remote task instead.
Create a new task for formatting DASDs. The logic for searching DASDs that should be formatted will stay in UI for now.
The sync_run_task method will call the given callback every iteration before sleeping. The callback can be used to process the current progress of the task.
The logic for DASD formatting can be removed from the DasdFormatting class and replaced with running a remote task instead. This is a temporary solution, because most of the DasdFormatting class should be moved to the DASD module and UI should run the tasks directly.
poncovka
force-pushed
the
modularization-devel-dasd_module
branch
from
1c267b9
to
aa1610e
Compare
Created new tests for the DASD module and its tasks.
poncovka
force-pushed
the
modularization-devel-dasd_module
branch
from
aa1610e
to
13bf0c2
Compare
|
Updated. |
Create a new storage module on s390x architectures for discovering
and formatting DASD devices and use it in UI.