Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes <rientjes@google.com>]

On Tue, 10 Sep 2013, Chen Gang wrote:

> > Why?  It can just store the string into the buffer pointed to by the 
> > char *buffer and terminate it appropriately while taking care that it 
> > doesn't exceed maxlen.  Why does the caller need to know the number of 
> > bytes written?  If it really does, you could just do strlen(buffer).
> > 
> > If there's a real reason for it, then that's fine, I just think it can be 
> > made to always succeed and never return < 0.  (And why is nobody checking 
> > the return value today if it's so necessary?)
> > 
> 
> For common printing functions: sprintf(), snprintf(), scnprintf().
> 
> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
> 
> at least they can let caller easy use.
> 

Nobody needs mpol_to_str() to return the number of characters written, 
period.  It's one of the most trivial functions you're going to see in the 
mempolicy code, it takes a pointer to a buffer and it stores characters to 
it for display.  Nobody is going to use it for anything else.  Let's not 
overcomplicate this trivial function.

> > Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)  
> > If the struct mempolicy really has a bad mode, then just store "unknown" 
> > or store a 0.  If maxlen is insufficient for the longest possible string 
> > stored by mpol_to_str(), then it should be a compile-time error.
> > 
> > 
> 
> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
> static funciton (only used within a file).
> 
> For extern function, callee (inside) can not assume anything of caller
> (outside) beyond the interface. So if failure occurs, better to report
> to caller only, and let caller to check what to do next.
> 

Are you just preaching about the best practices of software engineering?  
mpol_to_str() should never fail at runtime, plain and simple.  If somebody 
introduces a new mode and doesn't update it to print correctly, let's not 
fail the read().  Let's just print "unknown".  And if someone passes too 
small of a buffer, break it at compile time so it gets noticed and fixed.

I guarantee you that any kernel developer who writes code to call 
mpol_to_str() will be happy it never fails at runtime.  Really.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>