From: Alejandro Colomar <colomar.6.4.3@gmail.com>
To: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>,
lnx-man <linux-man@vger.kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name)
Date: Fri, 11 Sep 2020 19:21:38 +0200 [thread overview]
Message-ID: <f8d22f30-a03d-34c6-1736-0a73a7a6cdc8@gmail.com> (raw)
In-Reply-To: <7dd2ab72-3ce7-1f50-229a-e663c3df2dcd@gmail.com>
On 2020-09-11 17:28, Alejandro Colomar 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.
>
>
> Here's my definition for ARRAY_BYTES(), which is makes use of
> must_be_array() similar to the kernel ARRAY_SIZE():
>
> 4.1-
>
> #define is_same_type(a, b) \
> __builtin_types_compatible_p(__typeof__(a), __typeof__(b))
> #define is_array(a) (!is_same_type((a), &(a)[0]))
> #define must_be__(e, ...) ( \
> 0 * (int)sizeof( \
> struct { \
> _Static_assert((e) __VA_OPT__(,) __VA_ARGS__); \
> char ISO_C_forbids_a_struct_with_no_members__; \
> } \
> ) \
> )
> #define must_be_array__(a) must_be__(is_array(a), "Not an array!")
> #define ARRAY_BYTES(arr) (sizeof(arr) + must_be_array__(arr))
>
>
> The macro makes use of quite a few GNU extensions, though, which might
> be too much to ask.
>
> Actually, I was also going to propose this macro for the kernel itself,
> to make it a bit safer.
>
> There's a much simpler version of ARRAY_BYTES(), which requires the
> macro to be defined in a header that is not a system header (to avoid
> silencing warnings), and also requires a recent version of the compiler
> to show a warning:
>
> 4.2-
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])
> #define ARRAY_BYTES(arr) (sizeof((arr)[0]) * ARRAY_SIZE(arr))
>
>
> What do you all think about the 5 different options? I don't know which
> one is better.
I'd say 4.2 is the best one for the man pages. Just 2 one-line macro
definitions, very good safety, and pretty clear code.
Your thoughts?
next prev parent reply other threads:[~2020-09-11 17:21 UTC|newest]
Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-10 21:13 [PATCH 00/24] Many patches Alejandro Colomar
2020-09-10 21:13 ` [PATCH 01/24] inet_net_pton.3: Use 'PRIx32' rather than "%x" when printing 'uint32_t' values Alejandro Colomar
2020-09-11 9:31 ` Michael Kerrisk (man-pages)
2020-09-11 9:39 ` Alejandro Colomar
2020-09-11 9:59 ` Michael Kerrisk (man-pages)
2020-09-12 21:07 ` David Laight
2020-09-10 21:13 ` [PATCH 02/24] endian.3: " Alejandro Colomar
2020-09-11 10:13 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 03/24] timerfd_create.2: Use 'PRIxN' macros when printing C99 fixed-width integer types Alejandro Colomar
2020-09-11 8:12 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 04/24] eventfd.2: " Alejandro Colomar
2020-09-11 8:13 ` Michael Kerrisk (man-pages)
2020-09-11 12:44 ` AW: " Walter Harms
2020-09-10 21:13 ` [PATCH 05/24] offsetof.3: Use "%zu" rather than "%zd" when printing 'size_t' values Alejandro Colomar
2020-09-11 8:06 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 06/24] timer_create.2: Cast to 'unsigned long' rathen than 'long' when printing with "%lx" Alejandro Colomar
2020-09-11 7:57 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 07/24] request_key.2: Cast to 'unsigned long' rather " Alejandro Colomar
2020-09-11 7:51 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 08/24] add_key.2: " Alejandro Colomar
2020-09-11 7:50 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 09/24] clock_getcpuclockid.3: Remove unneeded cast Alejandro Colomar
2020-09-11 7:48 ` Michael Kerrisk (man-pages)
2020-09-11 10:25 ` Alejandro Colomar
2020-09-11 11:05 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 10/24] ioctl_ns.2: " Alejandro Colomar
2020-09-11 7:24 ` Michael Kerrisk (man-pages)
2020-09-11 9:13 ` [PATCH v2 10/24] ioctl_ns.2: Cast to 'unsigned long' rather than 'long' when printing with "%lx" Alejandro Colomar
2020-09-11 9:18 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 11/24] stat.2: Remove unneeded cast Alejandro Colomar
2020-09-11 7:25 ` Michael Kerrisk (man-pages)
2020-09-11 9:16 ` [PATCH v2 11/24] stat.2: Cast to 'unsigned long' rather than 'long' when printing with "%lx" Alejandro Colomar
2020-09-11 9:19 ` Michael Kerrisk (man-pages)
2020-09-11 9:34 ` [PATCH v3 11/24] stat.2: wsfix Alejandro Colomar
2020-09-11 9:36 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name) Alejandro Colomar
2020-09-11 7:17 ` Michael Kerrisk (man-pages)
2020-09-11 12:50 ` AW: " Walter Harms
2020-09-11 19:17 ` Alejandro Colomar
2020-09-14 9:24 ` AW: " Walter Harms
2020-09-14 9:51 ` Alejandro Colomar
2020-09-11 14:35 ` Stefan Puiu
2020-09-11 15:28 ` Alejandro Colomar
2020-09-11 17:21 ` Alejandro Colomar [this message]
2020-09-15 10:03 ` Stefan Puiu
2020-09-23 20:35 ` Michael Kerrisk (man-pages)
2020-09-24 9:35 ` Alejandro Colomar
2020-09-24 10:04 ` Michael Kerrisk (man-pages)
2020-09-24 10:08 ` Alejandro Colomar
2020-09-24 11:38 ` Michael Kerrisk (man-pages)
2020-09-24 13:09 ` Alejandro Colomar
2020-09-29 13:38 ` Michael Kerrisk (man-pages)
2020-09-29 13:57 ` Alejandro Colomar
2020-09-10 21:13 ` [PATCH 13/24] getpwent_r.3: " Alejandro Colomar
2020-09-11 7:17 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 14/24] fread.3: Move ARRAY_SIZE logic into macro Alejandro Colomar
2020-09-11 8:04 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 15/24] unix.7: Use sizeof() to get buffer size (instead of hardcoding macro name) Alejandro Colomar
2020-09-11 8:05 ` Michael Kerrisk (man-pages)
2020-09-11 8:07 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 16/24] getpwent_r.3: Declare variables with different types in different lines Alejandro Colomar
2020-09-11 6:43 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 17/24] get_phys_pages.3: Write 'long' instead of 'long int' Alejandro Colomar
2020-09-11 6:43 ` Michael Kerrisk (man-pages)
2020-09-11 13:07 ` AW: " Walter Harms
2020-09-11 19:24 ` Alejandro Colomar
2020-09-10 21:13 ` [PATCH 18/24] core.5: Use adequate type Alejandro Colomar
2020-09-11 8:08 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 19/24] pthread_setname_np.3: ffix Alejandro Colomar
2020-09-11 7:58 ` Michael Kerrisk (man-pages)
2020-09-11 8:32 ` Alejandro Colomar
2020-09-11 11:12 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 20/24] loop.4: ffix Alejandro Colomar
2020-09-11 6:42 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 21/24] aio.7: Use perror() directly Alejandro Colomar
2020-09-11 6:42 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 22/24] membarrier.2: Note that glibc does not provide a wrapper Alejandro Colomar
2020-09-11 6:44 ` Michael Kerrisk (man-pages)
2020-09-11 9:33 ` Jakub Wilk
2020-09-11 9:35 ` Michael Kerrisk (man-pages)
2020-09-11 11:42 ` Alejandro Colomar
2020-09-11 12:58 ` AW: " Walter Harms
2020-09-21 14:36 ` G. Branden Robinson
2020-09-24 8:06 ` Michael Kerrisk (man-pages)
2020-09-27 5:46 ` G. Branden Robinson
2020-09-27 20:05 ` Alejandro Colomar
2020-09-28 12:52 ` G. Branden Robinson
2020-09-28 13:33 ` Alejandro Colomar
2020-09-28 13:48 ` G. Branden Robinson
2020-09-28 14:31 ` David Laight
2020-09-28 14:42 ` Alejandro Colomar
2020-09-29 12:07 ` Michael Kerrisk (man-pages)
2020-09-29 12:06 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 23/24] select_tut.2: Use MAX(a, b) from <sys/param.h> Alejandro Colomar
2020-09-11 7:54 ` Michael Kerrisk (man-pages)
2020-09-11 8:46 ` Alejandro Colomar
2020-09-11 10:03 ` Michael Kerrisk (man-pages)
2020-09-10 21:13 ` [PATCH 24/24] bpf.2: Add missing headers Alejandro Colomar
2020-09-11 9:12 ` Michael Kerrisk (man-pages)
2020-09-11 9:32 ` [PATCH 00/24] Many patches Michael Kerrisk (man-pages)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f8d22f30-a03d-34c6-1736-0a73a7a6cdc8@gmail.com \
--to=colomar.6.4.3@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-man@vger.kernel.org \
--cc=mtk.manpages@gmail.com \
--cc=stefan.puiu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).