Message-ID: <de87f720-68fd-02ef-1ce4-aba7593dd84a@gmail.com>
Date:   Wed, 23 Sep 2020 22:35:15 +0200
From:   "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
To:     Stefan Puiu <stefan.puiu@...il.com>,
        Alejandro Colomar <colomar.6.4.3@...il.com>
Cc:     mtk.manpages@...il.com, lnx-man <linux-man@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size
 (instead of hardcoding macro name)

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@...il.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@...il.com> wrote:
>>  >>
>>  >> Signed-off-by: Alejandro Colomar <colomar.6.4.3@...il.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/

Powered by blists - more mailing lists