Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name)

Michael Kerrisk (man-pages)
2020-09-23T20:35:21Z

On 9/15/20 12:03 PM, Stefan Puiu wrote:
> Hi,
>
> On Fri, Sep 11, 2020 at 6:28 PM Alejandro Colomar
> <colomar.6.4.3@gmail.com> wrote:
>>
>> Hi Stefan,
>>
>> On 2020-09-11 16:35, Stefan Puiu wrote:
>> > Hi,
>> >
>> > On Fri, Sep 11, 2020 at 12:15 AM Alejandro Colomar
>> > <colomar.6.4.3@gmail.com> wrote:
>> >>
>> >> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>> >> ---
>> >> man3/getgrent_r.3 | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/man3/getgrent_r.3 b/man3/getgrent_r.3
>> >> index 81d81a851..76deec370 100644
>> >> --- a/man3/getgrent_r.3
>> >> +++ b/man3/getgrent_r.3
>> >> @@ -186,7 +186,7 @@ main(void)
>> >>
>> >> setgrent();
>> >> while (1) {
>> >> - i = getgrent_r(&grp, buf, BUFLEN, &grpp);
>> >> + i = getgrent_r(&grp, buf, sizeof(buf), &grpp);
>> >
>> > I'm worried that less attentive people might copy/paste parts of this
>> > in their code, where maybe buf is just a pointer, and expect it to
>> > work. Maybe leaving BUFLEN here is useful as a reminder that they need
>> > to change something to adapt the code?
>> >
>> > Just my 2 cents,
>> > Stefan.
>> >
>> That's a very good point.
>>
>> So we have 3 options and I will propose now a 4th one. Let's see all
>> of them and see which one is better for the man pages.
>>
>> 1.- Use the macro everywhere.
>>
>> pros:
>> - It is still valid when the buffer is a pointer and not an array.
>> cons:
>> - Hardcodes the initializer. If the array is later initialized with a
>> different value, it may produce a silent bug, or a compilation break.
>>
>> 2.- Use sizeof() everywhere, and the macro for the initializer.
>>
>> pros:
>> - It is valid as long as the buffer is an array.
>> cons:
>> - If the code gets into a function, and the buffer is then a pointer,
>> it will definitively produce a silent bug.
>>
>> 3.- Use sizeof() everywhere, and a magic number for the initializer.
>>
>> The same as 2.
>>
>> 4.- Use ARRAY_BYTES() macro
>>
>> pros:
>> - It is always safe and when code changes, it may break compilation, but
>> never a silent bug.
>> cons:
>> - Add a few lines of code. Maybe too much complexity for an example.
>> But I'd say that it is the only safe option, and in real code it
>> should probably be used more, so maybe it's good to show a good practice.
>
> If you ask me, I think examples should be simple and easy to
> understand, and easy to copy/paste in your code. I'd settle for easy
> enough, not perfect or completely foolproof. If you need to look up
> obscure gcc features to understand an example, that's not very
> helpful. So I'd be more inclined to prefer version 1 above. But let's
> see Michael's opinion on this.
>
> Just my 2c,

So, the fundamental problem is that C is nearly 50 years old.
It's a great high-level assembly language, but when it comes
to nuances like this it gets pretty painful. One can do macro
magic of the kind you suggest, but I agree with Stefan that it
gets confusing and distracting for the reader. I think I also
lean to solution 1. Yes, it's not perfect, but it's easy to
understand, and I don't think we can or should try and solve
the broken-ness of C in the manual pages.

Thanks,

Michael

--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/