Re: [tor-bugs] #7325 [pyobfsproxy]: Scrub IPs before displaying them in logs

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#7325: Scrub IPs before displaying them in logs
-------------------------+--------------------------------------------------
 Reporter:  sysrqb       |          Owner:  asn         
     Type:  defect       |         Status:  needs_review
 Priority:  normal       |      Milestone:              
Component:  pyobfsproxy  |        Version:              
 Keywords:               |         Parent:              
   Points:               |   Actualpoints:              
-------------------------+--------------------------------------------------

Comment(by gsathya):

 Replying to [comment:3 sysrqb]:

 Great! Thanks for making the changes :)

 > Replying to [comment:2 gsathya]:
 > >
 > > 2)
 > > {{{
 > > +    log.obfslogger = log.ObfsLogger(args)
 > > +
 > > }}}
 > > Generally good practice to avoid instantiating objects in other
 modules.
 >
 > Makes sense, but see next.
 >
 > > {{{
 > > +""" Global variable that will track our Obfslogger instance """
 > > +obfslogger = None
 > > +
 > > }}}
 > > {{{
 > > +def get_obfslogger():
 > > +    """ Return our instance of Obfslogger class """
 > > +
 > > +    assert(obfslogger)
 > > +    return obfslogger
 > > }}}
 > >
 > > Wouldn't be better to check if there is a obfslogger, and if not
 create a new instance instead of exiting out?
 >
 > Yes, but I was making the assumption that the logger was being
 instantiated in obfsproxy.py so, assuming normal operation, it should
 never be the case that obfslogger was not set.
 I completely agree that in normal operation, it should never be the case
 that obfslogger was not set. But I'm just wondering if it'd be better to
 make it modular if other modules wanted to access it in the future.
 > >
 > > These can be changed to -
 > > In bfsproxy.py -
 > > {{{
 > > obfslogger = log.get_logger(args.no_safe_logging)
 > > }}}
 > > In obfsproxy/common/log.py -
 > > {{{
 > > """ Global variable that will track our Obfslogger instance """
 > > OBFSLOGGER = None
 > > }}}
 > > {{{
 > > def get_obfslogger(no_safe_logging):
 > > """ Return Obfslogger class """
 > >   global OBFSLOGGER:
 > >   if not OBFSLOGGER: OBFSLOGGER = ObfsLogger(no_safe_logging)
 > >   return OBFSLOGGER
 > > }}}
 > > or even just a create_obfslogger(no_safe_logger) which doesn't return
 anything.
 > >
 >
 > I agree that this makes much more sense than what I had. I'm curious
 about why the obfslogger variable is all caps, is it so it's assumed to be
 a constant?

 Yep. From the PEP8 style guide -
 http://www.python.org/dev/peps/pep-0008/#constants

 The rest look good, thanks! I'm leaving the status as 'needs_review' for
 asn's comments.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7325#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs