Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SMART status to disk infos #2046

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

christophehenry
Copy link
Contributor

Rel: YunoHost/issues#1823

So this one may be a bit controversial since it rewrite the D-Bus part using sdbus-python which is not yet bundled in Debian. I think, however using sdbus-python will have a positive effect on code since it D-Bus proxies are plain Python classes which improves code expressiveness and API discovery.

raise NotImplementedError


class AtaDisk(Disk, AtaController): ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
class AtaDisk(Disk, AtaController): ...


class NvmeDisk(Disk, NVMeController): ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@christophehenry christophehenry force-pushed the storage-api-smartctl branch 3 times, most recently from 8ee3ff7 to 573daea Compare February 25, 2025 16:05
flags=DbusUnprivilegedFlag,
)
def can_format(self, type: str) -> Tuple[bool, str]:
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried to delete all NotImplemented methods ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those interfaces a automatically generated by the CLI as mentioned at the beginning of the file. The implementation is provided by @dbus_method. The NotImplementerError is just here to have a method content. It could be pass but that's what the CLI generates so I kept it.

@zamentur
Copy link
Member

zamentur commented Mar 22, 2025

Thanks for this contrib, i have not tested it yet.

Could you explain advantages to use python-sdbus ? Maybe we could try to make pro and cons for the 2 solutions ? We can indeed integrate sdbus in yunohost repo, but it's a things to maintain in more... Is it really needed ?

I think it could be nice to create a diagnosis test about smartctl using this...

@christophehenry
Copy link
Contributor Author

christophehenry commented Mar 22, 2025

With sdbus, properties and methods are defined by the generated interfaces. This makes the infos as easy to retrieve as accessing a class property whereas with the dbus-python library you have to unmarshall several layers of dictionary. It makes the code way cleaner and I think it's necessary as I plan to add more features using Udisks2 like formatting, retrieving detailed SMART test results and start new SMAT tests. Now the methods are already defined.

Of course, adding python-sdbus is more work but it's already packaged for Debian Trixie and in Bookworm packports.

@christophehenry christophehenry force-pushed the storage-api-smartctl branch 2 times, most recently from 519a2a5 to a55b895 Compare March 26, 2025 14:08
@christophehenry christophehenry force-pushed the storage-api-smartctl branch 2 times, most recently from 901ce85 to 67c7070 Compare April 5, 2025 11:14
- Also rewrite API using sdbus-python
UNKNOWN = enum.auto()

@staticmethod
def parse(drive: dict) -> "DiskState":

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants