search

truenas / py-SMART

Wrapper for smartctl (smartmontools)
GNU Lesser General Public License v2.1
76 stars 35 forks source link

Device.__getstate__() is broken #86

Closed f18m closed 5 months ago

f18m commented 5 months ago

Describe the bug The method getstate() of Device class used to return a dictionary. Since this commit: https://github.com/truenas/py-SMART/commit/b9e0a6426bd5b0c09569141110456ff32e55adfb#diff-75bf1e4cf04c226a1b9986d88bd9d3882dbb1e096e70920a63e243f83a9d278cR476 the implementation just returns "None".

Was that just a typo?

thanks!

ralequi commented 5 months ago

I think I don't have "getstate" method in my tests, it may be a bug/typo. Let me check in deep

ralequi commented 5 months ago

Well, probably "return None" is the issue here. Introduced 10 months ago.

Trying to remember, I think I founded some issue mapping the data. Probably left the none by a mistake.

I'm going to have a look on how to rescue it. Just to have some background: How and for what are you using it? Do you use the all_info param?

f18m commented 5 months ago

Actually I'm just trying to get psmqtt project to work. It uses PySMART and at this line:

https://github.com/eschava/psmqtt/blob/master/src/handlers.py#L538

it's invoking the getstate() method to get a nice dictionary to perform a lookup using a key obtained from the configuration file. In practice in the config file of that utility you list whatever SMART attribute you're interested in...

f18m commented 5 months ago

and thanks for the fast answer!

ralequi commented 5 months ago

Please, check when you are able. It should returns a valid dict now.

You may be able to install the dev version from git or from pip ( https://pypi.org/project/pySMART/1.3.1.dev8/ )

If you can provide me more info about something you are missing in that representation I may be able to provide a better output. The more info the better.

f18m commented 5 months ago

thanks @ralequi for the very fast fix! I tested it and it works fine. I just wonder: instead of using the 'magic' _ _getstate __() function (apparently magic is their official name, see https://peps.python.org/pep-0008/#descriptive-naming-styles), what about exposing these info also through a function like 'all_attributes_as_dict()' or 'get_state_dict()' or similar? I think is not a good practice to invoke magicfunctions just to inspect a Python class...

thanks!

ralequi commented 5 months ago

Hi,

This is the reason behind getstate : https://docs.python.org/3/library/pickle.html#object.__getstate__ : to serialize the class under certain generic libs.

The dict function you are suggesting is in fact defined by default. And do you know which name is? Yeah, exactly: https://docs.python.org/3/library/stdtypes.html#object.__dict__

I don't like that magics functions, but while there are people using it, I can't delete them.

Thanks for your report, I'll try to push it to an stable release soon