pypxe / PyPXE

Pure Python PXE (DHCP-(Proxy)/TFTP/HTTP/NBD) Server
MIT License
547 stars 125 forks source link

logging + DHCP.validateReq() #64

Closed pbertera closed 9 years ago

pbertera commented 9 years ago

Sorry I messed up my git repo, here a new PR with both patches.

psychomario commented 9 years ago

The children logging levels are being set twice. First where the children are created (tftp_logger = sys_logger.getChild("TFTP") pypxe-server.py L123), and secondly inside the respective classes (if self.mode_debug: tftp.py L39). This only needs to be done in one of the two places although I don't mind which.

Also can you clean up the commits and resolve the conflicts so I can cleanly merge this?

Thanks

mmattioli commented 9 years ago

@pbertera you need to do a rebase otherwise this can't be merged cleanly. Rebase off of psychomario:development and recommit your work on the logger. There are a few minor tweaks I will point out momentarily so you don't have to go back and do them twice but a rebase is necessary not only for this PR to be merged cleanly but for future look back on development, there needs to be linear progression.

mmattioli commented 9 years ago

@psychomario good catch on the level setting. I think it should be set in the respective class rather than in pypxe-server.py upon creation because then if importing the respective class into a library it can't be properly set. I believe @pbertera 's thinking was that if someone imported this and wanted to use the class's logger then there should be a way to set that class's logger's debug mode. Changing the log level within the class (like in tftp.py L39) cleans it up and makes it work both ways.

pbertera commented 9 years ago

@mmattioli yes, my thought about setting log level was with "library usage" in mind. We can remove the service setLevel() from pypxe-server.py, but service logger should used only inside the service and not in pypxe-server.py

pbertera commented 9 years ago

@mmattioli I'm not a git expert, can you give me some hints about the rebase ? Thanks a lot

mmattioli commented 9 years ago

@pbertera no worries :) First, backup all of your work elsewhere just incase. This link should be able to explain it better than I can type it here. Basically, after you've added the remote, it should be something like:

git checkout development-logging
git rebase upstream/development

That will rebase your branch off of the development branch and then a git status should show you the changes that you've made and finally do a git push onto your feature branch (should be origin/development-logging). You may have to do a git push --force but I'm not 100% sure. If you keep this PR open you don't need to make a new one since your changes to the branch will be reflected here.

pbertera commented 9 years ago

@mmattioli Thanks for the tip. This last commit should be mergeable, I removed the sys selector and fixed the missing space. I also removed the debug setter from pypxe-server.py

mmattioli commented 9 years ago

@pbertera thanks! If you could just confirm that line note above I'll merge this. Thanks again.