Fixed null ptr errors in print statements

[jayjaybillings]

I was compiling from source on Fedora 32 using GCC 10.1.1-1 and encountered numerous compilation errors. GCC 9 and 10 are rather picky about calling printf with arguments that can have NULL values.

To fix these errors I adjusted the vcos_logging header file and fixed two separate places in the file system and threading utilities.

Please let me know if I need to adjust anything. Happy to help get this in shape to push to master.

[6by9]

Thanks for the patch

Personally I hate if( const != variable), but also these can all be condensed down to if (var)

I'm also not quite clear what the logic is with the gcc change. printf("%s", NULL) always used to print "(null)", so throwing an error is really a backwards step. We're now not logging anything under these conditions, so perhaps they really ought to be eg

const char* null_str = "(null)";
DEBUG_MINOR( "vc_hostfs_totalspace for '%s' returning %" PRId64 "", path ? path : null_str, ret );

which is less readable, and you'll end up with loads of copies of a string "(null)" kicking around.

I wonder what others think.

[popcornmix]

I agree printing "(null)" is more useful for debugging that printing nothing so there is some cost to fixing the issues.

Do you have some examples of gcc error messages? I wonder if it's better (and possible) to handle them from caller code - I suspect if we have got to a point where we have a null pointer and are printing it out we may be potentially dereferencing it through means other than printf.

[tombsar]

Surely you should either make vcos_vlog_default_impl handle a NULL cat pointer gracefully, OR update the _VCOS_LOG_X macros to avoid ever passing one, but not both?

Note that vcos_vlog_default_impl can still crash after this patch due to the unconditional pointer dereference on line 301.

[6by9]

I'm expecting this should fix #631 which lists the error messages.

AFAIK vcos_vlog_default_impl will never be called with cat = NULL, but you can't tell the compiler that, and it complains. You could assert, but that only does anything on debug builds.

It makes no sense whatsoever as the code would segfault first at the deref of cat

         if (cat->flags.want_prefix) <<<<<<
            fprintf( log_fhandle, "%s: ", cat->name );

but that's not what the compiler is complaining of.
Sorry, my comment is twaddle. and the patch doesn't fix this.
cat->name could be null, and that is what the compiler is really complaining about, it doesn't do any checking of cat itself.

Line 301 is under a #idef ANDROID which is never defined on the Pi.

[tombsar]

AFAIK vcos_vlog_default_impl will never be called with cat = NULL

Right, so I don't think this patch actually helps the printf problem.

It looks to me as though the proper check would be if (cat->flags.want_prefix && cat->name). Hopefully that's straightforward enough for gcc to understand, and fix the over-zealous compile errors.

Line 301 is under a #idef ANDROID which is never defined on the Pi.

I saw that, but thought that since the code remains in the function, it should be kept up to date with its surroundings. Just curious, but is there any reason these parts of the code are kept in the repository if they are for sure never compiled?

[jayjaybillings]

I just pushed changes to fix the if statements. Now they check path and cat->name directly.

[jayjaybillings]

@tombsar I was just reading your comment more closely. FWIW I agree. I don't thinks this fixes the broader set of printf problems for GCC 10, but it at least gets it compiling on Fedora 32.