Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Sebastian Benoit]

ok

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2016.03.31 19:16:14 +0200:
> Jeremie Courreges-Anglas  writes:
> 
> > Mike Belopuhov  writes:
> >
> >> Good day, Dimitris.
> >>
> >> Long time ago in a galaxy far far away I've been using this
> >> alongside the -F option that I've added.  While managed
> >> switches are becoming cheaper, I don't see a reason for a
> >> working feature to go away, especially since there has been
> >> zero rationale provided apart from "ndp -f" doesn't work.
> >> Well, then how about fixing ndp?
> > [...]
> >
> > Diff below, seems to work fine.
> 
> Now with the manpage improvements initially proposed by Dimitris.
> I have kept my ndp.c diff, because it is more consistent with the style
> of the rest of the file.  Dimitris's diff points out that main() never
> reports failures (exit(0)), it would be nice to fix.
> 
> ok?
> 
> Index: ndp.8
> ===
> RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
> retrieving revision 1.35
> diff -u -p -r1.35 ndp.8
> --- ndp.8 5 Oct 2015 10:25:19 -   1.35
> +++ ndp.8 31 Mar 2016 16:49:53 -
> @@ -117,6 +117,12 @@ Delete the specified NDP entry.
>  .It Fl f Ar filename
>  Parse the file specified by
>  .Ar filename .
> +Entries in the file should be of the form:
> +.Bd -ragged -offset indent -compact
> +.Ar nodename etheraddr
> +.Op Ar temp
> +.Op Ar proxy
> +.Ed
>  .It Fl H
>  Harmonize consistency between the routing table and the default router
>  list; install the top entry of the list into the kernel routing table.
> Index: ndp.c
> ===
> RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 ndp.c
> --- ndp.c 26 Jan 2016 18:26:19 -  1.69
> +++ ndp.c 31 Mar 2016 16:49:53 -
> @@ -241,6 +241,11 @@ main(int argc, char *argv[])
>   }
>   delete(arg);
>   break;
> + case 'f':
> + if (argc != 0)
> + usage();
> + file(arg);
> + break;
>   case 'p':
>   if (argc != 0) {
>   usage();
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Jeremie Courreges-Anglas  writes:

> Mike Belopuhov  writes:
>
>> Good day, Dimitris.
>>
>> Long time ago in a galaxy far far away I've been using this
>> alongside the -F option that I've added.  While managed
>> switches are becoming cheaper, I don't see a reason for a
>> working feature to go away, especially since there has been
>> zero rationale provided apart from "ndp -f" doesn't work.
>> Well, then how about fixing ndp?
> [...]
>
> Diff below, seems to work fine.

Now with the manpage improvements initially proposed by Dimitris.
I have kept my ndp.c diff, because it is more consistent with the style
of the rest of the file.  Dimitris's diff points out that main() never
reports failures (exit(0)), it would be nice to fix.

ok?

Index: ndp.8
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
retrieving revision 1.35
diff -u -p -r1.35 ndp.8
--- ndp.8   5 Oct 2015 10:25:19 -   1.35
+++ ndp.8   31 Mar 2016 16:49:53 -
@@ -117,6 +117,12 @@ Delete the specified NDP entry.
 .It Fl f Ar filename
 Parse the file specified by
 .Ar filename .
+Entries in the file should be of the form:
+.Bd -ragged -offset indent -compact
+.Ar nodename etheraddr
+.Op Ar temp
+.Op Ar proxy
+.Ed
 .It Fl H
 Harmonize consistency between the routing table and the default router
 list; install the top entry of the list into the kernel routing table.
Index: ndp.c
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
retrieving revision 1.69
diff -u -p -r1.69 ndp.c
--- ndp.c   26 Jan 2016 18:26:19 -  1.69
+++ ndp.c   31 Mar 2016 16:49:53 -
@@ -241,6 +241,11 @@ main(int argc, char *argv[])
}
delete(arg);
break;
+   case 'f':
+   if (argc != 0)
+   usage();
+   file(arg);
+   break;
case 'p':
if (argc != 0) {
usage();


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Dimitris Papastamos  writes:

> On Wed, Mar 30, 2016 at 02:49:06PM +0200, Mike Belopuhov wrote:
>> Good day, Dimitris.
>> 
>> Long time ago in a galaxy far far away I've been using this
>> alongside the -F option that I've added.  While managed
>> switches are becoming cheaper, I don't see a reason for a
>> working feature to go away, especially since there has been
>> zero rationale provided apart from "ndp -f" doesn't work.
>> Well, then how about fixing ndp?  Half of ndp(8) features
>> don't work correctly (hello rdomains), but it's not a valid
>> reason to go ahead and rip useful bits out.
>
> Thanks for the feedback.  I will dig out the first patch I sent
> which basically fixed ndp(8) instead of removing the feature.

oops, I missed your mail and sent another diff.  Will take a look at it
soon.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Mike Belopuhov  writes:

> Good day, Dimitris.
>
> Long time ago in a galaxy far far away I've been using this
> alongside the -F option that I've added.  While managed
> switches are becoming cheaper, I don't see a reason for a
> working feature to go away, especially since there has been
> zero rationale provided apart from "ndp -f" doesn't work.
> Well, then how about fixing ndp?
[...]

Diff below, seems to work fine.

Index: ndp.c
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
retrieving revision 1.69
diff -u -p -p -u -r1.69 ndp.c
--- ndp.c   26 Jan 2016 18:26:19 -  1.69
+++ ndp.c   30 Mar 2016 11:49:50 -
@@ -241,6 +241,11 @@ main(int argc, char *argv[])
}
delete(arg);
break;
+   case 'f':
+   if (argc != 0)
+   usage();
+   file(arg);
+   break;
case 'p':
if (argc != 0) {
usage();


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Dimitris Papastamos]

