[PATCH 1/9] Add a target to check example programs

[PATCH 1/9] Add a target to check example programs

[Stephen Kitt]

This is a first step to automating example program checks. It extracts
source snippets introduced by "Program source", assuming it's C, and
attempts to compile it.

For now, only man pages with a single "Program source" entry are
processed. The compiled code isn't linked to avoid having to handle
library requirements (e.g. -ldl).

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 Makefile | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Makefile b/Makefile
index 0da0328d8..3f597d7c3 100644
--- a/Makefile
+++ b/Makefile
@@ -266,6 +266,23 @@ check-groff-warnings:
 	done; \
 	rm -f "$$GROFF_LOG"
 
+# Check that man pages' examples programs (for now, only pages with a
+# single program) actually compile
+.PHONY: check-example-programs
+check-example-programs:
+	GCC_LOG="$$(mktemp --tmpdir manpages-checksXXXX)" || exit $$?; \
+	for manpage in $$(grep -rc 'Program source' man?/* | grep ':1$$' | cut -d: -f1); \
+	do \
+		(man -Tutf8 $$manpage | \
+			col -bx | \
+			sed -n '/Program source/,/^[[:space:]]\{0,3\}[^[:space:]]/p' | \
+			tail -n+2 | \
+			head -n-1 | \
+			gcc -xc -c -o/dev/null -) 2>| "$$GCC_LOG"; \
+		[ -s "$$GCC_LOG" ] && { echo "$$manpage: "; cat "$$GCC_LOG"; echo; }; \
+	done; \
+	rm -f "$$GCC_LOG"
+
 # someone might also want to look at /var/catman/cat2 or so ...
 # a problem is that the location of cat pages varies a lot
 
-- 
2.30.2

[PATCH 2/9] seccomp.2: Use syscall() in the example code

[Stephen Kitt]

Since seccomp() doesn't exist in glibc, avoid relying on it, and use
syscall() instead. This updates the example program to match the
documentation, which was updated in commit 5945cd7bd3c3 ("seccomp.2:
Use syscall(SYS_...); for system calls without a wrapper").

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man2/seccomp.2 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/man2/seccomp.2 b/man2/seccomp.2
index a3421871f..67928ed3f 100644
--- a/man2/seccomp.2
+++ b/man2/seccomp.2
@@ -1133,6 +1133,7 @@ cecilia
 #include <linux/filter.h>
 #include <linux/seccomp.h>
 #include <sys/prctl.h>
+#include <sys/syscall.h>
 
 #define X32_SYSCALL_BIT 0x40000000
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
@@ -1190,7 +1191,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)
         .filter = filter,
     };
 
-    if (seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog)) {
+    if (syscall(SYS_seccomp, SECCOMP_SET_MODE_FILTER, 0, &prog)) {
         perror("seccomp");
         return 1;
     }
-- 
2.30.2

Re: [PATCH 2/9] seccomp.2: Use syscall() in the example code

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> Since seccomp() doesn't exist in glibc, avoid relying on it, and use
> syscall() instead. This updates the example program to match the
> documentation, which was updated in commit 5945cd7bd3c3 ("seccomp.2:
> Use syscall(SYS_...); for system calls without a wrapper").
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>

Patch applied.

Thanks!

Alex

> ---
>   man2/seccomp.2 | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/man2/seccomp.2 b/man2/seccomp.2
> index a3421871f..67928ed3f 100644
> --- a/man2/seccomp.2
> +++ b/man2/seccomp.2
> @@ -1133,6 +1133,7 @@ cecilia
>   #include <linux/filter.h>
>   #include <linux/seccomp.h>
>   #include <sys/prctl.h>
> +#include <sys/syscall.h>
>   
>   #define X32_SYSCALL_BIT 0x40000000
>   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> @@ -1190,7 +1191,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)
>           .filter = filter,
>       };
>   
> -    if (seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog)) {
> +    if (syscall(SYS_seccomp, SECCOMP_SET_MODE_FILTER, 0, &prog)) {
>           perror("seccomp");
>           return 1;
>       }

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[PATCH 3/9] inet.3: Switch to _DEFAULT_SOURCE in the example

[Stephen Kitt]

_BSD_SOURCE has been obsolete for long enough that it seems reasonable
to update the example program to use _DEFAULT_SOURCE instead.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/inet.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/inet.3 b/man3/inet.3
index 657fe45e0..47d3ba160 100644
--- a/man3/inet.3
+++ b/man3/inet.3
@@ -305,7 +305,7 @@ Here are some example runs:
 .SS Program source
 \&
 .EX
-#define _BSD_SOURCE
+#define _DEFAULT_SOURCE
 #include <arpa/inet.h>
 #include <stdio.h>
 #include <stdlib.h>
-- 
2.30.2

Re: [PATCH 3/9] inet.3: Switch to _DEFAULT_SOURCE in the example

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> _BSD_SOURCE has been obsolete for long enough that it seems reasonable
> to update the example program to use _DEFAULT_SOURCE instead. >
> Signed-off-by: Stephen Kitt <steve@sk2.org>

Deprecated, yes; obsolete, almost but not yet.

glibc 2.17 is still supported in CentOS 7, IIRC, and _BSD_SOURCE was 
deprecated since glibc 2.20 (see feature_test_macros(7)).

However, since this is an example program, which should mainly teach how 
to write new code, I'm inclined to use the non-deprecated version.

So, patch applied.

Thanks,

Alex

> ---
>   man3/inet.3 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/man3/inet.3 b/man3/inet.3
> index 657fe45e0..47d3ba160 100644
> --- a/man3/inet.3
> +++ b/man3/inet.3
> @@ -305,7 +305,7 @@ Here are some example runs:
>   .SS Program source
>   \&
>   .EX
> -#define _BSD_SOURCE
> +#define _DEFAULT_SOURCE
>   #include <arpa/inet.h>
>   #include <stdio.h>
>   #include <stdlib.h>

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 3/9] inet.3: Switch to _DEFAULT_SOURCE in the example

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

Hi Alex,

On Sat, 8 Jan 2022 02:26:51 +0100, "Alejandro Colomar (man-pages)"
<alx.manpages@gmail.com> wrote:
> On 1/7/22 17:46, Stephen Kitt wrote:
> > _BSD_SOURCE has been obsolete for long enough that it seems reasonable
> > to update the example program to use _DEFAULT_SOURCE instead. >
> > Signed-off-by: Stephen Kitt <steve@sk2.org>  
> 
> Deprecated, yes; obsolete, almost but not yet.
> 
> glibc 2.17 is still supported in CentOS 7, IIRC, and _BSD_SOURCE was 
> deprecated since glibc 2.20 (see feature_test_macros(7)).
> 
> However, since this is an example program, which should mainly teach how 
> to write new code, I'm inclined to use the non-deprecated version.

My reasoning here is that, from a distribution perspective, man-pages’s main
branch describes the future. CentOS 7 carries glibc 2.17, but it also carries
an old version of man-pages, which (hopefully) accurately describes the
environment there. Many example programs in man pages don’t build on older
distributions ;-).

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/9] matherr.3: Exclude the example from analysis

[Stephen Kitt]

The example program is obsolete, as indicate in the text, and
shouldn't be analysed. Changing the introductory heading to "Obsolete
program source" excludes it from automated analysis and makes it
hopefully clearer to readers that it is obsolete.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/matherr.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/matherr.3 b/man3/matherr.3
index 64bfa482a..e9d77d02c 100644
--- a/man3/matherr.3
+++ b/man3/matherr.3
@@ -376,7 +376,7 @@ matherr SING exception in log() function
 x=12345.000000
 .EE
 .in
-.SS Program source
+.SS Obsolete program source
 \&
 .EX
 #define _SVID_SOURCE
-- 
2.30.2

Re: [PATCH 4/9] matherr.3: Exclude the example from analysis

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> The example program is obsolete, as indicate in the text, and
> shouldn't be analysed. Changing the introductory heading to "Obsolete
> program source" excludes it from automated analysis and makes it
> hopefully clearer to readers that it is obsolete.

In this case, the first paragraph of the DESCRIPTION already notes that 
the whole page is obsolete (and even removed from glibc).  On top of 
that, I plan to add the [[deprecated]] attribute to the function 
prototype in the SYNOPSIS (when Michael comes back).

Do you think it's still necessary to mark the example program obsolete?
For a human reader, I'd say no.
For the automated analysis, I'd first decide on which format we want to 
standardize to mark code begin and code end, and after that fix this if 
it needs to be fixed.

Thanks,

Alex

> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>   man3/matherr.3 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/man3/matherr.3 b/man3/matherr.3
> index 64bfa482a..e9d77d02c 100644
> --- a/man3/matherr.3
> +++ b/man3/matherr.3
> @@ -376,7 +376,7 @@ matherr SING exception in log() function
>   x=12345.000000
>   .EE
>   .in
> -.SS Program source
> +.SS Obsolete program source
>   \&
>   .EX
>   #define _SVID_SOURCE

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 4/9] matherr.3: Exclude the example from analysis

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

Hi Alex,

On Sat, 8 Jan 2022 02:31:42 +0100, "Alejandro Colomar (man-pages)"
<alx.manpages@gmail.com> wrote:
> On 1/7/22 17:46, Stephen Kitt wrote:
> > The example program is obsolete, as indicate in the text, and
> > shouldn't be analysed. Changing the introductory heading to "Obsolete
> > program source" excludes it from automated analysis and makes it
> > hopefully clearer to readers that it is obsolete.  
> 
> In this case, the first paragraph of the DESCRIPTION already notes that 
> the whole page is obsolete (and even removed from glibc).  On top of 
> that, I plan to add the [[deprecated]] attribute to the function 
> prototype in the SYNOPSIS (when Michael comes back).
> 
> Do you think it's still necessary to mark the example program obsolete?
> For a human reader, I'd say no.
> For the automated analysis, I'd first decide on which format we want to 
> standardize to mark code begin and code end, and after that fix this if 
> it needs to be fixed.

I agree, let’s wait.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/9] mq_notify.3: Add signal.h for SIGEV_THREAD

[Stephen Kitt]

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/mq_notify.3 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/man3/mq_notify.3 b/man3/mq_notify.3
index 2c35a347d..31ba6f7cb 100644
--- a/man3/mq_notify.3
+++ b/man3/mq_notify.3
@@ -224,6 +224,7 @@ queue and then terminates the process.
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <signal.h>
 
 #define handle_error(msg) \e
     do { perror(msg); exit(EXIT_FAILURE); } while (0)
-- 
2.30.2

Re: [PATCH 5/9] mq_notify.3: Add signal.h for SIGEV_THREAD

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>   man3/mq_notify.3 | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/man3/mq_notify.3 b/man3/mq_notify.3
> index 2c35a347d..31ba6f7cb 100644
> --- a/man3/mq_notify.3
> +++ b/man3/mq_notify.3
> @@ -224,6 +224,7 @@ queue and then terminates the process.
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <unistd.h>
> +#include <signal.h>
I didn't read through the page too much.  Is SIGEV_THREAD or similar 
flags something necessary for normal use of this function?  If so, you 
might also want to add that header to the synopsis.

As an example, mkfifoat(3) documents <fcntl.h> in the synopsis for AT_* 
constants.

Thanks,

Alex

>   
>   #define handle_error(msg) \e
>       do { perror(msg); exit(EXIT_FAILURE); } while (0)

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[PATCH 6/9] newlocale.3: Use LC_GLOBAL_LOCALE, not ..._HANDLE

[Stephen Kitt]

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/newlocale.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/newlocale.3 b/man3/newlocale.3
index b5960c4c4..dc9406ede 100644
--- a/man3/newlocale.3
+++ b/man3/newlocale.3
@@ -360,7 +360,7 @@ main(int argc, char *argv[])
 
     /* Free the locale object. */
 
-    uselocale(LC_GLOBAL_HANDLE);    /* So \(aqloc\(aq is no longer in use */
+    uselocale(LC_GLOBAL_LOCALE);    /* So \(aqloc\(aq is no longer in use */
     freelocale(loc);
 
     exit(EXIT_SUCCESS);
-- 
2.30.2

Re: [PATCH 6/9] newlocale.3: Use LC_GLOBAL_LOCALE, not ..._HANDLE

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>   man3/newlocale.3 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/man3/newlocale.3 b/man3/newlocale.3
> index b5960c4c4..dc9406ede 100644
> --- a/man3/newlocale.3
> +++ b/man3/newlocale.3
> @@ -360,7 +360,7 @@ main(int argc, char *argv[])
>   
>       /* Free the locale object. */
>   
> -    uselocale(LC_GLOBAL_HANDLE);    /* So \(aqloc\(aq is no longer in use */
> +    uselocale(LC_GLOBAL_LOCALE);    /* So \(aqloc\(aq is no longer in use */

Why?  Not saying it's wrong, but I'd like a bit more of an explanation, 
since I don't know what this does, and would like to be able to check 
the correctness by just looking at the patch, without having to read 
whole man pages. :)

