Re: [PATCH] Enable -f in ndp(8)

[Sebastian Benoit]

There is no reason to remove this in arp.

As for ndp/ipv6, i'm not sure.
Is there anyone adding large numbers of ndp entries? Why?

/Benno


Dimitris Papastamos(s...@2f30.org) on 2015.07.25 21:11:41 +0100:
 On Sat, Jul 25, 2015 at 09:20:18PM +0200, Martin Pieuchot wrote:
  On 13/07/15(Mon) 14:04, Dimitris Papastamos wrote:
   Hi,
   
   I noticed -f in ndp(8) did nothing at all so I've enabled it and
   documented the file syntax in the man page.
  
  If it does nothing, I'd say let's kill it.
  
 
 Index: ndp.8
 ===
 RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
 retrieving revision 1.33
 diff -u -p -r1.33 ndp.8
 --- ndp.8 3 Sep 2014 10:39:41 -   1.33
 +++ ndp.8 25 Jul 2015 20:10:08 -
 @@ -44,7 +44,6 @@
  .Op Fl H | P | R
  .Op Fl A Ar wait
  .Op Fl d Ar hostname
 -.Op Fl f Ar filename
  .Op Fl i Ar interface Op Ar flag ...
  .Op Fl s Ar nodename etheraddr Oo Ic temp Oc Op Ic proxy
  .Op Fl V Ar rdomain
 @@ -119,9 +118,6 @@ the node has sent during the current sta
  Erase all the NDP entries.
  .It Fl d Ar hostname
  Delete the specified NDP entry.
 -.It Fl f Ar filename
 -Parse the file specified by
 -.Ar filename .
  .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.61
 diff -u -p -r1.61 ndp.c
 --- ndp.c 3 Jun 2015 08:10:53 -   1.61
 +++ ndp.c 25 Jul 2015 20:10:08 -
 @@ -123,7 +123,6 @@ char ntop_buf[INET6_ADDRSTRLEN];  /* inet
  char host_buf[NI_MAXHOST];   /* getnameinfo() */
  char ifix_buf[IFNAMSIZ]; /* if_indextoname() */
  
 -int file(char *);
  void getsocket(void);
  int set(int, char **);
  void get(char *);
 @@ -294,41 +293,6 @@ main(int argc, char *argv[])
   exit(0);
  }
  
 -/*
 - * Process a file to set standard ndp entries
 - */
 -int
 -file(char *name)
 -{
 - FILE *fp;
 - int i, retval;
 - char line[100], arg[5][50], *args[5];
 -
 - if ((fp = fopen(name, r)) == NULL) {
 - fprintf(stderr, ndp: cannot open %s\n, name);
 - exit(1);
 - }
 - args[0] = arg[0][0];
 - args[1] = arg[1][0];
 - args[2] = arg[2][0];
 - args[3] = arg[3][0];
 - args[4] = arg[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) {
 - fprintf(stderr, ndp: bad line: %s\n, line);
 - retval = 1;
 - continue;
 - }
 - if (set(i, args))
 - retval = 1;
 - }
 - fclose(fp);
 - return (retval);
 -}
 -
  void
  getsocket(void)
  {
 @@ -792,7 +756,7 @@ usage(void)
  {
   printf(usage: ndp [-nrt] [-a | -c | -p] [-H | -P | -R] );
   printf([-A wait] [-d hostname]\n);
 - printf(\t[-f filename] [-i interface [flag ...]]\n);
 + printf(\t[-i interface [flag ...]]\n);
   printf(\t[-s nodename etheraddr [temp] [proxy]] );
   printf([-V rdomain] [hostname]\n);
   exit(1);
 

-- 

Re: [PATCH] Enable -f in ndp(8)

[Martin Pieuchot]

On 02/08/15(Sun) 14:24, Sebastian Benoit wrote:
 There is no reason to remove this in arp.

Does that mean you use it?  If yes, could you take care of the first
diff?

Re: [PATCH] Enable -f in ndp(8)

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2015.08.02 16:20:15 +0200:
 On 02/08/15(Sun) 14:24, Sebastian Benoit wrote:
  There is no reason to remove this in arp.
 
 Does that mean you use it?  If yes, could you take care of the first
 diff?

i used arp -f in the past. i'm not sure where you need it in v6, but i
surely can commit this after unlock.

/Benno

Re: [PATCH] Enable -f in ndp(8)

[Mike Belopuhov]

On 2 August 2015 at 18:05, Sebastian Benoit be...@openbsd.org wrote:
 Martin Pieuchot(m...@openbsd.org) on 2015.08.02 16:20:15 +0200:
 On 02/08/15(Sun) 14:24, Sebastian Benoit wrote:
  There is no reason to remove this in arp.

 Does that mean you use it?  If yes, could you take care of the first
 diff?

 i used arp -f in the past. i'm not sure where you need it in v6, but i
 surely can commit this after unlock.

 /Benno


heh, 10 years ago i was using it as well (-:

revision 1.32
date: 2005/03/29 21:59:59;  author: henning;  state: Exp;  lines: +14 -6;
add -F to force replacement of entries with -s and -f
inspired by a diff from Mike Belopuhov m...@cvs.hnet.spb.ru,
these semantics with theo, manpage jaredy jmc and bob, ok bob

Re: [PATCH] Enable -f in ndp(8)

[Theo de Raadt]

 Martin Pieuchot(m...@openbsd.org) on 2015.08.02 16:20:15 +0200:
  On 02/08/15(Sun) 14:24, Sebastian Benoit wrote:
   There is no reason to remove this in arp.
  
  Does that mean you use it?  If yes, could you take care of the first
  diff?
 
 i used arp -f in the past. i'm not sure where you need it in v6, but i
 surely can commit this after unlock.

Exactly. Please defer both ndp -f and arp -f to a post-release discussion.

Re: [PATCH] Enable -f in ndp(8)

[Benny Lofgren]

On 2015-07-25 21:20, Martin Pieuchot wrote:
 On 13/07/15(Mon) 14:04, Dimitris Papastamos wrote:
 Hi,

 I noticed -f in ndp(8) did nothing at all so I've enabled it and
 documented the file syntax in the man page.
 
 If it does nothing, I'd say let's kill it.

Looking at the code, that looks more like an oversight to me.

There is a comment early in ndp.c that says ndp(8) is based on arp(8),
and the corresponding code in arp.c is similarly organized - with a
working -f switch - and both has near identical file() functions to do
the work.

So all the logic for the functionality is there, there's only one case
in the switch (mode) statement that's missing in order to activate it.

Also, the code has been like that since it was imported into the tree,
so if there is a reason it's missing it is to be found in somebody
else's repository. :-)


This is also a comment to Dmitris' later mail proposing an alternative
patch to instead remove -f in both arp and ndp, which I feel would be a
bit of a mistake.

I can't say I have used arp -f in recent memory, but I know for a fact
that I have used it on Solaris in ancient times to support strange
internet enabled devices like printer spoolers, intelligent power
switches and the like that acquires their IP address through rarp.


In other words, my recommendation is to accept this patch and discard
the latter one.


Regards,

/Benny

Re: [PATCH] Enable -f in ndp(8)

[Benny Lofgren]

On 2015-08-02 02:09, Benny Lofgren wrote:
 This is also a comment to Dmitris' later mail proposing an alternative

Apologies Dimitris, I managed to misspell your name.


Regards,
/Benny

Re: [PATCH] Enable -f in ndp(8)

[Martin Pieuchot]

On 13/07/15(Mon) 14:04, Dimitris Papastamos wrote:
 Hi,
 
 I noticed -f in ndp(8) did nothing at all so I've enabled it and
 documented the file syntax in the man page.

If it does nothing, I'd say let's kill it.

Re: [PATCH] Enable -f in ndp(8)

[Dimitris Papastamos]

On Sat, Jul 25, 2015 at 09:20:18PM +0200, Martin Pieuchot wrote:
 On 13/07/15(Mon) 14:04, Dimitris Papastamos wrote:
  Hi,
  
  I noticed -f in ndp(8) did nothing at all so I've enabled it and
  documented the file syntax in the man page.
 
 If it does nothing, I'd say let's kill it.
 

Index: ndp.8
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
retrieving revision 1.33
diff -u -p -r1.33 ndp.8
--- ndp.8   3 Sep 2014 10:39:41 -   1.33
+++ ndp.8   25 Jul 2015 20:10:08 -
@@ -44,7 +44,6 @@
 .Op Fl H | P | R
 .Op Fl A Ar wait
 .Op Fl d Ar hostname
-.Op Fl f Ar filename
 .Op Fl i Ar interface Op Ar flag ...
 .Op Fl s Ar nodename etheraddr Oo Ic temp Oc Op Ic proxy
 .Op Fl V Ar rdomain
@@ -119,9 +118,6 @@ the node has sent during the current sta
 Erase all the NDP entries.
 .It Fl d Ar hostname
 Delete the specified NDP entry.
-.It Fl f Ar filename
-Parse the file specified by
-.Ar filename .
 .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.61
diff -u -p -r1.61 ndp.c
--- ndp.c   3 Jun 2015 08:10:53 -   1.61
+++ ndp.c   25 Jul 2015 20:10:08 -
@@ -123,7 +123,6 @@ char ntop_buf[INET6_ADDRSTRLEN];/* inet
 char host_buf[NI_MAXHOST]; /* getnameinfo() */
 char ifix_buf[IFNAMSIZ];   /* if_indextoname() */
 
-int file(char *);
 void getsocket(void);
 int set(int, char **);
 void get(char *);
@@ -294,41 +293,6 @@ main(int argc, char *argv[])
exit(0);
 }
 
-/*
- * Process a file to set standard ndp entries
- */
-int
-file(char *name)
-{
-   FILE *fp;
-   int i, retval;
-   char line[100], arg[5][50], *args[5];
-
-   if ((fp = fopen(name, r)) == NULL) {
-   fprintf(stderr, ndp: cannot open %s\n, name);
-   exit(1);
-   }
-   args[0] = arg[0][0];
-   args[1] = arg[1][0];
-   args[2] = arg[2][0];
-   args[3] = arg[3][0];
-   args[4] = arg[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) {
-   fprintf(stderr, ndp: bad line: %s\n, line);
-   retval = 1;
-   continue;
-   }
-   if (set(i, args))
-   retval = 1;
-   }
-   fclose(fp);
-   return (retval);
-}
-
 void
 getsocket(void)
 {
@@ -792,7 +756,7 @@ usage(void)
 {
printf(usage: ndp [-nrt] [-a | -c | -p] [-H | -P | -R] );
printf([-A wait] [-d hostname]\n);
-   printf(\t[-f filename] [-i interface [flag ...]]\n);
+   printf(\t[-i interface [flag ...]]\n);
printf(\t[-s nodename etheraddr [temp] [proxy]] );
printf([-V rdomain] [hostname]\n);
exit(1);

Re: [PATCH] Enable -f in ndp(8)

[Dimitris Papastamos]

On Mon, Jul 13, 2015 at 02:04:40PM +0100, Dimitris Papastamos wrote:
 Hi,
 
 I noticed -f in ndp(8) did nothing at all so I've enabled it and
 documented the file syntax in the man page.
 
 ===
 RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
 retrieving revision 1.33
 diff -u -p -r1.33 ndp.8
 --- ndp.8 3 Sep 2014 10:39:41 -   1.33
 +++ ndp.8 13 Jul 2015 13:02:49 -
 @@ -122,6 +122,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.61
 diff -u -p -r1.61 ndp.c
 --- ndp.c 3 Jun 2015 08:10:53 -   1.61
 +++ ndp.c 13 Jul 2015 13:02:49 -
 @@ -241,6 +241,8 @@ main(int argc, char *argv[])
   }
   delete(arg);
   break;
 + case 'f':
 + exit(file(arg) ? 1 : 0);
   case 'p':
   if (argc != 0) {
   usage();
 

Is this ok?  Looking at it, it should just be exit(file(arg)) as file()
only returns 0 for success and 1 on failure.

[PATCH] Enable -f in ndp(8)

[Dimitris Papastamos]

Hi,

I noticed -f in ndp(8) did nothing at all so I've enabled it and
documented the file syntax in the man page.

===
RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
retrieving revision 1.33
diff -u -p -r1.33 ndp.8
--- ndp.8   3 Sep 2014 10:39:41 -   1.33
+++ ndp.8   13 Jul 2015 13:02:49 -
@@ -122,6 +122,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.61
diff -u -p -r1.61 ndp.c
--- ndp.c   3 Jun 2015 08:10:53 -   1.61
+++ ndp.c   13 Jul 2015 13:02:49 -
@@ -241,6 +241,8 @@ main(int argc, char *argv[])
}
delete(arg);
break;
+   case 'f':
+   exit(file(arg) ? 1 : 0);
case 'p':
if (argc != 0) {
usage();