On Wed, Mar 30, 2016 at 02:49:06PM +0200, Mike Belopuhov wrote:
> Good day, Dimitris.
> 
> Long time ago in a galaxy far far away I've been using this
> alongside the -F option that I've added.  While managed
> switches are becoming cheaper, I don't see a reason for a
> working feature to go away, especially since there has been
> zero rationale provided apart from "ndp -f" doesn't work.
> Well, then how about fixing ndp?  Half of ndp(8) features
> don't work correctly (hello rdomains), but it's not a valid
> reason to go ahead and rip useful bits out.

Thanks for the feedback.  I will dig out the first patch I sent
which basically fixed ndp(8) instead of removing the feature.

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Mike Belopuhov]

Good day, Dimitris.

Long time ago in a galaxy far far away I've been using this
alongside the -F option that I've added.  While managed
switches are becoming cheaper, I don't see a reason for a
working feature to go away, especially since there has been
zero rationale provided apart from "ndp -f" doesn't work.
Well, then how about fixing ndp?  Half of ndp(8) features
don't work correctly (hello rdomains), but it's not a valid
reason to go ahead and rip useful bits out.

Regards,
Mike

On 30 March 2016 at 12:23, Dimitris Papastamos  wrote:
> Hi everyone,
>
> I totally forgot about this patch.  At the time it couldn't go in
> because the tree was locked.  Does it make sense?  If so I will check
> whether it applies on -current and resubmit.
>
> On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
>> Hi everyone,
>>
>> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
>> currently, ndp(8) -f is not hooked in the code so no one is probably
>> using that.
>>
>> arp(8) -f is currently functional but I am not sure how useful.  If you
>> are using this option, please reply to this thread.
>>
>> Below you will find a patch that removes -f for both of these tools.
>>
>> Cheers,
>> Dimitris
>>

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Dimitris Papastamos  writes:

> Hi everyone,

Hi,

> I totally forgot about this patch.  At the time it couldn't go in
> because the tree was locked.  Does it make sense?  If so I will check
> whether it applies on -current and resubmit.

I don't understand the rationale.  Using -f sounds nicer than forcing
possible users to write yet another shell script.  The code is already
there, it is simple and short, so why remove it?

> On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
>> Hi everyone,
>> 
>> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
>> currently, ndp(8) -f is not hooked in the code so no one is probably
>> using that.

Well maybe it can be fixed instead.

