Remove useless NULL casts from mv(1)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Remove useless NULL casts from mv(1)

Michael McConville-2
Index: mv.c
===================================================================
RCS file: /cvs/src/bin/mv/mv.c,v
retrieving revision 1.40
diff -u -p -r1.40 mv.c
--- mv.c 24 Aug 2015 00:10:59 -0000 1.40
+++ mv.c 14 Sep 2015 13:38:13 -0000
@@ -348,7 +348,7 @@ copy(char *from, char *to)
  pid_t pid;
 
  if ((pid = vfork()) == 0) {
- execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
+ execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
  warn("%s", _PATH_CP);
  _exit(1);
  }
@@ -366,7 +366,7 @@ copy(char *from, char *to)
  return (1);
  }
  if (!(pid = vfork())) {
- execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
+ execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
  warn("%s", _PATH_RM);
  _exit(1);
  }

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Philip Guenther-2
On Mon, Sep 14, 2015 at 3:39 PM, Michael McConville
<[hidden email]> wrote:
...
> -               execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
> +               execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);

Nope, this is a case in C where the cast is necessary.  To quote style(9):

     NULL is the preferred null pointer constant.  Use NULL instead of
     (type *)0 or (type *)NULL in all cases except for arguments to variadic
     functions where the compiler does not know the type.


Philip Guenther

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Nicholas Marriott-2
In reply to this post by Michael McConville-2
These used to be necessary but now that NULL is (void *)0 they
aren't.

I'd be inclined to keep them anyway - with the cast it is clearly not a
bug, without is not so clear. There are also loads of them and it'd be a
lot of churn to fix them all, for no real advantage. But I don't feel
strongly about it, others may disagree.



On Mon, Sep 14, 2015 at 09:39:48AM -0400, Michael McConville wrote:

> Index: mv.c
> ===================================================================
> RCS file: /cvs/src/bin/mv/mv.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 mv.c
> --- mv.c 24 Aug 2015 00:10:59 -0000 1.40
> +++ mv.c 14 Sep 2015 13:38:13 -0000
> @@ -348,7 +348,7 @@ copy(char *from, char *to)
>   pid_t pid;
>  
>   if ((pid = vfork()) == 0) {
> - execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
> + execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
>   warn("%s", _PATH_CP);
>   _exit(1);
>   }
> @@ -366,7 +366,7 @@ copy(char *from, char *to)
>   return (1);
>   }
>   if (!(pid = vfork())) {
> - execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
> + execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
>   warn("%s", _PATH_RM);
>   _exit(1);
>   }
>

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Michael McConville-2
Nicholas Marriott wrote:
> These used to be necessary but now that NULL is (void *)0 they
> aren't.
>
> I'd be inclined to keep them anyway - with the cast it is clearly not a
> bug, without is not so clear. There are also loads of them and it'd be a
> lot of churn to fix them all, for no real advantage. But I don't feel
> strongly about it, others may disagree.

No strong feelings here either, and apparently style(9) suggests the
cast. It was just something I came across while reading.

> On Mon, Sep 14, 2015 at 09:39:48AM -0400, Michael McConville wrote:
> > Index: mv.c
> > ===================================================================
> > RCS file: /cvs/src/bin/mv/mv.c,v
> > retrieving revision 1.40
> > diff -u -p -r1.40 mv.c
> > --- mv.c 24 Aug 2015 00:10:59 -0000 1.40
> > +++ mv.c 14 Sep 2015 13:38:13 -0000
> > @@ -348,7 +348,7 @@ copy(char *from, char *to)
> >   pid_t pid;
> >  
> >   if ((pid = vfork()) == 0) {
> > - execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
> > + execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
> >   warn("%s", _PATH_CP);
> >   _exit(1);
> >   }
> > @@ -366,7 +366,7 @@ copy(char *from, char *to)
> >   return (1);
> >   }
> >   if (!(pid = vfork())) {
> > - execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
> > + execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
> >   warn("%s", _PATH_RM);
> >   _exit(1);
> >   }
> >

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Ted Unangst-6
In reply to this post by Michael McConville-2
Michael McConville wrote:

> Index: mv.c
> ===================================================================
> RCS file: /cvs/src/bin/mv/mv.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 mv.c
> --- mv.c 24 Aug 2015 00:10:59 -0000 1.40
> +++ mv.c 14 Sep 2015 13:38:13 -0000
> @@ -348,7 +348,7 @@ copy(char *from, char *to)
>   pid_t pid;
>  
>   if ((pid = vfork()) == 0) {
> - execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
> + execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
>   warn("%s", _PATH_CP);
>   _exit(1);
>   }
> @@ -366,7 +366,7 @@ copy(char *from, char *to)
>   return (1);
>   }
>   if (!(pid = vfork())) {
> - execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
> + execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
>   warn("%s", _PATH_RM);
>   _exit(1);
>   }
>

I don't know where we stand on this. It was previously necessary to have the
cast because otherwise NULL would only get passed as an integer, not a pointer
(long). We have fixed NULL in our headers, but the cast may still be the
correct incantation for some systems. I think the best approach is to delete
casts in other more obvious places, but leave these alone for now.

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Theo de Raadt
In reply to this post by Michael McConville-2
>These used to be necessary but now that NULL is (void *)0 they
>aren't.
>
>I'd be inclined to keep them anyway - with the cast it is clearly not a
>bug, without is not so clear. There are also loads of them and it'd be a
>lot of churn to fix them all, for no real advantage. But I don't feel
>strongly about it, others may disagree.

It is neccesary for portable code.

The discussion goes a bit like this:

1. It is not needed here.
2. It is not needed in src/bin
3. It is not needed in usr.bin
4. Oh look, we deleted it out of sshd...

Seems risky.

Reply | Threaded
Open this post in threaded view
|

Re: Remove useless NULL casts from mv(1)

Theo de Raadt
In reply to this post by Michael McConville-2
>Michael McConville wrote:
>> Index: mv.c
>> ===================================================================
>> RCS file: /cvs/src/bin/mv/mv.c,v
>> retrieving revision 1.40
>> diff -u -p -r1.40 mv.c
>> --- mv.c 24 Aug 2015 00:10:59 -0000 1.40
>> +++ mv.c 14 Sep 2015 13:38:13 -0000
>> @@ -348,7 +348,7 @@ copy(char *from, char *to)
>>   pid_t pid;
>>  
>>   if ((pid = vfork()) == 0) {
>> - execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
>> + execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
>>   warn("%s", _PATH_CP);
>>   _exit(1);
>>   }
>> @@ -366,7 +366,7 @@ copy(char *from, char *to)
>>   return (1);
>>   }
>>   if (!(pid = vfork())) {
>> - execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
>> + execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
>>   warn("%s", _PATH_RM);
>>   _exit(1);
>>   }
>>
>
>I don't know where we stand on this. It was previously necessary to have the
>cast because otherwise NULL would only get passed as an integer, not a pointer
>(long). We have fixed NULL in our headers, but the cast may still be the
>correct incantation for some systems. I think the best approach is to delete
>casts in other more obvious places, but leave these alone for now.

Placing more burden on people who make -portable versions of important
software from our base.