What was wrong, and in what sense does this patch fix it?

Thanks,

Alex


>       freelocale(loc);
>   
>       exit(EXIT_SUCCESS);

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 6/9] newlocale.3: Use LC_GLOBAL_LOCALE, not ..._HANDLE

[Jakub Wilk]

* Alejandro Colomar <alx.manpages@gmail.com>, 2022-01-08, 02:41:
>>      /* Free the locale object. */
>>-    uselocale(LC_GLOBAL_HANDLE);    /* So \(aqloc\(aq is no longer in use */
>>+    uselocale(LC_GLOBAL_LOCALE);    /* So \(aqloc\(aq is no longer in use */
>
>Why?

$ gcc -Wall newlocale-example.c
newlocale-example.c: In function ‘main’:
newlocale-example.c:67:15: error: ‘LC_GLOBAL_HANDLE’ undeclared (first use in this function); did you mean ‘LC_GLOBAL_LOCALE’?

-- 
Jakub Wilk

Re: [PATCH 6/9] newlocale.3: Use LC_GLOBAL_LOCALE, not ..._HANDLE

[Alejandro Colomar (man-pages)]

Hi, Jakub!

On 1/8/22 10:13, Jakub Wilk wrote:
> * Alejandro Colomar <alx.manpages@gmail.com>, 2022-01-08, 02:41:
>>>      /* Free the locale object. */
>>> -    uselocale(LC_GLOBAL_HANDLE);    /* So \(aqloc\(aq is no longer 
>>> in use */
>>> +    uselocale(LC_GLOBAL_LOCALE);    /* So \(aqloc\(aq is no longer 
>>> in use */
>>
>> Why?
> 
> $ gcc -Wall newlocale-example.c
> newlocale-example.c: In function ‘main’:
> newlocale-example.c:67:15: error: ‘LC_GLOBAL_HANDLE’ undeclared (first 
> use in this function); did you mean ‘LC_GLOBAL_LOCALE’?
> 

