[PATCH 0/7] Remove and/or fix casts

[PATCH 0/7] Remove and/or fix casts

[Alejandro Colomar]

Hi Michael,

[PATCH 1/7] sock_diag.7: Remove unneeded casts
[PATCH 2/7] pthread_sigmask.3: Remove unneeded casts
[PATCH 3/7] msgop.2: Remove unneeded casts
[PATCH 4/7] user_namespaces.7: Remove unneeded cast
[PATCH 5/7] dlopen.3: Remove unneeded cast
[PATCH 6/7] bsearch.3: Fix intermediate type and remove unneeded cast
[PATCH 7/7] qsort.3: Fix casts

Here's a set of patches removing unneeded casts when they are
unneeded or incorrect, and fixing those that are incorrect but can't
be removed.

Regards,

Alex.

[PATCH 1/7] sock_diag.7: Remove unneeded casts

[Alejandro Colomar]

The type `struct sockaddr_nl *` is implicitly casted to `void *`.
Explicitly casting can silence warnings when mistakes are made, so it's
better to remove those casts when possible.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man7/sock_diag.7 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man7/sock_diag.7 b/man7/sock_diag.7
index a5816ceca..185fb77ee 100644
--- a/man7/sock_diag.7
+++ b/man7/sock_diag.7
@@ -680,7 +680,7 @@ send_query(int fd)
         .iov_len = sizeof(req)
     };
     struct msghdr msg = {
-        .msg_name = (void *) &nladdr,
+        .msg_name = &nladdr,
         .msg_namelen = sizeof(nladdr),
         .msg_iov = &iov,
         .msg_iovlen = 1
@@ -765,7 +765,7 @@ receive_responses(int fd)
 
     for (;;) {
         struct msghdr msg = {
-            .msg_name = (void *) &nladdr,
+            .msg_name = &nladdr,
             .msg_namelen = sizeof(nladdr),
             .msg_iov = &iov,
             .msg_iovlen = 1
-- 
2.28.0

[PATCH 2/7] pthread_sigmask.3: Remove unneeded casts

[Alejandro Colomar]

The type `sigset_t *` is implicitly casted to `void *`.
Explicitly casting can silence warnings when mistakes are made, so it's
better to remove those casts when possible.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/pthread_sigmask.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/pthread_sigmask.3 b/man3/pthread_sigmask.3
index 77fd49a89..87a58728a 100644
--- a/man3/pthread_sigmask.3
+++ b/man3/pthread_sigmask.3
@@ -154,7 +154,7 @@ main(int argc, char *argv[])
     if (s != 0)
         handle_error_en(s, "pthread_sigmask");
 
-    s = pthread_create(&thread, NULL, &sig_thread, (void *) &set);
+    s = pthread_create(&thread, NULL, &sig_thread, &set);
     if (s != 0)
         handle_error_en(s, "pthread_create");
 
-- 
2.28.0

[PATCH 3/7] msgop.2: Remove unneeded casts

[Alejandro Colomar]

The type `struct msgbuf *` is implicitly casted to `const void *`.
Not only that, but the explicit cast to `void *` was slightly
misleading.
Explicitly casting can silence warnings when mistakes are made, so it's
better to remove those casts when possible.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/msgop.2 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man2/msgop.2 b/man2/msgop.2
index f992d91a3..373e53aa7 100644
--- a/man2/msgop.2
+++ b/man2/msgop.2
@@ -637,7 +637,7 @@ send_msg(int qid, int msgtype)
     snprintf(msg.mtext, sizeof(msg.mtext), "a message at %s",
             ctime(&t));
 
-    if (msgsnd(qid, (void *) &msg, sizeof(msg.mtext),
+    if (msgsnd(qid, &msg, sizeof(msg.mtext),
                 IPC_NOWAIT) == \-1) {
         perror("msgsnd error");
         exit(EXIT_FAILURE);
@@ -650,7 +650,7 @@ get_msg(int qid, int msgtype)
 {
     struct msgbuf msg;
 
-    if (msgrcv(qid, (void *) &msg, sizeof(msg.mtext), msgtype,
+    if (msgrcv(qid, &msg, sizeof(msg.mtext), msgtype,
                MSG_NOERROR | IPC_NOWAIT) == \-1) {
         if (errno != ENOMSG) {
             perror("msgrcv");
-- 
2.28.0

[PATCH 4/7] user_namespaces.7: Remove unneeded cast

[Alejandro Colomar]

Casting `void *` to `struct child_args *` is already done implicitly.
Explicitly casting can silence warnings when mistakes are made, so it's
better to remove those casts when possible.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man7/user_namespaces.7 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man7/user_namespaces.7 b/man7/user_namespaces.7
index 954a50887..165b1b3d4 100644
--- a/man7/user_namespaces.7
+++ b/man7/user_namespaces.7
@@ -1222,7 +1222,7 @@ proc_setgroups_write(pid_t child_pid, char *str)
 static int              /* Start function for cloned child */
 childFunc(void *arg)
 {
-    struct child_args *args = (struct child_args *) arg;
+    struct child_args *args = arg;
     char ch;
 
     /* Wait until the parent has updated the UID and GID mappings.
-- 
2.28.0

[PATCH 5/7] dlopen.3: Remove unneeded cast

[Alejandro Colomar]

Casting `void *` to `double (*cosine)(double)` is already done
implicitly.
I had doubts about this one, but `gcc -Wall -Wextra` didn't complain
about it.
Explicitly casting can silence warnings when mistakes are made, so it's
better to remove those casts when possible.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/dlopen.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/dlopen.3 b/man3/dlopen.3
index 8e18f70c0..2de358ea3 100644
--- a/man3/dlopen.3
+++ b/man3/dlopen.3
@@ -581,7 +581,7 @@ main(void)
 
     dlerror();    /* Clear any existing error */
 
-    cosine = (double (*)(double)) dlsym(handle, "cos");
+    cosine = dlsym(handle, "cos");
 
     /* According to the ISO C standard, casting between function
        pointers and 'void *', as done above, produces undefined results.
-- 
2.28.0

[PATCH 6/7] bsearch.3: Fix intermediate type and remove unneeded casts

[Alejandro Colomar]

Casting `const void *` to `struct mi *` should result in a warning if
done implicitly.  The explicit cast was probably silencing that warning.
`const` can and should be kept.
Now, casting `const void *` to `const struct mi *` is done implicitly.

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/bsearch.3 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man3/bsearch.3 b/man3/bsearch.3
index 6859bdba2..a07bfd568 100644
--- a/man3/bsearch.3
+++ b/man3/bsearch.3
@@ -114,8 +114,8 @@ struct mi {
 static int
 compmi(const void *m1, const void *m2)
 {
-    struct mi *mi1 = (struct mi *) m1;
-    struct mi *mi2 = (struct mi *) m2;
+    const struct mi *mi1 = m1;
+    const struct mi *mi2 = m2;
     return strcmp(mi1\->name, mi2\->name);
 }
 
-- 
2.28.0

[PATCH 7/7] qsort.3: Fix casts

[Alejandro Colomar]

`p1` (and `p2` too) is `const void *` and it comes from a
`const char **` (for legacy reasons, argv is not `const` but should be
treated as if it were).  That means, the ultimate `char` is `const`:
"a pointer to a pointer to a const char".

Let's see what is going on before the fix first, and then the fix.

Before the fix:

`(char *const *)` (I removed the space on purpose) casts `p1` to be
"a pointer to a const pointer to a non-const char".  That's clearly
not what it originally was.

Then we dereference, ending with a `char *const`, which is
"a const pointer to a non-const char".  But given that the pointer value
is passed to a function, `const` doesn't make sense there, because the
function will already take a copy of it, so it is impossible to modify
the pointer itself.

The fix:

`(const char **)` The only thing that is const is the ultimate `char`,
which is the only thing that matters, because it is the only thing
strcmp(3) has access to (everything else, i.e. the pointers, are
copies).

Then, after the dereference we end up with `const char *`, the type of
argv (more or less, as previously noted), which is also the type of the
arguments to strcmp(3).

Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man3/qsort.3 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man3/qsort.3 b/man3/qsort.3
index e1af43cf0..24f0b6c92 100644
--- a/man3/qsort.3
+++ b/man3/qsort.3
@@ -137,7 +137,7 @@ cmpstringp(const void *p1, const void *p2)
        pointers to char", but strcmp(3) arguments are "pointers
        to char", hence the following cast plus dereference */
 
-    return strcmp(* (char * const *) p1, * (char * const *) p2);
+    return strcmp(*(const char **) p1, *(const char **) p2);
 }
 
 int
-- 
2.28.0

Re: [PATCH 0/7] Remove and/or fix casts

[Michael Kerrisk (man-pages)]

Hello Alex,

On 9/5/20 5:14 PM, Alejandro Colomar wrote:
> Hi Michael,
> 
> [PATCH 1/7] sock_diag.7: Remove unneeded casts
> [PATCH 2/7] pthread_sigmask.3: Remove unneeded casts
> [PATCH 3/7] msgop.2: Remove unneeded casts
> [PATCH 4/7] user_namespaces.7: Remove unneeded cast
> [PATCH 5/7] dlopen.3: Remove unneeded cast
> [PATCH 6/7] bsearch.3: Fix intermediate type and remove unneeded cast
> [PATCH 7/7] qsort.3: Fix casts
> 
> Here's a set of patches removing unneeded casts when they are
> unneeded or incorrect, and fixing those that are incorrect but can't
> be removed.

I've applied all of these ,except 5/7 (I'll reply to that mail).
Mea culpa for 2 and 4...

Thanks,

Michael


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

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Michael Kerrisk (man-pages)]

Hello Alex,

On 9/5/20 5:14 PM, Alejandro Colomar wrote:
> Casting `void *` to `double (*cosine)(double)` is already done
> implicitly.
> I had doubts about this one, but `gcc -Wall -Wextra` didn't complain
> about it.
> Explicitly casting can silence warnings when mistakes are made, so it's
> better to remove those casts when possible.
> 
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
> ---
>  man3/dlopen.3 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/man3/dlopen.3 b/man3/dlopen.3
> index 8e18f70c0..2de358ea3 100644
> --- a/man3/dlopen.3
> +++ b/man3/dlopen.3
> @@ -581,7 +581,7 @@ main(void)
>  
>      dlerror();    /* Clear any existing error */
>  
> -    cosine = (double (*)(double)) dlsym(handle, "cos");
> +    cosine = dlsym(handle, "cos");
>  
>      /* According to the ISO C standard, casting between function
>         pointers and 'void *', as done above, produces undefined results.

This cast really is needed. See the comment just below, and also try
compiling the code with your patch applied:

cc -pedantic -Wall prog.c
d.c: In function ‘main’:
d.c:21:19: warning: ISO C forbids assignment between function pointer and ‘void *’ [-Wpedantic]
   21 |            cosine = dlsym(handle, "cos");
      |                   ^

Thanks,

Michael

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

Re: [PATCH 0/7] Remove and/or fix casts

[Michael Kerrisk (man-pages)]

On 9/6/20 2:59 PM, Michael Kerrisk (man-pages) wrote:
> Hello Alex,
> 
> On 9/5/20 5:14 PM, Alejandro Colomar wrote:
>> Hi Michael,
>>
>> [PATCH 1/7] sock_diag.7: Remove unneeded casts
>> [PATCH 2/7] pthread_sigmask.3: Remove unneeded casts
>> [PATCH 3/7] msgop.2: Remove unneeded casts
>> [PATCH 4/7] user_namespaces.7: Remove unneeded cast
>> [PATCH 5/7] dlopen.3: Remove unneeded cast
>> [PATCH 6/7] bsearch.3: Fix intermediate type and remove unneeded cast
>> [PATCH 7/7] qsort.3: Fix casts
>>
>> Here's a set of patches removing unneeded casts when they are
>> unneeded or incorrect, and fixing those that are incorrect but can't
>> be removed.
> 
> I've applied all of these ,except 5/7 (I'll reply to that mail).
> Mea culpa for 2 and 4...

Oops -- and I meant to say: thanks for the patches.

Cheers,

Michael


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

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Alejandro Colomar]

Hola Michael,

On 9/6/20 3:02 PM, Michael Kerrisk (man-pages) wrote:
> Hello Alex,
>
> On 9/5/20 5:14 PM, Alejandro Colomar wrote:
>> Casting `void *` to `double (*cosine)(double)` is already done
>> implicitly.
>> I had doubts about this one, but `gcc -Wall -Wextra` didn't complain
>> about it.
>> Explicitly casting can silence warnings when mistakes are made, so it's
>> better to remove those casts when possible.
>>
>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>> ---
>>  man3/dlopen.3 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/man3/dlopen.3 b/man3/dlopen.3
>> index 8e18f70c0..2de358ea3 100644
>> --- a/man3/dlopen.3
>> +++ b/man3/dlopen.3
>> @@ -581,7 +581,7 @@ main(void)
>>
>>      dlerror();    /* Clear any existing error */
>>
>> -    cosine = (double (*)(double)) dlsym(handle, "cos");
>> +    cosine = dlsym(handle, "cos");
>>
>>      /* According to the ISO C standard, casting between function
>>         pointers and 'void *', as done above, produces undefined results.
>
> This cast really is needed. See the comment just below, and also try
> compiling the code with your patch applied:
>
> cc -pedantic -Wall prog.c
> d.c: In function ‘main’:
> d.c:21:19: warning: ISO C forbids assignment between function pointer
and ‘void *’ [-Wpedantic]
>    21 |            cosine = dlsym(handle, "cos");
>       |                   ^

Hmmm, not sure about it.

The thing is, standard C doesn't allow this, no matter how.  POSIX does
allow it, however.

The only thing with the casts is to avoid the warning, but they don't
avoid the possible undefined behaviour (only in non-POSIX systems).
But that warning, `-pedantic`, is specifically targeted to warn about
whatever code that is not strict standard C, which this code isn't,
so the warning is legit IMHO, and anyone using `-pedantic` would
probably be warned about this line, and anyone not wanting to be warned
about this line should probably disable `-pedantic`.

So, in POSIX, without `-pedantic`, that line without casts will result
in correct code and no warnings, as expected.

And in non-POSIX, with `-pedantic`, that line without casts will
correctly result in a warning.

And more importatnly, in non-POSIX, with `-pedantic`, that line with
casts will result in no warnings but undefined results.

I'd say that no casting is less problematic than casting, although both
have their problems.


Saludos,

Alex

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Alejandro Colomar]

Hi Michael,

On 9/6/20 3:22 PM, Alejandro Colomar wrote:
> so the warning is legit IMHO, and anyone using `-pedantic` would
> probably
want to
> be warned about this line, and anyone not wanting to be warned
> about this line should probably disable `-pedantic`.


On 9/6/20 3:02 PM, Michael Kerrisk (man-pages) wrote:
>> I've applied all of these ,except 5/7 (I'll reply to that mail).

Actually all of the patches were applied, so probably you
applied 5/7 by accident.  My branch is up to date with upstream/master.

> Oops -- and I meant to say: thanks for the patches.

:-)

Cheers,
Alex.

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Michael Kerrisk (man-pages)]

>>> I've applied all of these ,except 5/7 (I'll reply to that mail).
> 
> Actually all of the patches were applied, so probably you
> applied 5/7 by accident.  My branch is up to date with upstream/master.

D'oh! I just now reverted that patch :-).

Cheers,

Michael

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

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Michael Kerrisk (man-pages)]

Hi Alex,

On 9/6/20 3:22 PM, Alejandro Colomar wrote:
> Hola Michael,
> 
> On 9/6/20 3:02 PM, Michael Kerrisk (man-pages) wrote:
>> Hello Alex,
>>
>> On 9/5/20 5:14 PM, Alejandro Colomar wrote:
>>> Casting `void *` to `double (*cosine)(double)` is already done
>>> implicitly.
>>> I had doubts about this one, but `gcc -Wall -Wextra` didn't complain
>>> about it.
>>> Explicitly casting can silence warnings when mistakes are made, so it's
>>> better to remove those casts when possible.
>>>
>>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>>> ---
>>>  man3/dlopen.3 | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/man3/dlopen.3 b/man3/dlopen.3
>>> index 8e18f70c0..2de358ea3 100644
>>> --- a/man3/dlopen.3
>>> +++ b/man3/dlopen.3
>>> @@ -581,7 +581,7 @@ main(void)
>>>
>>>      dlerror();    /* Clear any existing error */
>>>
>>> -    cosine = (double (*)(double)) dlsym(handle, "cos");
>>> +    cosine = dlsym(handle, "cos");
>>>
>>>      /* According to the ISO C standard, casting between function
>>>         pointers and 'void *', as done above, produces undefined results.
>>
>> This cast really is needed. See the comment just below, and also try
>> compiling the code with your patch applied:
>>
>> cc -pedantic -Wall prog.c
>> d.c: In function ‘main’:
>> d.c:21:19: warning: ISO C forbids assignment between function pointer
> and ‘void *’ [-Wpedantic]
>>    21 |            cosine = dlsym(handle, "cos");
>>       |                   ^
> 
> Hmmm, not sure about it.
> 
> The thing is, standard C doesn't allow this, no matter how.  

Agreed.

> POSIX does allow it, however.

Yes, POSIX is explicit on this point, and the specification
gives an example of the use casts in the manner shown in the
manual page.

> The only thing with the casts is to avoid the warning, but they don't
> avoid the possible undefined behaviour (only in non-POSIX systems).

Yes. (But, dlopen() is a "UNIX-only" API, and thus non-POSIX
doesn't matter here.)

> But that warning, `-pedantic`, is specifically targeted to warn about
> whatever code that is not strict standard C, which this code isn't,
> so the warning is legit IMHO, and anyone using `-pedantic` would
> probably be warned about this line, and anyone not wanting to be warned
> about this line should probably disable `-pedantic`.
> 
> So, in POSIX, without `-pedantic`, that line without casts will result
> in correct code and no warnings, as expected.
> 
> And in non-POSIX, with `-pedantic`, that line without casts will
> correctly result in a warning.
> 
> And more importatnly, in non-POSIX, with `-pedantic`, that line with
> casts will result in no warnings but undefined results.
> 
> I'd say that no casting is less problematic than casting, although both
> have their problems.

Two things:

* The standard uses the casts, and allows the extension on top of
what the C standard permits here. So, I think the manuial page
better use the casts also.
* Sometimes people have to compile a large body of code using
certain compiler options, perhaps including "-pedantic", so they
need to at least be aware of the warning that the cast may incur.

To address the second point, I make use of the appropriate pragma,
to eliminate the waring from -pedantic:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpedantic"
    funcp = (void (*)(void)) dlsym(libHandle, name);
#pragma GCC diagnostic pop

Thanks,

Michael


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

Re: [PATCH 5/7] dlopen.3: Remove unneeded cast

[Alejandro Colomar]

Hi Michael,

On 2020-09-06 17:13, Michael Kerrisk (man-pages) wrote:
>> The only thing with the casts is to avoid the warning, but they don't
>> avoid the possible undefined behaviour (only in non-POSIX systems).
> 
> Yes. (But, dlopen() is a "UNIX-only" API, and thus non-POSIX
> doesn't matter here.)

Agreed.  The cast is useful in this case.
The warning that is completely useless this time.

Cheers,
Alex.