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=email :
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=email :
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=email :
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.



--
To view, visit https://gerrit.osmocom.org/c/pysim/+/41229?usp=email
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I187f117e7e1ccdb2a85dfdfb18e84bd7561704eb
Gerrit-Change-Number: 41229
Gerrit-PatchSet: 7
Gerrit-Owner: dexter <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <[email protected]>
Gerrit-CC: laforge <[email protected]>
Gerrit-Attention: laforge <[email protected]>
Gerrit-Attention: fixeria <[email protected]>
Gerrit-Comment-Date: Fri, 24 Oct 2025 10:36:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <[email protected]>
Comment-In-Reply-To: fixeria <[email protected]>
Comment-In-Reply-To: dexter <[email protected]>

Reply via email to