lcdproc / lcdproc

Client/server suite for controlling a wide variety of LCD devices
http://lcdproc.org/
GNU General Public License v2.0
104 stars 86 forks source link

Add Disk ignore list, fix buffer overflow. #180

Closed vintagepc closed 2 years ago

vintagepc commented 2 years ago

Up to 10 mountpoints can be ignored, but this limit can be adjusted by changing the appropriate #define and recompiling.

vintagepc commented 2 years ago

Thanks, will review and make the requested changes shortly.

I did think about separate feature/bugfixes and I can definitely understand the fix-only sentiment in a stability perspective (e.g. minimally invasive changes to reduce risk of breakage) but in this case there isn't really a "lite" version of just the bugfix shy of the array resize in the other open PR;

If you still prefer I do so then I will split them out (I'm the guest here, after all) but TBH given the scope of the changes it does feel a little like formality for the sake of formality because the bulk of the changes are for the cleanup/bugfix.

haraldg commented 2 years ago

As far as "easier to review" is concerned, the big difference is the new variables, that you introduce for the new feature. That makes matching the old code with the new code quite a bit harder.

You are right, that the merging of the two loops as part of the crash fix is quite invasive by itself. But IMO this is just more reason to two the change in to steps, as each one is non-trivial. (And this is even more true, when you add the changes to the user documentation.)

As lcdproc itself is a rather small project, many things might seem like a formality. Still I think it is worth following them as long as they don't cause too much extra work.

vintagepc commented 2 years ago

Closing because it has been superseded by #182 and #181