Attention is currently required from: fixeria, laforge.

dexter has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/41229?usp=email )

Change subject: pySim-shell: add a logger class to centralize logging ......................................................................

Patch Set 7:

(5 comments)

Patchset:

PS1:

Yes, I think using pythons logging framework is the better option here.

Done

Patchset:

PS6:

It would be nice to have a command line option `-v/--verbose` to increase logging verbosity of pySim […]

I have now added method to control the log level and the verbosity. In pySim-shell i have have now added a setable "verbose" that uses both methods in a simplified way. I also don't think that we need much here. In any case debugging will become a lot easier when we see the module name and the line number in the log.

File pySim/log.py:

https://gerrit.osmocom.org/c/pysim/+/41229/comment/307c21c4_a38fae9b?usp=ema... : PS6, Line 62: staticmethod

You can use the `@classmethod` instead, which allows to reference "this class". […]

I think I have tried that, but _log_callback is passed to PySimLogHandler and this class may have problems with the cls parameter. I think I have tried that. At least @staticmethod worked better for me.

File pySim/runtime.py:

https://gerrit.osmocom.org/c/pysim/+/41229/comment/55f0c876_d059ad11?usp=ema... : PS6, Line 48: self.log = PySimLogger.get("RuntimeState")

A more usual approach is to have a module-local logger: […]

I have now changed it to a module local logger. Makes sense. I found out that we can always get the module name from the logger (%(module)s). The parameter we give to getLogger may be just an arbitrary name (%(name)s).

So I think we should get the log using

log = PySimLogger.get("ABC")

"ABC" is then the log facility.

File tests/pySim-trace_test/pySim-trace_test_gsmtap.pcapng.ok:

https://gerrit.osmocom.org/c/pysim/+/41229/comment/1c43327f_c68d1adb?usp=ema... : PS6, Line 9: USIM: a0000000871002

is it intentional that we're loosing this output in the unit test? […]

This was intentional with the previous approach (even though it was not a good idea, but I thought that those messages were dispensable in the pySim-trace usecase).

Since the current approach should not alter the behavior of existing code by default we should not get the messages of course. However there still may be slight alterations. Insisting on not changing any output in the slightest way would require a lot of extra code in PySimLogger. At the moment we still lose the "warning: " prefix in one line. I think that this is acceptable.