That makes sense!  I applied the patch, and added that to the commit 
message.

Thank you both,

Alex

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[PATCH 7/9] pkeys.7: Update the example to match glibc

[Stephen Kitt]

glibc 2.27 introduced support for the pkeys functions, but the glibc
versions don't match those declared in the example. Update the example
to match glibc, and avoid declaring the functions if glibc is new
enough.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man7/pkeys.7 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/man7/pkeys.7 b/man7/pkeys.7
index 73ddcdc43..480ff21d4 100644
--- a/man7/pkeys.7
+++ b/man7/pkeys.7
@@ -186,8 +186,10 @@ Segmentation fault (core dumped)
 #include <unistd.h>
 #include <sys/syscall.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <sys/mman.h>
 
+#if __GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ < 27)
 static inline void
 wrpkru(unsigned int pkru)
 {
@@ -200,7 +202,7 @@ wrpkru(unsigned int pkru)
 }
 
 int
-pkey_set(int pkey, unsigned long rights, unsigned long flags)
+pkey_set(int pkey, unsigned long rights)
 {
     unsigned int pkru = (rights << (2 * pkey));
     return wrpkru(pkru);
@@ -214,9 +216,9 @@ pkey_mprotect(void *ptr, size_t size, unsigned long orig_prot,
 }
 
 int
-pkey_alloc(void)
+pkey_alloc(unsigned int flags, unsigned int rights)
 {
-    return syscall(SYS_pkey_alloc, 0, 0);
+    return syscall(SYS_pkey_alloc, flags, rights);
 }
 
 int
@@ -224,6 +226,7 @@ pkey_free(unsigned long pkey)
 {
     return syscall(SYS_pkey_free, pkey);
 }
+#endif
 
 #define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \e
                            } while (0)
@@ -252,7 +255,7 @@ main(void)
     /*
      * Allocate a protection key:
      */
-    pkey = pkey_alloc();
+    pkey = pkey_alloc(0, 0);
     if (pkey == \-1)
         errExit("pkey_alloc");
 
@@ -260,7 +263,7 @@ main(void)
      * Disable access to any memory with "pkey" set,
      * even though there is none right now.
      */
-    status = pkey_set(pkey, PKEY_DISABLE_ACCESS, 0);
+    status = pkey_set(pkey, PKEY_DISABLE_ACCESS);
     if (status)
         errExit("pkey_set");
 
-- 
2.30.2

Re: [PATCH 7/9] pkeys.7: Update the example to match glibc

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> glibc 2.27 introduced support for the pkeys functions, but the glibc
> versions don't match those declared in the example. Update the example
> to match glibc, and avoid declaring the functions if glibc is new
> enough. >
> Signed-off-by: Stephen Kitt <steve@sk2.org>


There are a few problems with the prototypes.


alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype wrpkru
alx@ady1:~/src/gnu/glibc$ grep -rn define.wrpkru
alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_set
60:int pkey_set (int __key, unsigned int __access_rights) __THROW;
alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_mprotect
72:int pkey_mprotect (void *__addr, size_t __len, int __prot, int 
__pkey) __THROW;
alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_alloc
56:int pkey_alloc (unsigned int __flags, unsigned int __access_rights) 
__THROW;
alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_free
68:int pkey_free (int __key) __THROW;


As you see above, I couldn't find wrpkru().  Are you sure it exists in 
glibc?

pkey_mprotect(3) uses 'int' instead of 'unsigned long'.  Would you mind 
fixind that one too?

pkey_set(3) uses 'unsigned int' instead of 'unsigned long'.  Please fix 
that one.

pkey_free(3) uses 'int' instead of 'unsigned long'.  Would you mind 
fixing that one too?

