Nir Soffer has posted comments on this change. Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter ......................................................................
Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/48542/1/tests/utilsTests.py File tests/utilsTests.py: Line 1084: # various codings. Line 1085: invalid = chr(254) + chr(0) Line 1086: u_invalid = unichr(253) + unichr(254) Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום', e4='你好', Line 1088: e5=invalid, e6=u_invalid) > Who knows where unicode parameters come from... In this particular case it' VmId is a uuid, cannot contain unicode. We should validate input, not assume no control in the entire application. Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra) Line 1090: log.debug("Dobrý večer") Line 1091: log.debug(u"Dobrý večer") Line 1092: log.debug(invalid) Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום', e4='你好', Line 1088: e5=invalid, e6=u_invalid) Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra) Line 1090: log.debug("Dobrý večer") Line 1091: log.debug(u"Dobrý večer") > Complete control? In theory, yes. But how do you ensure it? If any single n We control whats in the source, for input, we should encode/decode values as needed. There are 2 options with unicode support: - You work internally with plain strings - any unicode input is encode to utf8 - You work internally with unicode - any non-unicode input is decoded to unicode, and any output is encoded. Since we don't need to work with unicode internally, there is no need to do this, and we should go with the first option. Line 1092: log.debug(invalid) -- To view, visit https://gerrit.ovirt.org/48542 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
