FS::read returning negative number on type size_t

[fcapano]

in FS.cpp, function File::read should return an unsigned int (aka size_t), however it returns a negative value (-1).

Arduino/cores/esp8266/FS.cpp

Line 78 in 98125f8

return -1;

@igrr

[themindfactory]

Seems OK to me, it denotes an error condition as there is no file pointer defined, this would only occur if there is no open file and you tried to read.

…

On Wed, Jan 6, 2021 at 1:42 PM Francesco Capano ***@***.***> wrote: in FS.cpp, function File::read should return an unsigned int (aka size_t), however it returns a negative value (-1). https://github.com/esp8266/Arduino/blob/98125f88605cd7e46e9be4e1b3ad0600dd5d2b51/cores/esp8266/FS.cpp#L78 @igrr <https://github.com/igrr> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7814>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBV22KAT25QJNKMFRMSNT3SYSVKRANCNFSM4VX5MJZQ> .

[TD-er]

@themindfactory
I guess @fcapano tries to make a point -1 is not of the same type as size_t.
This means you may need to perform a bit more complex (or at least less intuitive) test to check the return value.

So you have 3 different return value states to check:

• return value < size: Not all expected data was read.
• return value == size: OK situation.
• return value > size: The error value (-1)

[themindfactory]

True.. :-)

…

On Fri, Jan 8, 2021, 05:17 TD-er, ***@***.***> wrote: @themindfactory <https://github.com/themindfactory> I guess @fcapano <https://github.com/fcapano> tries to make a point -1 is not of the same type as size_t. This means you may need to perform a bit more complex (or at least less intuitive) test to check the return value. So you have 3 different return value states to check: - return value < size: Not all expected data was read. - return value == size: OK situation. - return value > size: The error value (-1) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7814 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBV22OAZVFVFYXLBPRN5GDSY3LSVANCNFSM4VX5MJZQ> .

[fcapano]

It's not a given the caller would check if the returned value is negative, given that it's returning an unsigned number.
Wouldn't it be more appropriate to return zero?

Just to drive the point further, this little program compiled with gcc will return 0:

#include <stddef.h>
size_t x(){ return -1; }

int main() {
    if (x()<0) return 1;
    return 0;
}

[earlephilhower]

This is a case that's probably not hit ever since it only happens when you have a naked File and not one returned by a {spiffs,sd,littlefs}.open() call, but you bring up a good point.

Returning MAXINT on a failed read is the wrong thing here IMO. Either the File::read type return type should have been ssize_t (i.e. signed size, just like POSIX where -1 indicates an error), or a 0 should be returned in this case.

I think I would just return 0 in this case, like in the File::write() calls.

[themindfactory]

So now zero means error and a naked file? I think there was a reason POSIX decided on ssize_t (-1).

…

On Fri, Jan 8, 2021 at 11:26 AM Earle F. Philhower, III < ***@***.***> wrote: This is a case that's probably not hit ever since it only happens when you have a naked File and not one returned by a {spiffs,sd,littlefs}.open() call, but you bring up a good point. Returning MAXINT on a failed read is the wrong thing here IMO. Either the File::read type return type should have been ssize_t (i.e. signed size, just like POSIX where -1 indicates an error), or a 0 should be returned in this case. I think I would just return 0 in this case, like in the File::write() calls. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7814 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBV22OP6XKDCZSTUYZRIELSY4W37ANCNFSM4VX5MJZQ> .

[earlephilhower]

0 means EOF for read which seems like a reasonable answer. We don't have errno and I'm loathe to change so many function signatures for such a rare, uncommon case.

Again, this only happens if you have a fundamental, static source code problem (File x; x.read();) and not in the case of SPIFFS/LittleFS/SD file-not-found or other cases. Those cases do have an implementation pointer and already return 0 when the open()ed file isn't valid.