>> arp(8) -f is currently functional but I am not sure how useful.  If you
>> are using this option, please reply to this thread.
>> 
>> Below you will find a patch that removes -f for both of these tools.
>> 
>> Cheers,
>> Dimitris
>> 
>> ===
>> RCS file: /cvs/src/usr.sbin/arp/arp.8,v
>> retrieving revision 1.36
>> diff -u -p -r1.36 arp.8
>> --- usr.sbin/arp/arp.8   27 Jul 2015 17:28:39 -  1.36
>> +++ usr.sbin/arp/arp.8   31 Jul 2015 12:51:29 -
>> @@ -43,7 +43,6 @@
>>  .Ar hostname
>>  .Nm arp
>>  .Op Fl F
>> -.Op Fl f Ar file
>>  .Op Fl V Ar rdomain
>>  .Fl s Ar hostname ether_addr
>>  .Op Cm temp | permanent
>> @@ -123,37 +122,6 @@ Force existing entries for the given hos
>>  and
>>  .Fl s
>>  options).
>> -.It Fl f Ar file
>> -Process entries from
>> -.Ar file
>> -to be set in the ARP tables.
>> -Any entries in the file that already exist for a given host
>> -will not be overwritten unless
>> -.Fl F
>> -is given.
>> -Entries in the file should be of the form:
>> -.Bd -filled -offset indent
>> -.Ar hostname ether_addr
>> -.Op Cm temp | permanent
>> -.Op Cm pub
>> -.Ed
>> -.Pp
>> -The entry will be static (will not time out) unless the word
>> -.Cm temp
>> -is given in the command.
>> -A static ARP entry can be overwritten by network traffic, unless the word
>> -.Cm permanent
>> -is given.
>> -If the word
>> -.Cm pub
>> -is given, the entry will be
>> -.Dq published ;
>> -that is, this system will act as an ARP server,
>> -responding to requests for
>> -.Ar hostname
>> -even though the host address is not its own.
>> -This behavior has traditionally been called
>> -.Em proxy ARP .
>>  .It Fl n
>>  Show network addresses as numbers (normally
>>  .Nm
>> Index: usr.sbin/arp/arp.c
>> ===
>> RCS file: /cvs/src/usr.sbin/arp/arp.c,v
>> retrieving revision 1.64
>> diff -u -p -r1.64 arp.c
>> --- usr.sbin/arp/arp.c   3 Jun 2015 08:10:53 -   1.64
>> +++ usr.sbin/arp/arp.c   31 Jul 2015 12:51:29 -
>> @@ -71,7 +71,6 @@ void nuke_entry(struct sockaddr_dl *sdl,
>>  struct sockaddr_inarp *sin, struct rt_msghdr *rtm);
>>  static char *ether_str(struct sockaddr_dl *);
>>  int wake(const char *ether_addr, const char *iface);
>> -int file(char *);
>>  int get(const char *);
>>  int getinetaddr(const char *, struct in_addr *);
>>  void getsocket(void);
>> @@ -97,9 +96,8 @@ extern int h_errno;
>>  /* which function we're supposed to do */
>>  #define F_GET   1
>>  #define F_SET   2
>> -#define F_FILESET   3
>> -#define F_DELETE4
>> -#define F_WAKE  5
>> +#define F_DELETE3
>> +#define F_WAKE  4
>>  
>>  int
>>  main(int argc, char *argv[])
>> @@ -131,11 +129,6 @@ main(int argc, char *argv[])
>>  case 'F':
>>  replace = 1;
>>  break;
>> -case 'f':
>> -if (func)
>> -usage();
>> -func = F_FILESET;
>> -break;
>>  case 'V':
>>  rdomain = strtonum(optarg, 0, RT_TABLEID_MAX, );
>>  if (errstr != NULL) {
>> @@ -184,11 +177,6 @@ main(int argc, char *argv[])
>>  else
>>  usage();
>>  break;
>> -case F_FILESET:
>> -if (argc != 1)
>> -usage();
>> -rtn = file(argv[0]);
>> -break;
>>  case F_WAKE:
>>  if (aflag || nflag || replace || rdomain > 0)
>>  usage();
>> @@ -203,41 +191,6 @@ main(int argc, char *argv[])
>>  return (rtn);
>>  }
>>  
>> -/*
>> - * Process a file to set standard arp entries
>> - */
>> -int
>> -file(char *name)
>> -{
>> -char line[100], arg[5][50], *args[5];
>> -int  i, retval;
>> -FILE*fp;
>> -
>> -if ((fp = fopen(name, "r")) == NULL)
>> -err(1, "cannot open %s", name);
>> -args[0] = [0][0];
>> -args[1] = [1][0];
>> -args[2] = [2][0];
>> -args[3] = [3][0];
>> -args[4] = [4][0];
>> -retval = 0;
>> -while (fgets(line, sizeof(line), fp) != NULL) {
>> -i = 

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Dimitris Papastamos]

Hi everyone,

I totally forgot about this patch.  At the time it couldn't go in
because the tree was locked.  Does it make sense?  If so I will check
whether it applies on -current and resubmit.

On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
> Hi everyone,
> 
> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
> currently, ndp(8) -f is not hooked in the code so no one is probably
> using that.
> 
> arp(8) -f is currently functional but I am not sure how useful.  If you
> are using this option, please reply to this thread.
> 
> Below you will find a patch that removes -f for both of these tools.
> 
> Cheers,
> Dimitris
> 
> ===
> RCS file: /cvs/src/usr.sbin/arp/arp.8,v
> retrieving revision 1.36
> diff -u -p -r1.36 arp.8
> --- usr.sbin/arp/arp.827 Jul 2015 17:28:39 -  1.36
> +++ usr.sbin/arp/arp.831 Jul 2015 12:51:29 -
> @@ -43,7 +43,6 @@
>  .Ar hostname
>  .Nm arp
>  .Op Fl F
> -.Op Fl f Ar file
>  .Op Fl V Ar rdomain
>  .Fl s Ar hostname ether_addr
>  .Op Cm temp | permanent
> @@ -123,37 +122,6 @@ Force existing entries for the given hos
>  and
>  .Fl s
>  options).
> -.It Fl f Ar file
> -Process entries from
> -.Ar file
> -to be set in the ARP tables.
> -Any entries in the file that already exist for a given host
> -will not be overwritten unless
> -.Fl F
> -is given.
> -Entries in the file should be of the form:
> -.Bd -filled -offset indent
> -.Ar hostname ether_addr
> -.Op Cm temp | permanent
> -.Op Cm pub
> -.Ed
> -.Pp
> -The entry will be static (will not time out) unless the word
> -.Cm temp
> -is given in the command.
> -A static ARP entry can be overwritten by network traffic, unless the word
> -.Cm permanent
> -is given.
> -If the word
> -.Cm pub
> -is given, the entry will be
> -.Dq published ;
> -that is, this system will act as an ARP server,
> -responding to requests for
> -.Ar hostname
> -even though the host address is not its own.
> -This behavior has traditionally been called
> -.Em proxy ARP .
>  .It Fl n
>  Show network addresses as numbers (normally
>  .Nm
> Index: usr.sbin/arp/arp.c
> ===
> RCS file: /cvs/src/usr.sbin/arp/arp.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 arp.c
> --- usr.sbin/arp/arp.c3 Jun 2015 08:10:53 -   1.64
> +++ usr.sbin/arp/arp.c31 Jul 2015 12:51:29 -
> @@ -71,7 +71,6 @@ void nuke_entry(struct sockaddr_dl *sdl,
>   struct sockaddr_inarp *sin, struct rt_msghdr *rtm);
>  static char *ether_str(struct sockaddr_dl *);
>  int wake(const char *ether_addr, const char *iface);
> -int file(char *);
>  int get(const char *);
>  int getinetaddr(const char *, struct in_addr *);
>  void getsocket(void);
> @@ -97,9 +96,8 @@ extern int h_errno;
>  /* which function we're supposed to do */
>  #define F_GET1
>  #define F_SET2
> -#define F_FILESET3
> -#define F_DELETE 4
> -#define F_WAKE   5
> +#define F_DELETE 3
> +#define F_WAKE   4
>  
>  int
>  main(int argc, char *argv[])
> @@ -131,11 +129,6 @@ main(int argc, char *argv[])
>   case 'F':
>   replace = 1;
>   break;
> - case 'f':
> - if (func)
> - usage();
> - func = F_FILESET;
> - break;
>   case 'V':
>   rdomain = strtonum(optarg, 0, RT_TABLEID_MAX, );
>   if (errstr != NULL) {
> @@ -184,11 +177,6 @@ main(int argc, char *argv[])
>   else
>   usage();
>   break;
> - case F_FILESET:
> - if (argc != 1)
> - usage();
> - rtn = file(argv[0]);
> - break;
>   case F_WAKE:
>   if (aflag || nflag || replace || rdomain > 0)
>   usage();
> @@ -203,41 +191,6 @@ main(int argc, char *argv[])
>   return (rtn);
>  }
>  
> -/*
> - * Process a file to set standard arp entries
> - */
> -int
> -file(char *name)
> -{
> - char line[100], arg[5][50], *args[5];
> - int  i, retval;
> - FILE*fp;
> -
> - if ((fp = fopen(name, "r")) == NULL)
> - err(1, "cannot open %s", name);
> - args[0] = [0][0];
> - args[1] = [1][0];
> - args[2] = [2][0];
> - args[3] = [3][0];
> - args[4] = [4][0];
> - retval = 0;
> - while (fgets(line, sizeof(line), fp) != NULL) {
> - i = sscanf(line, "%49s %49s %49s %49s %49s", arg[0], arg[1],
> - arg[2], arg[3], arg[4]);
> - if (i < 2) {
> - warnx("bad line: %s", line);
> - retval = 1;
> - continue;
> - }
> - if (replace)
> - delete(arg[0], NULL);
> - if (set(i, args))