On 09/12/2013 10:19 AM, KOSAKI Motohiro wrote:
> (9/11/13 8:33 PM), David Rientjes wrote:
>> 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.
>
> Agreed. Even though we don't change mpol_to_str() interface, please just
> add BUG_ON into shmem_show_mpol(). It is much simpler than current
> proposal.
>
Hmm... that is simpler and clearer for writers, but may not for readers.
> At least, currently mpol_to_str() already have following assertion. I mean,
> the code assume every developer know maximum length of mempolicy. I have no
> seen any reason to bring addional complication to shmem area.
>
>
> /*
> * Sanity check: room for longest mode, flag and some nodes
> */
> VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>
> Thanks.
>
Hmm... *BUG_ON() is for protecting the OS continue blindly, can we be
sure: "in current condition, OS is continuing blindly?"
If an extern function's parameter is invalid, it mainly means caller
incorrectly use the function (which need return -EINVAL), not means "the
OS is continuing blindly".
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>