BTW, I need to modify grep_glibc_prototype() so that it always prints 
the file name, even if only one file is passed to grep (adding /dev/null 
to the file list).


A part from that, I prefer EXAMPLES to be as simple as possible, so I'd 
do 2 patches.  One to match the definitions to the glibc ones, and then 
one commit removing old code, assuming glibc is new enough.  Would you 
mind sending a subsequent patch to remove everything under #if ... #endif?


Thanks,

Alex

> ---
>   man7/pkeys.7 | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/man7/pkeys.7 b/man7/pkeys.7
> index 73ddcdc43..480ff21d4 100644
> --- a/man7/pkeys.7
> +++ b/man7/pkeys.7
> @@ -186,8 +186,10 @@ Segmentation fault (core dumped)
>   #include <unistd.h>
>   #include <sys/syscall.h>
>   #include <stdio.h>
> +#include <stdlib.h>
>   #include <sys/mman.h>
>   
> +#if __GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ < 27)
>   static inline void
>   wrpkru(unsigned int pkru)
>   {
> @@ -200,7 +202,7 @@ wrpkru(unsigned int pkru)
>   }
>   
>   int
> -pkey_set(int pkey, unsigned long rights, unsigned long flags)
> +pkey_set(int pkey, unsigned long rights)
>   {
>       unsigned int pkru = (rights << (2 * pkey));
>       return wrpkru(pkru);
> @@ -214,9 +216,9 @@ pkey_mprotect(void *ptr, size_t size, unsigned long orig_prot,
>   }
>   
>   int
> -pkey_alloc(void)
> +pkey_alloc(unsigned int flags, unsigned int rights)
>   {
> -    return syscall(SYS_pkey_alloc, 0, 0);
> +    return syscall(SYS_pkey_alloc, flags, rights);
>   }
>   
>   int
> @@ -224,6 +226,7 @@ pkey_free(unsigned long pkey)
>   {
>       return syscall(SYS_pkey_free, pkey);
>   }
> +#endif
>   
>   #define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \e
>                              } while (0)
> @@ -252,7 +255,7 @@ main(void)
>       /*
>        * Allocate a protection key:
>        */
> -    pkey = pkey_alloc();
> +    pkey = pkey_alloc(0, 0);
>       if (pkey == \-1)
>           errExit("pkey_alloc");
>   
> @@ -260,7 +263,7 @@ main(void)
>        * Disable access to any memory with "pkey" set,
>        * even though there is none right now.
>        */
> -    status = pkey_set(pkey, PKEY_DISABLE_ACCESS, 0);
> +    status = pkey_set(pkey, PKEY_DISABLE_ACCESS);
>       if (status)
>           errExit("pkey_set");
>   

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 7/9] pkeys.7: Update the example to match glibc

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

Hi Alex,

On Sat, 8 Jan 2022 02:59:12 +0100, "Alejandro Colomar (man-pages)"
<alx.manpages@gmail.com> wrote:
> On 1/7/22 17:46, Stephen Kitt wrote:
> > glibc 2.27 introduced support for the pkeys functions, but the glibc
> > versions don't match those declared in the example. Update the example
> > to match glibc, and avoid declaring the functions if glibc is new
> > enough. >
> > Signed-off-by: Stephen Kitt <steve@sk2.org>  
> 
> 
> There are a few problems with the prototypes.
> 
> 
> alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype wrpkru
> alx@ady1:~/src/gnu/glibc$ grep -rn define.wrpkru
> alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_set
> 60:int pkey_set (int __key, unsigned int __access_rights) __THROW;
> alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_mprotect
> 72:int pkey_mprotect (void *__addr, size_t __len, int __prot, int 
> __pkey) __THROW;
> alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_alloc
> 56:int pkey_alloc (unsigned int __flags, unsigned int __access_rights) 
> __THROW;
> alx@ady1:~/src/gnu/glibc$ grep_glibc_prototype pkey_free
> 68:int pkey_free (int __key) __THROW;
> 
> 
> As you see above, I couldn't find wrpkru().  Are you sure it exists in 
> glibc?

It doesn’t exist in glibc(), but if we use the glibc version of pkey_set(),
it’s not needed in the example. In glibc, the equivalent function is
pkey_write(), but that’s an implementation detail, it’s not part of the API.

> pkey_mprotect(3) uses 'int' instead of 'unsigned long'.  Would you mind 
> fixind that one too?
> 
> pkey_set(3) uses 'unsigned int' instead of 'unsigned long'.  Please fix 
> that one.
> 
> pkey_free(3) uses 'int' instead of 'unsigned long'.  Would you mind 
> fixing that one too?

Right, I’ll take care of all that.

> BTW, I need to modify grep_glibc_prototype() so that it always prints 
> the file name, even if only one file is passed to grep (adding /dev/null 
> to the file list).
> 
> 
> A part from that, I prefer EXAMPLES to be as simple as possible, so I'd 
> do 2 patches.  One to match the definitions to the glibc ones, and then 
> one commit removing old code, assuming glibc is new enough.  Would you 
> mind sending a subsequent patch to remove everything under #if ... #endif?

Right, will do!

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/9] pkeys.7: Update the example to match glibc

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/8/22 15:18, Stephen Kitt wrote:
>> A part from that, I prefer EXAMPLES to be as simple as possible, so I'd
>> do 2 patches.  One to match the definitions to the glibc ones, and then
>> one commit removing old code, assuming glibc is new enough.  Would you
>> mind sending a subsequent patch to remove everything under #if ... #endif?
> 
> Right, will do!

If you know those functions enough and would like to send new pages for 
them, that'd be great, but maybe too much to ask.  The simplest thing 
would be enough, and if someone cares enough to add more info, we can 
add it later.


Thanks,

Alex

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[PATCH 8/9] strtok.3: Enable example analysis, fix declaration

