search

tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
957 stars 42 forks source link

Be robust towards input containing null bytes (regression from #98) #131

Closed hartwork closed 7 months ago

hartwork commented 7 months ago

Regression from #98, may need coordination with #115.

edgar-bonet commented 7 months ago

may need coordination with #115

It indeed does. Your fix seems perfect to me. Should I add this to refactor-input? Or maybe let you rebase this PR once #115 gets merged?

hartwork commented 7 months ago

It indeed does. Your fix seems perfect to me. Should I add this to refactor-input? Or maybe let you rebase this PR once #115 gets merged?

@edgar-bonet I can rebase it onto #115 after merge but if acceptible I would prefer the other way around: merging #131 here first and rebasing #115 onto this after. It would reduce the number of things in flight, get the fix out earlier, and I'm imagining the conflict resolution to be easy here, if not I'd definitely volunteer to do the resolution. It would help us separate bugfixes from the refactoring, which I consider a good thing if feasible.

edgar-bonet commented 7 months ago

It would reduce the number of things in flight

That's exactly why I would like to merge #115 first. It has been “in flight” for quite a while now. But if you insist in getting this first, OK, go ahead.

hartwork commented 7 months ago

@edgar-bonet you gave #129 to me where order was more important to me, I don't insist, let's do your #115 before my #131 here then, happy to rebase after, it's still a good deal for me :+1:

hartwork commented 7 months ago

@edgar-bonet rebased and ready for re-review now

hartwork commented 7 months ago

Is there a reason for doing this in handle_input_event rather than handle_input_data? I seems to me it belongs to the latter. For instance, handle_input_event has so far no notion of “valid delimiter”: as far as it is concerned, the buffer is just a chunk of arbitrary binary data. Making sense of those bytes is the job of handle_input_data.

@edgar-bonet handle_input_data does not have knowledge of how much data was just read — variable bytes_read — and so we would need to null-check the whole buffer every time rather than just the bytes read this time, correct? Also one could argue that handle_input_data should get safe to process data and that null bytes could be not considered safe. The last part is a question of view, the former is a question of performance, at least in part. What do you think?