tuxera / reliance-edge

Transactional power-failsafe filesystem for microcontrollers
https://www.tuxera.com/company/open-source
GNU General Public License v2.0
109 stars 32 forks source link

Small changes proposed to enhance the code. #8

Closed jcdubois closed 7 years ago

jcdubois commented 7 years ago

FSE code doesn't handle path separators the same way as POSIX code. Proposed change to fix it.

osbdev for linux makes comparisons between signed and unsigned ints. This triggers warnings with -Wextra compile option. Proposed change to fix it.

jcdubois commented 7 years ago

I added some precautions around the strncpy usage to make sure the strings are 0 terminated.

aamcampbell commented 7 years ago

We appreciate the corrections you have made, except that I have a couple of comments on the strncopy usage.

First of all, the added precautions are unnecessary because the existing strcpy() functions are copying either from an array of HOST_PATH_MAX characters or from a string that has already been checked to make sure it isn't longer than HOST_PATH_MAX. However, I think double-checking is still appropriate in IbSetRelativePath, since the array is passed in as an argument, without documenting the length restriction. Otherwise, we would prefer to avoid unnecessary "dead" code.

Secondly, the new code is set up to truncate any paths that are too long (by adding '\0' at the end) without reporting any error. We would prefer the code to immediately report an error and abort when such a condition is detected.

aamcampbell commented 7 years ago

As I reviewed the code, I realized that my comments about the precautions did not quite make sense, since the code had already been using strncpy incorrectly in several locations, and appending a '\0' did make sense. However, I chose to explicitly check the length prior to copying the string (where needed) instead of using strncpy. I merged in the request after making that change.