[Stephen Kitt]

    for (int j = 1, str1 = argv[1]; ...

declares two variables of type int, j and str1; the pre-existing
char * str1 isn't used. This causes compiler warnings. Declaring j
outside the loop fixes everything.

To enable automated source extraction, separate the text following the
code.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/strtok.3 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/man3/strtok.3 b/man3/strtok.3
index aec914094..19d5d9204 100644
--- a/man3/strtok.3
+++ b/man3/strtok.3
@@ -255,6 +255,7 @@ main(int argc, char *argv[])
 {
     char *str1, *str2, *token, *subtoken;
     char *saveptr1, *saveptr2;
+    int j;
 
     if (argc != 4) {
         fprintf(stderr, "Usage: %s string delim subdelim\en",
@@ -262,7 +263,7 @@ main(int argc, char *argv[])
         exit(EXIT_FAILURE);
     }
 
-    for (int j = 1, str1 = argv[1]; ; j++, str1 = NULL) {
+    for (j = 1, str1 = argv[1]; ; j++, str1 = NULL) {
         token = strtok_r(str1, argv[2], &saveptr1);
         if (token == NULL)
             break;
@@ -280,6 +281,7 @@ main(int argc, char *argv[])
 }
 .EE
 .PP
+.SS Further examples
 Another example program using
 .BR strtok ()
 can be found in
-- 
2.30.2

Re: [PATCH 8/9] strtok.3: Enable example analysis, fix declaration

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
>      for (int j = 1, str1 = argv[1]; ...
> 
> declares two variables of type int, j and str1; the pre-existing
> char * str1 isn't used. This causes compiler warnings. Declaring j
> outside the loop fixes everything.
> 
> To enable automated source extraction, separate the text following the
> code.
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>

Since these are two completely unrelated things, I'd prefer 2 patches.
If you resend this one without the subsection heading, I'll apply it.

Thanks,

Alex

> ---
>   man3/strtok.3 | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/man3/strtok.3 b/man3/strtok.3
> index aec914094..19d5d9204 100644
> --- a/man3/strtok.3
> +++ b/man3/strtok.3
> @@ -255,6 +255,7 @@ main(int argc, char *argv[])
>   {
>       char *str1, *str2, *token, *subtoken;
>       char *saveptr1, *saveptr2;
> +    int j;
>   
>       if (argc != 4) {
>           fprintf(stderr, "Usage: %s string delim subdelim\en",
> @@ -262,7 +263,7 @@ main(int argc, char *argv[])
>           exit(EXIT_FAILURE);
>       }
>   
> -    for (int j = 1, str1 = argv[1]; ; j++, str1 = NULL) {
> +    for (j = 1, str1 = argv[1]; ; j++, str1 = NULL) {
>           token = strtok_r(str1, argv[2], &saveptr1);
>           if (token == NULL)
>               break;
> @@ -280,6 +281,7 @@ main(int argc, char *argv[])
>   }
>   .EE
>   .PP
> +.SS Further examples
>   Another example program using
>   .BR strtok ()
>   can be found in

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[PATCH 9/9] malloc_info.3: Use intptr_t to store pointers

[Stephen Kitt]

int isn't large enough to store pointers on all platforms, use
intptr_t instead.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
 man3/malloc_info.3 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man3/malloc_info.3 b/man3/malloc_info.3
index a5b8d34f9..3baa891fd 100644
--- a/man3/malloc_info.3
+++ b/man3/malloc_info.3
@@ -198,7 +198,7 @@ static int numThreads, numBlocks;
 static void *
 thread_func(void *arg)
 {
-    int tn = (int) arg;
+    intptr_t tn = (intptr_t) arg;
 
     /* The multiplier \(aq(2 + tn)\(aq ensures that each thread (including
        the main thread) allocates a different amount of memory. */
@@ -237,7 +237,7 @@ main(int argc, char *argv[])
 
     /* Create threads that allocate different amounts of memory. */
 
-    for (int tn = 0; tn < numThreads; tn++) {
+    for (intptr_t tn = 0; tn < numThreads; tn++) {
         errno = pthread_create(&thr[tn], NULL, thread_func,
                                (void *) tn);
         if (errno != 0)
-- 
2.30.2

Re: [PATCH 9/9] malloc_info.3: Use intptr_t to store pointers

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> int isn't large enough to store pointers on all platforms, use
> intptr_t instead.

Well, since the pointer came from a previous 'int', there should be no 
problem.  But since the C language (or even POSIX) is very permissive 
about what a conforming implementation can do with pointers, and it only 
guarantees conversions to/from [u]intptr_t, I'd take this patch for 
correctness.  However...

I still don't like at all the fact that compilers require an explicit 
cast.  Casts are dangerous.  Period.  I haven't seen a place where casts 
are good, except when you explicitly want to rely on undefined behavior 
(which I've found very useful in a few cases).  It should be clear to 
the compiler that we're doing the right thing, since we declared the 
variable to be 'intptr_t', which is by itself very rare.  Requiring a 
cast adds no clarity, while it does increase the chances of a bug.

I'd like to know your opinion.  I think I'll remove the cast, even if 
compilers are going to give a warning for implicitly converting from 
pointer to integer (intptr_t).  I'd say patch the compiler, not the code.

Thanks,

Alex

> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
>   man3/malloc_info.3 | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/man3/malloc_info.3 b/man3/malloc_info.3
> index a5b8d34f9..3baa891fd 100644
> --- a/man3/malloc_info.3
> +++ b/man3/malloc_info.3
> @@ -198,7 +198,7 @@ static int numThreads, numBlocks;
>   static void *
>   thread_func(void *arg)
>   {
> -    int tn = (int) arg;
> +    intptr_t tn = (intptr_t) arg;
>   
>       /* The multiplier \(aq(2 + tn)\(aq ensures that each thread (including
>          the main thread) allocates a different amount of memory. */
> @@ -237,7 +237,7 @@ main(int argc, char *argv[])
>   
>       /* Create threads that allocate different amounts of memory. */
>   
> -    for (int tn = 0; tn < numThreads; tn++) {
> +    for (intptr_t tn = 0; tn < numThreads; tn++) {
>           errno = pthread_create(&thr[tn], NULL, thread_func,
>                                  (void *) tn);
>           if (errno != 0)

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 9/9] malloc_info.3: Use intptr_t to store pointers

[Jakub Wilk]

For the record, this is what you get when you compile the original code 
on a 64-bit architecture:

    $ gcc -Wall -pthread malloc_info-example.c
    malloc_info-example.c: In function 'thread_func':
    malloc_info-example.c:16:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
       16 |     int tn = (int) arg;
          |              ^
    malloc_info-example.c: In function 'main':
    malloc_info-example.c:57:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
       57 |                                (void *) tn);
          |                                ^

* Alejandro Colomar <alx.manpages@gmail.com>, 2022-01-08, 03:25:
>On 1/7/22 17:46, Stephen Kitt wrote:
>>int isn't large enough to store pointers on all platforms, use 
>>intptr_t instead.
>
>Well, since the pointer came from a previous 'int', there should be no 
>problem.  But since the C language (or even POSIX) is very permissive 
>about what a conforming implementation can do with pointers, and it 
>only guarantees conversions to/from [u]intptr_t, I'd take this patch 
>for correctness.  However...

The standards guarantee that void* → intptr_t → void* round-trips, 
but that's not what this code does.

The example converts int → void* → int. Changing int to intptr_t makes 
the compiler warnings go away, but I don't think it improves correctness 
in any way.

-- 
Jakub Wilk

Re: [PATCH 9/9] malloc_info.3: Use intptr_t to store pointers

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

On Sat, 8 Jan 2022 11:30:41 +0100, Jakub Wilk <jwilk@jwilk.net> wrote:
> For the record, this is what you get when you compile the original code 
> on a 64-bit architecture:
> 
>     $ gcc -Wall -pthread malloc_info-example.c
>     malloc_info-example.c: In function 'thread_func':
>     malloc_info-example.c:16:14: warning: cast from pointer to integer of
> different size [-Wpointer-to-int-cast] 16 |     int tn = (int) arg;
>           |              ^
>     malloc_info-example.c: In function 'main':
>     malloc_info-example.c:57:32: warning: cast to pointer from integer of
> different size [-Wint-to-pointer-cast] 57 |
> (void *) tn); |                                ^
> 
> * Alejandro Colomar <alx.manpages@gmail.com>, 2022-01-08, 03:25:
> >On 1/7/22 17:46, Stephen Kitt wrote:  
> >>int isn't large enough to store pointers on all platforms, use 
> >>intptr_t instead.  
> >
> >Well, since the pointer came from a previous 'int', there should be no 
> >problem.  But since the C language (or even POSIX) is very permissive 
> >about what a conforming implementation can do with pointers, and it 
> >only guarantees conversions to/from [u]intptr_t, I'd take this patch 
> >for correctness.  However...  
> 
> The standards guarantee that void* → intptr_t → void* round-trips, 
> but that's not what this code does.
> 
> The example converts int → void* → int. Changing int to intptr_t makes 
> the compiler warnings go away, but I don't think it improves correctness 
> in any way.

Agreed, the example is just trying to pass an int into the function given to
pthread_create. AFAICT the only requirement for correctness is that the type
used is at most the size of void *.

It’s probably not worth bothering about the compiler warning...

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 9/9] malloc_info.3: Use intptr_t to store pointers

[Alejandro Colomar (man-pages)]

Hi Jakub,

On 1/8/22 11:30, Jakub Wilk wrote:
> For the record, this is what you get when you compile the original code 
> on a 64-bit architecture:
> 
>     $ gcc -Wall -pthread malloc_info-example.c
>     malloc_info-example.c: In function 'thread_func':
>     malloc_info-example.c:16:14: warning: cast from pointer to integer 
> of different size [-Wpointer-to-int-cast]
>        16 |     int tn = (int) arg;
>           |              ^
>     malloc_info-example.c: In function 'main':
>     malloc_info-example.c:57:32: warning: cast to pointer from integer 
> of different size [-Wint-to-pointer-cast]
>        57 |                                (void *) tn);
>           |                                ^
> 
> * Alejandro Colomar <alx.manpages@gmail.com>, 2022-01-08, 03:25:
>> On 1/7/22 17:46, Stephen Kitt wrote:
>>> int isn't large enough to store pointers on all platforms, use 
>>> intptr_t instead.
>>
>> Well, since the pointer came from a previous 'int', there should be no 
>> problem.  But since the C language (or even POSIX) is very permissive 
>> about what a conforming implementation can do with pointers, and it 
>> only guarantees conversions to/from [u]intptr_t, I'd take this patch 
>> for correctness.  However...
> 
> The standards guarantee that void* → intptr_t → void* round-trips, but 
> that's not what this code does.
> 
> The example converts int → void* → int. Changing int to intptr_t makes 
> the compiler warnings go away, but I don't think it improves correctness 
> in any way.
> 


Hmm, you're right.
Semantically, 'int' feels better, since the original data is 'int', not 
a pointer.  So a cast here to pass through the pointer API would be ok. 
  I don't think there's any arch where it would be a problem.

I'll keep the program as is.

Thanks,

Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 1/9] Add a target to check example programs

[Alejandro Colomar (man-pages)]

Hi Stephen,

On 1/7/22 17:46, Stephen Kitt wrote:
> This is a first step to automating example program checks. It extracts
> source snippets introduced by "Program source", assuming it's C, and
> attempts to compile it.
> 
> For now, only man pages with a single "Program source" entry are
> processed. The compiled code isn't linked to avoid having to handle
> library requirements (e.g. -ldl).
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>

I like the concept of this patch.

However, a few things:

- I prefer a script in ./scripts/,
   which can be called from the Makefile.
   I'd like to keep the Makefile simple
   (it's already quite complex to add more stuff to it).

- I'd like to use make(1) properly,
   and only test programs incrementally,
   so if a page has already been tested and it hasn't been modified,
   it should not be retested.
   That part should go in the Makefile, not the test itself.
   For that, I'd chose some arbitrary dirname
   (<./tests/example_programs/> sounds like a good candidate),
   and touch dummy files there when a test is performed:


builddir := $(CURDIR)/build
TESTS_example_programs := $(patsubst 
$(MANDIR)/%,$(builddir)/%.example-programs.touch,$(MANPAGES))

$(TESTS_example_programs): $(builddir)/%.example-programs.touch: \
                                       $(MANDIR)/% \
                                       Makefile \
 
$(CURDIR)/scripts/check_example_programs.sh \
                                       | $$@D)/.
	$(info TEST example programs:	$@)
	$(CURDIR)/scripts/check_example_programs $@
	touch $@


Something like the above would be nice.
That would also remove the call to mktemp(1).

- This expects pages to have 'Program source' just before the source.
   Few pages do have that currently.
   Do you have plans to standardize some convention in the man pages?
   I'd be happy to see that.
   I think a good approach would be to use C comments,
   one line before the code, and one line after the code,
   so that there's no doubts about the limits of the code
   (we should decide the format of the comment).
   The comment should be something not too noisy, but very reliable.

   I think I'd also first restrict to the EXAMPLES sections,
   even before reading that hypothetical comment (or whatever we put),
   to avoid mistakes.

- Logs should go to stdout/stderr,
   as in any other standard Unix command,
   so just let the compiler print wherever it wants to print,
   and let the user redirect them to wherever the user wants to too.

   I know there was the groff-warnings test.  But I don't like it.
   It's there because it predates me,
   and I yet have to understand how and if it works,
   and then I'll rewrite it properly.


Cheers,

Alex

> ---
>   Makefile | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 0da0328d8..3f597d7c3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -266,6 +266,23 @@ check-groff-warnings:
>   	done; \
>   	rm -f "$$GROFF_LOG"
>   
> +# Check that man pages' examples programs (for now, only pages with a
> +# single program) actually compile
> +.PHONY: check-example-programs
> +check-example-programs:
> +	GCC_LOG="$$(mktemp --tmpdir manpages-checksXXXX)" || exit $$?; \
> +	for manpage in $$(grep -rc 'Program source' man?/* | grep ':1$$' | cut -d: -f1); \
> +	do \
> +		(man -Tutf8 $$manpage | \
> +			col -bx | \
> +			sed -n '/Program source/,/^[[:space:]]\{0,3\}[^[:space:]]/p' | \
> +			tail -n+2 | \
> +			head -n-1 | \
> +			gcc -xc -c -o/dev/null -) 2>| "$$GCC_LOG"; \
> +		[ -s "$$GCC_LOG" ] && { echo "$$manpage: "; cat "$$GCC_LOG"; echo; }; \
> +	done; \
> +	rm -f "$$GCC_LOG"
> +
>   # someone might also want to look at /var/catman/cat2 or so ...
>   # a problem is that the location of cat pages varies a lot
>   

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 1/9] Add a target to check example programs

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]

Hi Alex,

On Sat, 8 Jan 2022 02:15:47 +0100, "Alejandro Colomar (man-pages)"
<alx.manpages@gmail.com> wrote:
> On 1/7/22 17:46, Stephen Kitt wrote:
> > This is a first step to automating example program checks. It extracts
> > source snippets introduced by "Program source", assuming it's C, and
> > attempts to compile it.
> > 
> > For now, only man pages with a single "Program source" entry are
> > processed. The compiled code isn't linked to avoid having to handle
> > library requirements (e.g. -ldl).
> > 
> > Signed-off-by: Stephen Kitt <steve@sk2.org>  
> 
> I like the concept of this patch.
> 
> However, a few things:
> 
> - I prefer a script in ./scripts/,
>    which can be called from the Makefile.
>    I'd like to keep the Makefile simple
>    (it's already quite complex to add more stuff to it).
> 
> - I'd like to use make(1) properly,
>    and only test programs incrementally,
>    so if a page has already been tested and it hasn't been modified,
>    it should not be retested.
>    That part should go in the Makefile, not the test itself.
>    For that, I'd chose some arbitrary dirname
>    (<./tests/example_programs/> sounds like a good candidate),
>    and touch dummy files there when a test is performed:
> 
> 
> builddir := $(CURDIR)/build
> TESTS_example_programs := $(patsubst 
> $(MANDIR)/%,$(builddir)/%.example-programs.touch,$(MANPAGES))
> 
> $(TESTS_example_programs): $(builddir)/%.example-programs.touch: \
>                                        $(MANDIR)/% \
>                                        Makefile \
>  
> $(CURDIR)/scripts/check_example_programs.sh \
>                                        | $$@D)/.
> 	$(info TEST example programs:	$@)
> 	$(CURDIR)/scripts/check_example_programs $@
> 	touch $@
> 
> 
> Something like the above would be nice.
> That would also remove the call to mktemp(1).

Yes, that’s along the lines of what I was thinking too; in particular,
something like that would be necessary to handle man pages with multiple
example files (e.g. unix.7).

The main purpose of this patch was to get the conversation started and to
show how I’d identified the problems I was fixing in the rest of the series.
I wanted to have something simple to get going, just to measure how likely it
was that there actually were problems in the example programs before
embarking on a more complex process ;-).

> - This expects pages to have 'Program source' just before the source.
>    Few pages do have that currently.
>    Do you have plans to standardize some convention in the man pages?
>    I'd be happy to see that.
>    I think a good approach would be to use C comments,
>    one line before the code, and one line after the code,
>    so that there's no doubts about the limits of the code
>    (we should decide the format of the comment).
>    The comment should be something not too noisy, but very reliable.
> 
>    I think I'd also first restrict to the EXAMPLES sections,
>    even before reading that hypothetical comment (or whatever we put),
>    to avoid mistakes.

At first I’d tried going with troff comments in the man page sources, but
that doesn’t work because we rely on the troff *output* being correct C, not
the troff input. So we need something that can be processed from the output,
and “Program source” was available for a first stab.

Ultimately though, as you say, we’d probably need C comments to start and
finish the code. That would fit in nicely with a Make-based approach: we
could have one target to generate the “dependencies” (C files generated from
man pages), then include that — that way we’d only update C files when the
corresponding man page changes.

> - Logs should go to stdout/stderr,
>    as in any other standard Unix command,
>    so just let the compiler print wherever it wants to print,
>    and let the user redirect them to wherever the user wants to too.
> 
>    I know there was the groff-warnings test.  But I don't like it.
>    It's there because it predates me,
>    and I yet have to understand how and if it works,
>    and then I'll rewrite it properly.

And I was basing my code on the groff-warnings test ;-). From running a
number of variants, what I liked about this is that it provides output
(including the name of the file being processed) only when an error occurs.
If we only rely on the regular output going to stderr, we also need to output
something for every file being processed, which clutters the output IMO.

But anyone serious about working on this could be expected to redirect the
output to a file!

For follow-up revisions of the patches, should I target
https://github.com/alejandro-colomar/man-pages instead of
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git?

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/9] Add a target to check example programs

[Alejandro Colomar (man-pages)]

On 1/8/22 10:22, Stephen Kitt wrote:
>> Something like the above would be nice.
>> That would also remove the call to mktemp(1).
> 
> Yes, that’s along the lines of what I was thinking too; in particular,
> something like that would be necessary to handle man pages with multiple
> example files (e.g. unix.7).
> 
> The main purpose of this patch was to get the conversation started and to
> show how I’d identified the problems I was fixing in the rest of the series.
> I wanted to have something simple to get going, just to measure how likely it
> was that there actually were problems in the example programs before
> embarking on a more complex process ;-).

Okay.  I tried to come up with something the last time you mentioned 
this (IIRC it was you a few months ago that mentioned this plan, 
right?), but didn't have good ideas.  After reading this email of yours 
I have the feeling that this will be simpler than I thought back then.

> 
>> - This expects pages to have 'Program source' just before the source.
>>     Few pages do have that currently.
>>     Do you have plans to standardize some convention in the man pages?
>>     I'd be happy to see that.
>>     I think a good approach would be to use C comments,
>>     one line before the code, and one line after the code,
>>     so that there's no doubts about the limits of the code
>>     (we should decide the format of the comment).
>>     The comment should be something not too noisy, but very reliable.
>>
>>     I think I'd also first restrict to the EXAMPLES sections,
>>     even before reading that hypothetical comment (or whatever we put),
>>     to avoid mistakes.
> 
> At first I’d tried going with troff comments in the man page sources, but
> that doesn’t work because we rely on the troff *output* being correct C, not
> the troff input. So we need something that can be processed from the output,
> and “Program source” was available for a first stab.

No.  You had a point with that of the troff comments.  I think that's 
the key.  See man_section(), defined in <scripts/bash_aliases>.  There I 
did something similar to read junks from the man pages (that was for 
efficiently printing just specific sections of the manpages).  We can 
get something very similar for printing just specific parts that aren't 
necessarily sections.  Let me write some (untested) draft:

function man_example_program()
{
	if (($# < 3)); then
		>&2 echo "Usage: ${FUNCNAME[0]} <manpage> <begin-pattern> <end-pattern>";
		return ${EX_USAGE};
	fi;

	local page="$1";
	local begin="$2";
	local end="$3";

	cat \
	<(<$f sed -n '/^\.TH/,/^\.SH/{/^\.SH/!p}') \
	<(<$f sed -n "/$begin/,/$end/p") \
	|man -P cat -l - \
	|tail -n +2 \
	|head -n -1;
}


We could write something based on that.

> 
> Ultimately though, as you say, we’d probably need C comments to start and
> finish the code. That would fit in nicely with a Make-based approach: we
> could have one target to generate the “dependencies” (C files generated from
> man pages), then include that — that way we’d only update C files when the
> corresponding man page changes.

Yes, an intermediate file could be the .c files.  The C comment I don't 
like it so much, since it's noisy for the reader (although we could 
maybe use a C comment if it improves readability of the page).  Let's 
start with your idea of the groff comment, and if we see that C comments 
might help we could change.

> 
>> - Logs should go to stdout/stderr,
>>     as in any other standard Unix command,
>>     so just let the compiler print wherever it wants to print,
>>     and let the user redirect them to wherever the user wants to too.
>>
>>     I know there was the groff-warnings test.  But I don't like it.
>>     It's there because it predates me,
>>     and I yet have to understand how and if it works,
>>     and then I'll rewrite it properly.
> 
> And I was basing my code on the groff-warnings test ;-). From running a
> number of variants, what I liked about this is that it provides output
> (including the name of the file being processed) only when an error occurs.
> If we only rely on the regular output going to stderr, we also need to output
> something for every file being processed, which clutters the output IMO.

That's why on the seventh day god invented >/dev/null and went to sleep :p

I'm used to having a line per file for makefiles.  It goes a bit against 
the Unix thing of muting until something goes wrong, but compilation is 
usually slow enough to make people unhappy if there's no progress 
indicator.  A line per file is usually not too noisy.

> 
> But anyone serious about working on this could be expected to redirect the
> output to a file!
> 
> For follow-up revisions of the patches, should I target
> https://github.com/alejandro-colomar/man-pages instead of
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git?

Normally, I'd say follow kernel.org.  But now, there's too much of a 
difference between them, so you'll be better following my main branch, 
which has many changes that will some day be pushed to the kernel.org 
repo.  However, be warned that my branches aren't stable at all, and I 
may rebase them against Michael's master branch when it's updated (and 
branches other than my main branch are even more unstable to the point 
that I may remove them).  Just consider my main to be Debian's testing, 
and the others sid/experimental.  But kernel.org is oldoldstable at the 
moment ;)

Also, I've recently started serving my own git at home, and will some 
day remove the github repo, but for the moment you can still rely on it.

If you prefer, however, you could already use 
<git://www.alejandro-colomar.es/src/linux/man-pages.git>.  There, I've 
only uploaded my main branch for the moment.  There's no browsing yet, 
but I plan to add it.  For the moment it's just git:// (and SSH for me).

Regards,

Alex

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 1/9] Add a target to check example programs

[Alejandro Colomar (man-pages)]

On 1/8/22 20:05, Alejandro Colomar (man-pages) wrote:
> If you prefer, however, you could already use 
> <git://www.alejandro-colomar.es/src/linux/man-pages.git>.  There's no browsing yet, 
> but I plan to add it.  For the moment it's just git:// (and SSH for me).

Typo there.
<git://www.alejandro-colomar.es/src/alx/linux/man-pages.git>
is the correct one.

> 
> Regards,
> 
> Alex
> 

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH 1/9] Add a target to check example programs

[Alejandro Colomar (man-pages)]

On 1/7/22 17:46, Stephen Kitt wrote:
> The compiled code isn't linked to avoid having to handle
> library requirements (e.g. -ldl).

BTW, I plan to add that information so that you will be able to parse it 
easily in a script.  See 
<https://lore.kernel.org/linux-man/20210911160117.552617-1-alx.manpages@gmail.com/> 
and <https://github.com/alejandro-colomar/man-pages/tree/library>

Cheers,

Alex

-- 
Alejandro Colomar
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/