Milan Zamazal 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) > VmId is a uuid, cannot contain unicode. Believe me or not, vm_id as passed to SimpleLogAdapter constructor in Vm.__init__ is a unicode instance (I didn't investigated why). It's not a problem per se, but it makes a unicode from the formatted string in SimpleLogAdapter.perform (thus the returned result is going to be unicode as well!) and when a non-ASCII message is logged, it crashes. No objections against validating input, it's definitely a right thing to do, but a few additional checks don't harm, especially at a place where we've already been hit. 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") > We control whats in the source, for input, we should encode/decode values a I agree, and the check in SimpleLogAdapter conforms to that rule now. 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