Propagate errors correctly in ps_files_cleanup_dir()

[nielsdos]

In SessionHandler::gc, we use a virtual call to PS(default_mod)->s_gc to call the gc implementation. That return value is checked against FAILURE.
One of the call targets of PS(default_mod)->s_gc is ps_gc_files(). ps_gc_files() calls to ps_files_cleanup_dir(). The latter function has some error checks and outputs a notice if something goes wrong. In cases of errors, the function returns 0. This means that the check in SessionHandler::gc will misinterpret this as a success and report that 0 files have been successfully cleaned up. Fix it by returning -1 to indicate something did go wrong.

Found using an experimental static analyser I'm developing.

[devnexen]

Should not you change the return type as well ?

[nielsdos]

zend_result is not appropriate, because the function here returns a value >= 0 on success. The only two valid values for zend_result are SUCCESS and FAILURE.

[devnexen]

maybe just returning -1 might be more appropriate then, usually those constants are for zend_result. otherwise LGTM to me.

[nielsdos]

The result is eventually compared to FAILURE though. If FAILURE would ever change its value this could cause problems.

[Girgias]

The result is eventually compared to FAILURE though. If FAILURE would ever change its value this could cause problems.

I'd rather not have FAILURE be used somewhere which doesn't not explicitly return a zend_result, as if we ever want to change them, e.g. by aliasing them to false and type deffing zend_result to bool it's going to be a pain to figure out which places break.

[nielsdos]

Okay, I changed it. Thx

[devnexen]

Thank you.