Hi,
Alan wrote (02 Mar 2014 10:09:59 GMT) :
> - I can't remove something that I don't have examples how it looks
> like;
There's plenty of documentation about IPv6 available, not mentioning
the RFCs :)
> - the proposed fix looks too complicated to me and I have no easy way
> to test it.
Agreed. There's most likely a way simpler solution.
> OK. Created :
Great!
>> > + log_string = re.sub(r'[0-9a-fA-F]{4}(:[0-9a-fA-F]{4}){7}',
>> > + r'[IP6 REMOVED]',
>> > + log_string)
>>
>> Better make the regexp case insensitive, I think.
>>
> Why? The above regexp will match uppercase and lowercase.
/[a-fA-F]/ is the expanded version of /a-f/i, with the usual downcases
of expanding things that could be factored: longer, harder to read and
understand, more prone to typos. Also, I think it expresses the
meaning in a less clearer way. But that's certainly a bit subjective,
and I don't care that much.
>> Also, there's "IP6" written in a few places. Is this intentional?
>>
> Yes. What would you prefer?
Unless "IP6" has a well-defined meaning that I'm not aware of, let's
not invent a new name for it. If what you mean is "IPv6 addresses",
just write that :)
>> > - Whisperback should sanitize DMI
>> > (https://labs.riseup.net/code/issues/6797)
>>
>> I'm unsure about that one. The DMI string often contain useful info
>> (e.g. the version of the BIOS), so killing it entirely because it may
>> contain serial numbers does not look optimal to me.
>>
> I think we already have most of the information we want from dmidecode,
> which gives it more selectively. I fail to see how keep what could be
> useful to us while removing the serial number of the machine in this
> very vendor-specific string, so I preferred to be on the safe side.
Fair enough, assuming we already extract the Version field from the
dmidecode output (which I've not checked).
>> Also, I'm glad there is a test suite, but the way it works seems
>> strange to me: if I understand it correctly, it's testing that
>> "[$something REMOVED]" can be found after sanitizing, but it does not
>> test that the info that should be removed cannot be found anymore,
>> which is what we should really test, no?
> It would be better, yes. Want to create a ticket?
That's now #6806. I'd rather see it fixed as part of this pull
request, if we want to rely on this test suite to guarantee that the
code is working well, but I won't insist so I've refrained myself from
marking this ticket as blocking #6798.
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
