ospfd: allow specifying area by number as well as id

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

ospfd: allow specifying area by number as well as id

David Gwynne-5
it's always bothered me that i config areas on a crisco using a number,
but then have to think hard to convert that number to an address for use
in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
as an address. super annoying.

so this changes the ospfd parser so it accepts both a number or address.
i also changed it so it prints the number by default, which may be
contentious. the manpage is slightly tweaked too.

thoughts?

with this diff, i can do the following and things keep
working:

--- /etc/ospfd.conf Mon Apr 29 11:29:56 2019
+++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019
@@ -7,5 +7,5 @@
 redistribute rtlabel "backup" set metric 65535
 
-area 0.0.2.188 {
+area 700 {
  router-dead-time minimal
  fast-hello-interval msec 300

Index: ospfd.conf.5
===================================================================
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v
retrieving revision 1.55
diff -u -p -r1.55 ospfd.conf.5
--- ospfd.conf.5 28 Dec 2018 19:25:10 -0000 1.55
+++ ospfd.conf.5 29 Apr 2019 01:45:40 -0000
@@ -68,7 +68,7 @@ Macros are not expanded inside quotes.
 For example:
 .Bd -literal -offset indent
 hi="5"
-area 0.0.0.0 {
+area 0 {
  interface em0 {
  hello-interval $hi
  }
@@ -257,10 +257,10 @@ Areas are used for grouping interfaces.
 All interface-specific parameters can
 be configured per area, overruling the global settings.
 .Bl -tag -width Ds
-.It Ic area Ar address
+.It Ic area Ar id Ns | Ns Ar address
 Specify an area section, grouping one or more interfaces.
 .Bd -literal -offset indent
-area 0.0.0.0 {
+area 0 {
  interface em0
  interface em1 {
  metric 10
Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.95
diff -u -p -r1.95 parse.y
--- parse.y 13 Feb 2019 22:57:08 -0000 1.95
+++ parse.y 29 Apr 2019 01:45:40 -0000
@@ -120,6 +120,7 @@ typedef struct {
  int64_t number;
  char *string;
  struct redistribute *redist;
+ struct in_addr id;
  } v;
  int lineno;
 } YYSTYPE;
@@ -145,6 +146,7 @@ typedef struct {
 %type <v.number> deadtime
 %type <v.string> string dependon
 %type <v.redist> redistribute
+%type <v.id> areaid
 
 %%
 
@@ -588,15 +590,8 @@ comma : ','
  | /*empty*/
  ;
 
-area : AREA STRING {
- struct in_addr id;
- if (inet_aton($2, &id) == 0) {
- yyerror("error parsing area");
- free($2);
- YYERROR;
- }
- free($2);
- area = conf_get_area(id);
+area : AREA areaid {
+ area = conf_get_area($2);
 
  memcpy(&areadefs, defs, sizeof(areadefs));
  md_list_copy(&areadefs.md_list, &defs->md_list);
@@ -610,6 +605,23 @@ area : AREA STRING {
 
 demotecount : NUMBER { $$ = $1; }
  | /*empty*/ { $$ = 1; }
+ ;
+
+areaid : NUMBER {
+ if ($1 < 0 || $1 > 0xffffffff) {
+ yyerror("invalid area id");
+ YYERROR;
+ }
+ $$.s_addr = htonl($1);
+ }
+ | STRING {
+ if (inet_aton($1, &$$) == 0) {
+ yyerror("error parsing area");
+ free($1);
+ YYERROR;
+ }
+ free($1);
+ }
  ;
 
 areaopts_l : areaopts_l areaoptsl nl
Index: printconf.c
===================================================================
RCS file: /cvs/src/usr.sbin/ospfd/printconf.c,v
retrieving revision 1.20
diff -u -p -r1.20 printconf.c
--- printconf.c 28 Dec 2018 19:25:10 -0000 1.20
+++ printconf.c 29 Apr 2019 01:45:40 -0000
@@ -181,7 +181,8 @@ print_config(struct ospfd_conf *conf)
  printf("\n");
 
  LIST_FOREACH(area, &conf->area_list, entry) {
- printf("area %s {\n", inet_ntoa(area->id));
+ printf("area %u { # %s\n", ntohl(area->id.s_addr),
+    inet_ntoa(area->id));
  if (area->stub) {
  printf("\tstub");
  if (SIMPLEQ_EMPTY(&area->redist_list))

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Remi Locherer
Hi David

On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:

> it's always bothered me that i config areas on a crisco using a number,
> but then have to think hard to convert that number to an address for use
> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> as an address. super annoying.
>
> so this changes the ospfd parser so it accepts both a number or address.
> i also changed it so it prints the number by default, which may be
> contentious. the manpage is slightly tweaked too.
>
> thoughts?

I like it to be able to use a number instead of an address!

It worked fine in my short test I performed.

The output with the comment looks a bit strange to me.

typhoon ..sbin/ospfd$ doas obj/ospfd -nv

router-id 0.0.0.7
fib-update yes
fib-priority 32
rfc1583compat no
spf-delay msec 1000
spf-holdtime msec 5000

area 7 { # 0.0.0.7
         ^^^^^^^^^
        interface pair7:10.77.77.1 {
                metric 10
                retransmit-interval 5
                router-dead-time 40


I'd prefer if we settle for one output format and then use only that. The
number format is more common but that would be a change for the users. I'm
fine with either format for outputs.

There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-)

Regards,
Remi


>
> with this diff, i can do the following and things keep
> working:
>
> --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019
> +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019
> @@ -7,5 +7,5 @@
>  redistribute rtlabel "backup" set metric 65535
>  
> -area 0.0.2.188 {
> +area 700 {
>   router-dead-time minimal
>   fast-hello-interval msec 300
>
> Index: ospfd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v
> retrieving revision 1.55
> diff -u -p -r1.55 ospfd.conf.5
> --- ospfd.conf.5 28 Dec 2018 19:25:10 -0000 1.55
> +++ ospfd.conf.5 29 Apr 2019 01:45:40 -0000
> @@ -68,7 +68,7 @@ Macros are not expanded inside quotes.
>  For example:
>  .Bd -literal -offset indent
>  hi="5"
> -area 0.0.0.0 {
> +area 0 {
>   interface em0 {
>   hello-interval $hi
>   }
> @@ -257,10 +257,10 @@ Areas are used for grouping interfaces.
>  All interface-specific parameters can
>  be configured per area, overruling the global settings.
>  .Bl -tag -width Ds
> -.It Ic area Ar address
> +.It Ic area Ar id Ns | Ns Ar address
>  Specify an area section, grouping one or more interfaces.
>  .Bd -literal -offset indent
> -area 0.0.0.0 {
> +area 0 {
>   interface em0
>   interface em1 {
>   metric 10
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
> retrieving revision 1.95
> diff -u -p -r1.95 parse.y
> --- parse.y 13 Feb 2019 22:57:08 -0000 1.95
> +++ parse.y 29 Apr 2019 01:45:40 -0000
> @@ -120,6 +120,7 @@ typedef struct {
>   int64_t number;
>   char *string;
>   struct redistribute *redist;
> + struct in_addr id;
>   } v;
>   int lineno;
>  } YYSTYPE;
> @@ -145,6 +146,7 @@ typedef struct {
>  %type <v.number> deadtime
>  %type <v.string> string dependon
>  %type <v.redist> redistribute
> +%type <v.id> areaid
>  
>  %%
>  
> @@ -588,15 +590,8 @@ comma : ','
>   | /*empty*/
>   ;
>  
> -area : AREA STRING {
> - struct in_addr id;
> - if (inet_aton($2, &id) == 0) {
> - yyerror("error parsing area");
> - free($2);
> - YYERROR;
> - }
> - free($2);
> - area = conf_get_area(id);
> +area : AREA areaid {
> + area = conf_get_area($2);
>  
>   memcpy(&areadefs, defs, sizeof(areadefs));
>   md_list_copy(&areadefs.md_list, &defs->md_list);
> @@ -610,6 +605,23 @@ area : AREA STRING {
>  
>  demotecount : NUMBER { $$ = $1; }
>   | /*empty*/ { $$ = 1; }
> + ;
> +
> +areaid : NUMBER {
> + if ($1 < 0 || $1 > 0xffffffff) {
> + yyerror("invalid area id");
> + YYERROR;
> + }
> + $$.s_addr = htonl($1);
> + }
> + | STRING {
> + if (inet_aton($1, &$$) == 0) {
> + yyerror("error parsing area");
> + free($1);
> + YYERROR;
> + }
> + free($1);
> + }
>   ;
>  
>  areaopts_l : areaopts_l areaoptsl nl
> Index: printconf.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/printconf.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 printconf.c
> --- printconf.c 28 Dec 2018 19:25:10 -0000 1.20
> +++ printconf.c 29 Apr 2019 01:45:40 -0000
> @@ -181,7 +181,8 @@ print_config(struct ospfd_conf *conf)
>   printf("\n");
>  
>   LIST_FOREACH(area, &conf->area_list, entry) {
> - printf("area %s {\n", inet_ntoa(area->id));
> + printf("area %u { # %s\n", ntohl(area->id.s_addr),
> +    inet_ntoa(area->id));
>   if (area->stub) {
>   printf("\tstub");
>   if (SIMPLEQ_EMPTY(&area->redist_list))
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

David Gwynne-5


> On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
>
> Hi David
>
> On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
>> it's always bothered me that i config areas on a crisco using a number,
>> but then have to think hard to convert that number to an address for use
>> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
>> as an address. super annoying.
>>
>> so this changes the ospfd parser so it accepts both a number or address.
>> i also changed it so it prints the number by default, which may be
>> contentious. the manpage is slightly tweaked too.
>>
>> thoughts?
>
> I like it to be able to use a number instead of an address!
>
> It worked fine in my short test I performed.
>
> The output with the comment looks a bit strange to me.

Are you sure it doesn't look... awesome?

> typhoon ..sbin/ospfd$ doas obj/ospfd -nv
>
> router-id 0.0.0.7
> fib-update yes
> fib-priority 32
> rfc1583compat no
> spf-delay msec 1000
> spf-holdtime msec 5000
>
> area 7 { # 0.0.0.7
>         ^^^^^^^^^
>        interface pair7:10.77.77.1 {
>                metric 10
>                retransmit-interval 5
>                router-dead-time 40
>
>
> I'd prefer if we settle for one output format and then use only that. The
> number format is more common but that would be a change for the users. I'm
> fine with either format for outputs.

I lean toward the number too. I don't think it would hurt to change it so only one is output, so long input works either way.

> There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-)

Are you offering to help with the implementation of those?

dlg

>
> Regards,
> Remi
>
>
>>
>> with this diff, i can do the following and things keep
>> working:
>>
>> --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019
>> +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019
>> @@ -7,5 +7,5 @@
>> redistribute rtlabel "backup" set metric 65535
>>
>> -area 0.0.2.188 {
>> +area 700 {
>> router-dead-time minimal
>> fast-hello-interval msec 300
>>
>> Index: ospfd.conf.5
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v
>> retrieving revision 1.55
>> diff -u -p -r1.55 ospfd.conf.5
>> --- ospfd.conf.5 28 Dec 2018 19:25:10 -0000 1.55
>> +++ ospfd.conf.5 29 Apr 2019 01:45:40 -0000
>> @@ -68,7 +68,7 @@ Macros are not expanded inside quotes.
>> For example:
>> .Bd -literal -offset indent
>> hi="5"
>> -area 0.0.0.0 {
>> +area 0 {
>> interface em0 {
>> hello-interval $hi
>> }
>> @@ -257,10 +257,10 @@ Areas are used for grouping interfaces.
>> All interface-specific parameters can
>> be configured per area, overruling the global settings.
>> .Bl -tag -width Ds
>> -.It Ic area Ar address
>> +.It Ic area Ar id Ns | Ns Ar address
>> Specify an area section, grouping one or more interfaces.
>> .Bd -literal -offset indent
>> -area 0.0.0.0 {
>> +area 0 {
>> interface em0
>> interface em1 {
>> metric 10
>> Index: parse.y
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
>> retrieving revision 1.95
>> diff -u -p -r1.95 parse.y
>> --- parse.y 13 Feb 2019 22:57:08 -0000 1.95
>> +++ parse.y 29 Apr 2019 01:45:40 -0000
>> @@ -120,6 +120,7 @@ typedef struct {
>> int64_t number;
>> char *string;
>> struct redistribute *redist;
>> + struct in_addr id;
>> } v;
>> int lineno;
>> } YYSTYPE;
>> @@ -145,6 +146,7 @@ typedef struct {
>> %type <v.number> deadtime
>> %type <v.string> string dependon
>> %type <v.redist> redistribute
>> +%type <v.id> areaid
>>
>> %%
>>
>> @@ -588,15 +590,8 @@ comma : ','
>> | /*empty*/
>> ;
>>
>> -area : AREA STRING {
>> - struct in_addr id;
>> - if (inet_aton($2, &id) == 0) {
>> - yyerror("error parsing area");
>> - free($2);
>> - YYERROR;
>> - }
>> - free($2);
>> - area = conf_get_area(id);
>> +area : AREA areaid {
>> + area = conf_get_area($2);
>>
>> memcpy(&areadefs, defs, sizeof(areadefs));
>> md_list_copy(&areadefs.md_list, &defs->md_list);
>> @@ -610,6 +605,23 @@ area : AREA STRING {
>>
>> demotecount : NUMBER { $$ = $1; }
>> | /*empty*/ { $$ = 1; }
>> + ;
>> +
>> +areaid : NUMBER {
>> + if ($1 < 0 || $1 > 0xffffffff) {
>> + yyerror("invalid area id");
>> + YYERROR;
>> + }
>> + $$.s_addr = htonl($1);
>> + }
>> + | STRING {
>> + if (inet_aton($1, &$$) == 0) {
>> + yyerror("error parsing area");
>> + free($1);
>> + YYERROR;
>> + }
>> + free($1);
>> + }
>> ;
>>
>> areaopts_l : areaopts_l areaoptsl nl
>> Index: printconf.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/ospfd/printconf.c,v
>> retrieving revision 1.20
>> diff -u -p -r1.20 printconf.c
>> --- printconf.c 28 Dec 2018 19:25:10 -0000 1.20
>> +++ printconf.c 29 Apr 2019 01:45:40 -0000
>> @@ -181,7 +181,8 @@ print_config(struct ospfd_conf *conf)
>> printf("\n");
>>
>> LIST_FOREACH(area, &conf->area_list, entry) {
>> - printf("area %s {\n", inet_ntoa(area->id));
>> + printf("area %u { # %s\n", ntohl(area->id.s_addr),
>> +    inet_ntoa(area->id));
>> if (area->stub) {
>> printf("\tstub");
>> if (SIMPLEQ_EMPTY(&area->redist_list))
>>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Sebastian Benoit-3
David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:

>
>
> > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> >
> > Hi David
> >
> > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> >> it's always bothered me that i config areas on a crisco using a number,
> >> but then have to think hard to convert that number to an address for use
> >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> >> as an address. super annoying.
> >>
> >> so this changes the ospfd parser so it accepts both a number or address.
> >> i also changed it so it prints the number by default, which may be
> >> contentious. the manpage is slightly tweaked too.
> >>
> >> thoughts?
> >
> > I like it to be able to use a number instead of an address!
> >
> > It worked fine in my short test I performed.
> >
> > The output with the comment looks a bit strange to me.
>
> Are you sure it doesn't look... awesome?

I like it!

>
> > typhoon ..sbin/ospfd$ doas obj/ospfd -nv
> >
> > router-id 0.0.0.7
> > fib-update yes
> > fib-priority 32
> > rfc1583compat no
> > spf-delay msec 1000
> > spf-holdtime msec 5000
> >
> > area 7 { # 0.0.0.7
> >         ^^^^^^^^^
> >        interface pair7:10.77.77.1 {
> >                metric 10
> >                retransmit-interval 5
> >                router-dead-time 40
> >
> >
> > I'd prefer if we settle for one output format and then use only that. The
> > number format is more common but that would be a change for the users. I'm
> > fine with either format for outputs.
>
> I lean toward the number too. I don't think it would hurt to change it so
> only one is output, so long input works either way.

We need a way to show both: to make migration easier, and to avoid the same
problem you encountered when entering the area in some other equipment.

I dont care if thats in the config printer or in ospfctl output though.

>
> > There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-)
>
> Are you offering to help with the implementation of those?
>
> dlg
>
> >
> > Regards,
> > Remi
> >
> >
> >>
> >> with this diff, i can do the following and things keep
> >> working:
> >>
> >> --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019
> >> +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019
> >> @@ -7,5 +7,5 @@
> >> redistribute rtlabel "backup" set metric 65535
> >>
> >> -area 0.0.2.188 {
> >> +area 700 {
> >> router-dead-time minimal
> >> fast-hello-interval msec 300
> >>
> >> Index: ospfd.conf.5
> >> ===================================================================
> >> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v
> >> retrieving revision 1.55
> >> diff -u -p -r1.55 ospfd.conf.5
> >> --- ospfd.conf.5 28 Dec 2018 19:25:10 -0000 1.55
> >> +++ ospfd.conf.5 29 Apr 2019 01:45:40 -0000
> >> @@ -68,7 +68,7 @@ Macros are not expanded inside quotes.
> >> For example:
> >> .Bd -literal -offset indent
> >> hi="5"
> >> -area 0.0.0.0 {
> >> +area 0 {
> >> interface em0 {
> >> hello-interval $hi
> >> }
> >> @@ -257,10 +257,10 @@ Areas are used for grouping interfaces.
> >> All interface-specific parameters can
> >> be configured per area, overruling the global settings.
> >> .Bl -tag -width Ds
> >> -.It Ic area Ar address
> >> +.It Ic area Ar id Ns | Ns Ar address
> >> Specify an area section, grouping one or more interfaces.
> >> .Bd -literal -offset indent
> >> -area 0.0.0.0 {
> >> +area 0 {
> >> interface em0
> >> interface em1 {
> >> metric 10
> >> Index: parse.y
> >> ===================================================================
> >> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
> >> retrieving revision 1.95
> >> diff -u -p -r1.95 parse.y
> >> --- parse.y 13 Feb 2019 22:57:08 -0000 1.95
> >> +++ parse.y 29 Apr 2019 01:45:40 -0000
> >> @@ -120,6 +120,7 @@ typedef struct {
> >> int64_t number;
> >> char *string;
> >> struct redistribute *redist;
> >> + struct in_addr id;
> >> } v;
> >> int lineno;
> >> } YYSTYPE;
> >> @@ -145,6 +146,7 @@ typedef struct {
> >> %type <v.number> deadtime
> >> %type <v.string> string dependon
> >> %type <v.redist> redistribute
> >> +%type <v.id> areaid
> >>
> >> %%
> >>
> >> @@ -588,15 +590,8 @@ comma : ','
> >> | /*empty*/
> >> ;
> >>
> >> -area : AREA STRING {
> >> - struct in_addr id;
> >> - if (inet_aton($2, &id) == 0) {
> >> - yyerror("error parsing area");
> >> - free($2);
> >> - YYERROR;
> >> - }
> >> - free($2);
> >> - area = conf_get_area(id);
> >> +area : AREA areaid {
> >> + area = conf_get_area($2);
> >>
> >> memcpy(&areadefs, defs, sizeof(areadefs));
> >> md_list_copy(&areadefs.md_list, &defs->md_list);
> >> @@ -610,6 +605,23 @@ area : AREA STRING {
> >>
> >> demotecount : NUMBER { $$ = $1; }
> >> | /*empty*/ { $$ = 1; }
> >> + ;
> >> +
> >> +areaid : NUMBER {
> >> + if ($1 < 0 || $1 > 0xffffffff) {
> >> + yyerror("invalid area id");
> >> + YYERROR;
> >> + }
> >> + $$.s_addr = htonl($1);
> >> + }
> >> + | STRING {
> >> + if (inet_aton($1, &$$) == 0) {
> >> + yyerror("error parsing area");
> >> + free($1);
> >> + YYERROR;
> >> + }
> >> + free($1);
> >> + }
> >> ;
> >>
> >> areaopts_l : areaopts_l areaoptsl nl
> >> Index: printconf.c
> >> ===================================================================
> >> RCS file: /cvs/src/usr.sbin/ospfd/printconf.c,v
> >> retrieving revision 1.20
> >> diff -u -p -r1.20 printconf.c
> >> --- printconf.c 28 Dec 2018 19:25:10 -0000 1.20
> >> +++ printconf.c 29 Apr 2019 01:45:40 -0000
> >> @@ -181,7 +181,8 @@ print_config(struct ospfd_conf *conf)
> >> printf("\n");
> >>
> >> LIST_FOREACH(area, &conf->area_list, entry) {
> >> - printf("area %s {\n", inet_ntoa(area->id));
> >> + printf("area %u { # %s\n", ntohl(area->id.s_addr),
> >> +    inet_ntoa(area->id));
> >> if (area->stub) {
> >> printf("\tstub");
> >> if (SIMPLEQ_EMPTY(&area->redist_list))
> >>
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Stuart Henderson
On 2019/04/29 11:58, Sebastian Benoit wrote:

> David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> >
> >
> > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > >
> > > Hi David
> > >
> > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > >> it's always bothered me that i config areas on a crisco using a number,
> > >> but then have to think hard to convert that number to an address for use
> > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > >> as an address. super annoying.
> > >>
> > >> so this changes the ospfd parser so it accepts both a number or address.
> > >> i also changed it so it prints the number by default, which may be
> > >> contentious. the manpage is slightly tweaked too.
> > >>
> > >> thoughts?
> > >
> > > I like it to be able to use a number instead of an address!
> > >
> > > It worked fine in my short test I performed.
> > >
> > > The output with the comment looks a bit strange to me.
> >
> > Are you sure it doesn't look... awesome?
>
> I like it!

I don't really, but if we change this it needs to be displayed somehow
and I don't have an idea to make it look nicer than this (cisco's method
seems pretty horrible and wouldn't work for us anyway - looks like they
remember which format was used to configure an area and use that as
the output format...)

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Alexander Bluhm
In reply to this post by David Gwynne-5
On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> it's always bothered me that i config areas on a crisco using a number,
> but then have to think hard to convert that number to an address for use
> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> as an address. super annoying.

I don't use Cisco and I don't use numbers.  Fine to me if you make
ospfd.conf more flexible.  The only thing about notation I found
in RFC 2328 is:

        The OSPF backbone is the special OSPF Area 0 (often written as
        Area 0.0.0.0, since OSPF Area ID's are typically formatted as IP
        addresses).

> so this changes the ospfd parser so it accepts both a number or address.
> i also changed it so it prints the number by default, which may be
> contentious. the manpage is slightly tweaked too.

Please keep it as it is.  ospfctl show database uses the dot notation
everywhere.  It would break my scripts if you change that.

bluhm

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Gregory Edigarov-5
In reply to this post by David Gwynne-5

On 29.04.19 04:53, David Gwynne wrote:

> it's always bothered me that i config areas on a crisco using a number,
> but then have to think hard to convert that number to an address for use
> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> as an address. super annoying.
>
> so this changes the ospfd parser so it accepts both a number or address.
> i also changed it so it prints the number by default, which may be
> contentious. the manpage is slightly tweaked too.
>
> thoughts?
>
why don't just add something like 'convert' to ospfctl? wouldn't it be
easier?

$ ospfctl convert 700

0.0.2.188

$ ospfctl convert 0.0.2.188
700

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Remi Locherer
In reply to this post by Stuart Henderson
On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:

> On 2019/04/29 11:58, Sebastian Benoit wrote:
> > David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> > >
> > >
> > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > > >
> > > > Hi David
> > > >
> > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > >> it's always bothered me that i config areas on a crisco using a number,
> > > >> but then have to think hard to convert that number to an address for use
> > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > >> as an address. super annoying.
> > > >>
> > > >> so this changes the ospfd parser so it accepts both a number or address.
> > > >> i also changed it so it prints the number by default, which may be
> > > >> contentious. the manpage is slightly tweaked too.
> > > >>
> > > >> thoughts?
> > > >
> > > > I like it to be able to use a number instead of an address!
> > > >
> > > > It worked fine in my short test I performed.
> > > >
> > > > The output with the comment looks a bit strange to me.
> > >
> > > Are you sure it doesn't look... awesome?
> >
> > I like it!
>
> I don't really, but if we change this it needs to be displayed somehow
> and I don't have an idea to make it look nicer than this (cisco's method
> seems pretty horrible and wouldn't work for us anyway - looks like they
> remember which format was used to configure an area and use that as
> the output format...)
>

Maybe it's better when we just allow both input formats but don't change
any output.

Below diff changes ospfctl to accept the address and number format for
"ospfct show database area XXX".


Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
retrieving revision 1.20
diff -u -p -r1.20 parser.c
--- parser.c 9 May 2011 12:25:35 -0000 1.20
+++ parser.c 30 Apr 2019 20:28:18 -0000
@@ -39,7 +39,8 @@ enum token_type {
  ADDRESS,
  FLAG,
  PREFIX,
- IFNAME
+ IFNAME,
+ AREA
 };
 
 struct token {
@@ -107,7 +108,7 @@ static const struct token t_show_db[] =
 };
 
 static const struct token t_show_area[] = {
- {ADDRESS, "", NONE, NULL},
+ {AREA, "", NONE, NULL},
  {ENDTOKEN, "", NONE, NULL}
 };
 
@@ -218,6 +219,14 @@ match_token(const char *word, const stru
  res->action = t->value;
  }
  break;
+ case AREA:
+ if (parse_area(word, &res->addr)) {
+ match++;
+ t = &table[i];
+ if (t->value)
+ res->action = t->value;
+ }
+ break;
  case PREFIX:
  if (parse_prefix(word, &res->addr, &res->prefixlen)) {
  match++;
@@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
  case ADDRESS:
  fprintf(stderr, "  <address>\n");
  break;
+ case AREA:
+ fprintf(stderr, "  <area>\n");
+ break;
  case PREFIX:
  fprintf(stderr, "  <address>[/<len>]\n");
  break;
@@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
  bzero(&ina, sizeof(ina));
 
  if (inet_pton(AF_INET, word, &ina)) {
+ addr->s_addr = ina.s_addr;
+ return (1);
+ }
+
+ return (0);
+}
+
+int
+parse_area(const char *word, struct in_addr *addr)
+{
+ struct in_addr ina;
+ const char *errstr;
+
+ if (word == NULL)
+ return (0);
+
+ bzero(addr, sizeof(struct in_addr));
+ bzero(&ina, sizeof(ina));
+
+ if (inet_pton(AF_INET, word, &ina)) {
+ addr->s_addr = ina.s_addr;
+ return (1);
+ }
+
+ ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
+ if (errstr == NULL) {
  addr->s_addr = ina.s_addr;
  return (1);
  }
Index: parser.h
===================================================================
RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
retrieving revision 1.13
diff -u -p -r1.13 parser.h
--- parser.h 9 May 2011 12:25:35 -0000 1.13
+++ parser.h 30 Apr 2019 20:28:52 -0000
@@ -64,6 +64,7 @@ struct parse_result {
 
 struct parse_result *parse(int, char *[]);
 int parse_addr(const char *, struct in_addr *);
+int parse_area(const char *, struct in_addr *);
 int parse_prefix(const char *, struct in_addr *,
      u_int8_t *);
 

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Remi Locherer
On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:

> On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
> > On 2019/04/29 11:58, Sebastian Benoit wrote:
> > > David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> > > >
> > > >
> > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > > > >
> > > > > Hi David
> > > > >
> > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > > >> it's always bothered me that i config areas on a crisco using a number,
> > > > >> but then have to think hard to convert that number to an address for use
> > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > > >> as an address. super annoying.
> > > > >>
> > > > >> so this changes the ospfd parser so it accepts both a number or address.
> > > > >> i also changed it so it prints the number by default, which may be
> > > > >> contentious. the manpage is slightly tweaked too.
> > > > >>
> > > > >> thoughts?
> > > > >
> > > > > I like it to be able to use a number instead of an address!
> > > > >
> > > > > It worked fine in my short test I performed.
> > > > >
> > > > > The output with the comment looks a bit strange to me.
> > > >
> > > > Are you sure it doesn't look... awesome?
> > >
> > > I like it!
> >
> > I don't really, but if we change this it needs to be displayed somehow
> > and I don't have an idea to make it look nicer than this (cisco's method
> > seems pretty horrible and wouldn't work for us anyway - looks like they
> > remember which format was used to configure an area and use that as
> > the output format...)
> >
>
> Maybe it's better when we just allow both input formats but don't change
> any output.

Any opinions or comments on this? I think this would be a valuable addition
to ospfd.

>
> Below diff changes ospfctl to accept the address and number format for
> "ospfct show database area XXX".
>
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 parser.c
> --- parser.c 9 May 2011 12:25:35 -0000 1.20
> +++ parser.c 30 Apr 2019 20:28:18 -0000
> @@ -39,7 +39,8 @@ enum token_type {
>   ADDRESS,
>   FLAG,
>   PREFIX,
> - IFNAME
> + IFNAME,
> + AREA
>  };
>  
>  struct token {
> @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
>  };
>  
>  static const struct token t_show_area[] = {
> - {ADDRESS, "", NONE, NULL},
> + {AREA, "", NONE, NULL},
>   {ENDTOKEN, "", NONE, NULL}
>  };
>  
> @@ -218,6 +219,14 @@ match_token(const char *word, const stru
>   res->action = t->value;
>   }
>   break;
> + case AREA:
> + if (parse_area(word, &res->addr)) {
> + match++;
> + t = &table[i];
> + if (t->value)
> + res->action = t->value;
> + }
> + break;
>   case PREFIX:
>   if (parse_prefix(word, &res->addr, &res->prefixlen)) {
>   match++;
> @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
>   case ADDRESS:
>   fprintf(stderr, "  <address>\n");
>   break;
> + case AREA:
> + fprintf(stderr, "  <area>\n");
> + break;
>   case PREFIX:
>   fprintf(stderr, "  <address>[/<len>]\n");
>   break;
> @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
>   bzero(&ina, sizeof(ina));
>  
>   if (inet_pton(AF_INET, word, &ina)) {
> + addr->s_addr = ina.s_addr;
> + return (1);
> + }
> +
> + return (0);
> +}
> +
> +int
> +parse_area(const char *word, struct in_addr *addr)
> +{
> + struct in_addr ina;
> + const char *errstr;
> +
> + if (word == NULL)
> + return (0);
> +
> + bzero(addr, sizeof(struct in_addr));
> + bzero(&ina, sizeof(ina));
> +
> + if (inet_pton(AF_INET, word, &ina)) {
> + addr->s_addr = ina.s_addr;
> + return (1);
> + }
> +
> + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
> + if (errstr == NULL) {
>   addr->s_addr = ina.s_addr;
>   return (1);
>   }
> Index: parser.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 parser.h
> --- parser.h 9 May 2011 12:25:35 -0000 1.13
> +++ parser.h 30 Apr 2019 20:28:52 -0000
> @@ -64,6 +64,7 @@ struct parse_result {
>  
>  struct parse_result *parse(int, char *[]);
>  int parse_addr(const char *, struct in_addr *);
> +int parse_area(const char *, struct in_addr *);
>  int parse_prefix(const char *, struct in_addr *,
>       u_int8_t *);
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Denis Fondras-3
On Wed, May 15, 2019 at 11:15:03PM +0200, Remi Locherer wrote:
> Any opinions or comments on this? I think this would be a valuable addition
> to ospfd.
>

I can't see any harm in it.

OK denis@

> >
> > Below diff changes ospfctl to accept the address and number format for
> > "ospfct show database area XXX".
> >
> >
> > Index: parser.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 parser.c
> > --- parser.c 9 May 2011 12:25:35 -0000 1.20
> > +++ parser.c 30 Apr 2019 20:28:18 -0000
> > @@ -39,7 +39,8 @@ enum token_type {
> >   ADDRESS,
> >   FLAG,
> >   PREFIX,
> > - IFNAME
> > + IFNAME,
> > + AREA
> >  };
> >  
> >  struct token {
> > @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
> >  };
> >  
> >  static const struct token t_show_area[] = {
> > - {ADDRESS, "", NONE, NULL},
> > + {AREA, "", NONE, NULL},
> >   {ENDTOKEN, "", NONE, NULL}
> >  };
> >  
> > @@ -218,6 +219,14 @@ match_token(const char *word, const stru
> >   res->action = t->value;
> >   }
> >   break;
> > + case AREA:
> > + if (parse_area(word, &res->addr)) {
> > + match++;
> > + t = &table[i];
> > + if (t->value)
> > + res->action = t->value;
> > + }
> > + break;
> >   case PREFIX:
> >   if (parse_prefix(word, &res->addr, &res->prefixlen)) {
> >   match++;
> > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
> >   case ADDRESS:
> >   fprintf(stderr, "  <address>\n");
> >   break;
> > + case AREA:
> > + fprintf(stderr, "  <area>\n");
> > + break;
> >   case PREFIX:
> >   fprintf(stderr, "  <address>[/<len>]\n");
> >   break;
> > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
> >   bzero(&ina, sizeof(ina));
> >  
> >   if (inet_pton(AF_INET, word, &ina)) {
> > + addr->s_addr = ina.s_addr;
> > + return (1);
> > + }
> > +
> > + return (0);
> > +}
> > +
> > +int
> > +parse_area(const char *word, struct in_addr *addr)
> > +{
> > + struct in_addr ina;
> > + const char *errstr;
> > +
> > + if (word == NULL)
> > + return (0);
> > +
> > + bzero(addr, sizeof(struct in_addr));
> > + bzero(&ina, sizeof(ina));
> > +
> > + if (inet_pton(AF_INET, word, &ina)) {
> > + addr->s_addr = ina.s_addr;
> > + return (1);
> > + }
> > +
> > + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
> > + if (errstr == NULL) {
> >   addr->s_addr = ina.s_addr;
> >   return (1);
> >   }
> > Index: parser.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 parser.h
> > --- parser.h 9 May 2011 12:25:35 -0000 1.13
> > +++ parser.h 30 Apr 2019 20:28:52 -0000
> > @@ -64,6 +64,7 @@ struct parse_result {
> >  
> >  struct parse_result *parse(int, char *[]);
> >  int parse_addr(const char *, struct in_addr *);
> > +int parse_area(const char *, struct in_addr *);
> >  int parse_prefix(const char *, struct in_addr *,
> >       u_int8_t *);
> >  
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Sebastian Benoit-3
In reply to this post by Remi Locherer



Remi Locherer([hidden email]) on 2019.05.15 23:15:03 +0200:

> On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:
> > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
> > > On 2019/04/29 11:58, Sebastian Benoit wrote:
> > > > David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> > > > >
> > > > >
> > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > > > > >
> > > > > > Hi David
> > > > > >
> > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > > > >> it's always bothered me that i config areas on a crisco using a number,
> > > > > >> but then have to think hard to convert that number to an address for use
> > > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > > > >> as an address. super annoying.
> > > > > >>
> > > > > >> so this changes the ospfd parser so it accepts both a number or address.
> > > > > >> i also changed it so it prints the number by default, which may be
> > > > > >> contentious. the manpage is slightly tweaked too.
> > > > > >>
> > > > > >> thoughts?
> > > > > >
> > > > > > I like it to be able to use a number instead of an address!
> > > > > >
> > > > > > It worked fine in my short test I performed.
> > > > > >
> > > > > > The output with the comment looks a bit strange to me.
> > > > >
> > > > > Are you sure it doesn't look... awesome?
> > > >
> > > > I like it!
> > >
> > > I don't really, but if we change this it needs to be displayed somehow
> > > and I don't have an idea to make it look nicer than this (cisco's method
> > > seems pretty horrible and wouldn't work for us anyway - looks like they
> > > remember which format was used to configure an area and use that as
> > > the output format...)
> > >
> >
> > Maybe it's better when we just allow both input formats but don't change
> > any output.
>
> Any opinions or comments on this? I think this would be a valuable addition
> to ospfd.

Yes, and diff is ok benno@

What about ospf6d?

> >
> > Below diff changes ospfctl to accept the address and number format for
> > "ospfct show database area XXX".
> >
> >
> > Index: parser.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 parser.c
> > --- parser.c 9 May 2011 12:25:35 -0000 1.20
> > +++ parser.c 30 Apr 2019 20:28:18 -0000
> > @@ -39,7 +39,8 @@ enum token_type {
> >   ADDRESS,
> >   FLAG,
> >   PREFIX,
> > - IFNAME
> > + IFNAME,
> > + AREA
> >  };
> >  
> >  struct token {
> > @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
> >  };
> >  
> >  static const struct token t_show_area[] = {
> > - {ADDRESS, "", NONE, NULL},
> > + {AREA, "", NONE, NULL},
> >   {ENDTOKEN, "", NONE, NULL}
> >  };
> >  
> > @@ -218,6 +219,14 @@ match_token(const char *word, const stru
> >   res->action = t->value;
> >   }
> >   break;
> > + case AREA:
> > + if (parse_area(word, &res->addr)) {
> > + match++;
> > + t = &table[i];
> > + if (t->value)
> > + res->action = t->value;
> > + }
> > + break;
> >   case PREFIX:
> >   if (parse_prefix(word, &res->addr, &res->prefixlen)) {
> >   match++;
> > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
> >   case ADDRESS:
> >   fprintf(stderr, "  <address>\n");
> >   break;
> > + case AREA:
> > + fprintf(stderr, "  <area>\n");
> > + break;
> >   case PREFIX:
> >   fprintf(stderr, "  <address>[/<len>]\n");
> >   break;
> > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
> >   bzero(&ina, sizeof(ina));
> >  
> >   if (inet_pton(AF_INET, word, &ina)) {
> > + addr->s_addr = ina.s_addr;
> > + return (1);
> > + }
> > +
> > + return (0);
> > +}
> > +
> > +int
> > +parse_area(const char *word, struct in_addr *addr)
> > +{
> > + struct in_addr ina;
> > + const char *errstr;
> > +
> > + if (word == NULL)
> > + return (0);
> > +
> > + bzero(addr, sizeof(struct in_addr));
> > + bzero(&ina, sizeof(ina));
> > +
> > + if (inet_pton(AF_INET, word, &ina)) {
> > + addr->s_addr = ina.s_addr;
> > + return (1);
> > + }
> > +
> > + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
> > + if (errstr == NULL) {
> >   addr->s_addr = ina.s_addr;
> >   return (1);
> >   }
> > Index: parser.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 parser.h
> > --- parser.h 9 May 2011 12:25:35 -0000 1.13
> > +++ parser.h 30 Apr 2019 20:28:52 -0000
> > @@ -64,6 +64,7 @@ struct parse_result {
> >  
> >  struct parse_result *parse(int, char *[]);
> >  int parse_addr(const char *, struct in_addr *);
> > +int parse_area(const char *, struct in_addr *);
> >  int parse_prefix(const char *, struct in_addr *,
> >       u_int8_t *);
> >  
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Remi Locherer
On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote:

>
>
>
> Remi Locherer([hidden email]) on 2019.05.15 23:15:03 +0200:
> > On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:
> > > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
> > > > On 2019/04/29 11:58, Sebastian Benoit wrote:
> > > > > David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> > > > > >
> > > > > >
> > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > > > > > >
> > > > > > > Hi David
> > > > > > >
> > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > > > > >> it's always bothered me that i config areas on a crisco using a number,
> > > > > > >> but then have to think hard to convert that number to an address for use
> > > > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > > > > >> as an address. super annoying.
> > > > > > >>
> > > > > > >> so this changes the ospfd parser so it accepts both a number or address.
> > > > > > >> i also changed it so it prints the number by default, which may be
> > > > > > >> contentious. the manpage is slightly tweaked too.
> > > > > > >>
> > > > > > >> thoughts?
> > > > > > >
> > > > > > > I like it to be able to use a number instead of an address!
> > > > > > >
> > > > > > > It worked fine in my short test I performed.
> > > > > > >
> > > > > > > The output with the comment looks a bit strange to me.
> > > > > >
> > > > > > Are you sure it doesn't look... awesome?
> > > > >
> > > > > I like it!
> > > >
> > > > I don't really, but if we change this it needs to be displayed somehow
> > > > and I don't have an idea to make it look nicer than this (cisco's method
> > > > seems pretty horrible and wouldn't work for us anyway - looks like they
> > > > remember which format was used to configure an area and use that as
> > > > the output format...)
> > > >
> > >
> > > Maybe it's better when we just allow both input formats but don't change
> > > any output.
> >
> > Any opinions or comments on this? I think this would be a valuable addition
> > to ospfd.
>
> Yes, and diff is ok benno@
>

David: ok remi@ for your diff without the printconf part.

> What about ospf6d?

I'll handle that.

>
> > >
> > > Below diff changes ospfctl to accept the address and number format for
> > > "ospfct show database area XXX".
> > >
> > >
> > > Index: parser.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> > > retrieving revision 1.20
> > > diff -u -p -r1.20 parser.c
> > > --- parser.c 9 May 2011 12:25:35 -0000 1.20
> > > +++ parser.c 30 Apr 2019 20:28:18 -0000
> > > @@ -39,7 +39,8 @@ enum token_type {
> > >   ADDRESS,
> > >   FLAG,
> > >   PREFIX,
> > > - IFNAME
> > > + IFNAME,
> > > + AREA
> > >  };
> > >  
> > >  struct token {
> > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
> > >  };
> > >  
> > >  static const struct token t_show_area[] = {
> > > - {ADDRESS, "", NONE, NULL},
> > > + {AREA, "", NONE, NULL},
> > >   {ENDTOKEN, "", NONE, NULL}
> > >  };
> > >  
> > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru
> > >   res->action = t->value;
> > >   }
> > >   break;
> > > + case AREA:
> > > + if (parse_area(word, &res->addr)) {
> > > + match++;
> > > + t = &table[i];
> > > + if (t->value)
> > > + res->action = t->value;
> > > + }
> > > + break;
> > >   case PREFIX:
> > >   if (parse_prefix(word, &res->addr, &res->prefixlen)) {
> > >   match++;
> > > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
> > >   case ADDRESS:
> > >   fprintf(stderr, "  <address>\n");
> > >   break;
> > > + case AREA:
> > > + fprintf(stderr, "  <area>\n");
> > > + break;
> > >   case PREFIX:
> > >   fprintf(stderr, "  <address>[/<len>]\n");
> > >   break;
> > > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
> > >   bzero(&ina, sizeof(ina));
> > >  
> > >   if (inet_pton(AF_INET, word, &ina)) {
> > > + addr->s_addr = ina.s_addr;
> > > + return (1);
> > > + }
> > > +
> > > + return (0);
> > > +}
> > > +
> > > +int
> > > +parse_area(const char *word, struct in_addr *addr)
> > > +{
> > > + struct in_addr ina;
> > > + const char *errstr;
> > > +
> > > + if (word == NULL)
> > > + return (0);
> > > +
> > > + bzero(addr, sizeof(struct in_addr));
> > > + bzero(&ina, sizeof(ina));
> > > +
> > > + if (inet_pton(AF_INET, word, &ina)) {
> > > + addr->s_addr = ina.s_addr;
> > > + return (1);
> > > + }
> > > +
> > > + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
> > > + if (errstr == NULL) {
> > >   addr->s_addr = ina.s_addr;
> > >   return (1);
> > >   }
> > > Index: parser.h
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> > > retrieving revision 1.13
> > > diff -u -p -r1.13 parser.h
> > > --- parser.h 9 May 2011 12:25:35 -0000 1.13
> > > +++ parser.h 30 Apr 2019 20:28:52 -0000
> > > @@ -64,6 +64,7 @@ struct parse_result {
> > >  
> > >  struct parse_result *parse(int, char *[]);
> > >  int parse_addr(const char *, struct in_addr *);
> > > +int parse_area(const char *, struct in_addr *);
> > >  int parse_prefix(const char *, struct in_addr *,
> > >       u_int8_t *);
> > >  
> > >
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

Remi Locherer
Hi David,

are you going to commit this?

Remi


On Thu, May 16, 2019 at 11:14:55PM +0200, Remi Locherer wrote:

> On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote:
> >
> >
> >
> > Remi Locherer([hidden email]) on 2019.05.15 23:15:03 +0200:
> > > On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:
> > > > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
> > > > > On 2019/04/29 11:58, Sebastian Benoit wrote:
> > > > > > David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
> > > > > > >
> > > > > > >
> > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
> > > > > > > >
> > > > > > > > Hi David
> > > > > > > >
> > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > > > > > >> it's always bothered me that i config areas on a crisco using a number,
> > > > > > > >> but then have to think hard to convert that number to an address for use
> > > > > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > > > > > >> as an address. super annoying.
> > > > > > > >>
> > > > > > > >> so this changes the ospfd parser so it accepts both a number or address.
> > > > > > > >> i also changed it so it prints the number by default, which may be
> > > > > > > >> contentious. the manpage is slightly tweaked too.
> > > > > > > >>
> > > > > > > >> thoughts?
> > > > > > > >
> > > > > > > > I like it to be able to use a number instead of an address!
> > > > > > > >
> > > > > > > > It worked fine in my short test I performed.
> > > > > > > >
> > > > > > > > The output with the comment looks a bit strange to me.
> > > > > > >
> > > > > > > Are you sure it doesn't look... awesome?
> > > > > >
> > > > > > I like it!
> > > > >
> > > > > I don't really, but if we change this it needs to be displayed somehow
> > > > > and I don't have an idea to make it look nicer than this (cisco's method
> > > > > seems pretty horrible and wouldn't work for us anyway - looks like they
> > > > > remember which format was used to configure an area and use that as
> > > > > the output format...)
> > > > >
> > > >
> > > > Maybe it's better when we just allow both input formats but don't change
> > > > any output.
> > >
> > > Any opinions or comments on this? I think this would be a valuable addition
> > > to ospfd.
> >
> > Yes, and diff is ok benno@
> >
>
> David: ok remi@ for your diff without the printconf part.
>
> > What about ospf6d?
>
> I'll handle that.
>
> >
> > > >
> > > > Below diff changes ospfctl to accept the address and number format for
> > > > "ospfct show database area XXX".
> > > >
> > > >
> > > > Index: parser.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> > > > retrieving revision 1.20
> > > > diff -u -p -r1.20 parser.c
> > > > --- parser.c 9 May 2011 12:25:35 -0000 1.20
> > > > +++ parser.c 30 Apr 2019 20:28:18 -0000
> > > > @@ -39,7 +39,8 @@ enum token_type {
> > > >   ADDRESS,
> > > >   FLAG,
> > > >   PREFIX,
> > > > - IFNAME
> > > > + IFNAME,
> > > > + AREA
> > > >  };
> > > >  
> > > >  struct token {
> > > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
> > > >  };
> > > >  
> > > >  static const struct token t_show_area[] = {
> > > > - {ADDRESS, "", NONE, NULL},
> > > > + {AREA, "", NONE, NULL},
> > > >   {ENDTOKEN, "", NONE, NULL}
> > > >  };
> > > >  
> > > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru
> > > >   res->action = t->value;
> > > >   }
> > > >   break;
> > > > + case AREA:
> > > > + if (parse_area(word, &res->addr)) {
> > > > + match++;
> > > > + t = &table[i];
> > > > + if (t->value)
> > > > + res->action = t->value;
> > > > + }
> > > > + break;
> > > >   case PREFIX:
> > > >   if (parse_prefix(word, &res->addr, &res->prefixlen)) {
> > > >   match++;
> > > > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
> > > >   case ADDRESS:
> > > >   fprintf(stderr, "  <address>\n");
> > > >   break;
> > > > + case AREA:
> > > > + fprintf(stderr, "  <area>\n");
> > > > + break;
> > > >   case PREFIX:
> > > >   fprintf(stderr, "  <address>[/<len>]\n");
> > > >   break;
> > > > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
> > > >   bzero(&ina, sizeof(ina));
> > > >  
> > > >   if (inet_pton(AF_INET, word, &ina)) {
> > > > + addr->s_addr = ina.s_addr;
> > > > + return (1);
> > > > + }
> > > > +
> > > > + return (0);
> > > > +}
> > > > +
> > > > +int
> > > > +parse_area(const char *word, struct in_addr *addr)
> > > > +{
> > > > + struct in_addr ina;
> > > > + const char *errstr;
> > > > +
> > > > + if (word == NULL)
> > > > + return (0);
> > > > +
> > > > + bzero(addr, sizeof(struct in_addr));
> > > > + bzero(&ina, sizeof(ina));
> > > > +
> > > > + if (inet_pton(AF_INET, word, &ina)) {
> > > > + addr->s_addr = ina.s_addr;
> > > > + return (1);
> > > > + }
> > > > +
> > > > + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
> > > > + if (errstr == NULL) {
> > > >   addr->s_addr = ina.s_addr;
> > > >   return (1);
> > > >   }
> > > > Index: parser.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> > > > retrieving revision 1.13
> > > > diff -u -p -r1.13 parser.h
> > > > --- parser.h 9 May 2011 12:25:35 -0000 1.13
> > > > +++ parser.h 30 Apr 2019 20:28:52 -0000
> > > > @@ -64,6 +64,7 @@ struct parse_result {
> > > >  
> > > >  struct parse_result *parse(int, char *[]);
> > > >  int parse_addr(const char *, struct in_addr *);
> > > > +int parse_area(const char *, struct in_addr *);
> > > >  int parse_prefix(const char *, struct in_addr *,
> > > >       u_int8_t *);
> > > >  
> > > >
> > >
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: ospfd: allow specifying area by number as well as id

David Gwynne-5
yes :D

> On 29 May 2019, at 15:05, Remi Locherer <[hidden email]> wrote:
>
> Hi David,
>
> are you going to commit this?
>
> Remi
>
>
> On Thu, May 16, 2019 at 11:14:55PM +0200, Remi Locherer wrote:
>> On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote:
>>>
>>>
>>>
>>> Remi Locherer([hidden email]) on 2019.05.15 23:15:03 +0200:
>>>> On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:
>>>>> On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
>>>>>> On 2019/04/29 11:58, Sebastian Benoit wrote:
>>>>>>> David Gwynne([hidden email]) on 2019.04.29 19:36:51 +1000:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 29 Apr 2019, at 4:59 pm, Remi Locherer <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> Hi David
>>>>>>>>>
>>>>>>>>> On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
>>>>>>>>>> it's always bothered me that i config areas on a crisco using a number,
>>>>>>>>>> but then have to think hard to convert that number to an address for use
>>>>>>>>>> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
>>>>>>>>>> as an address. super annoying.
>>>>>>>>>>
>>>>>>>>>> so this changes the ospfd parser so it accepts both a number or address.
>>>>>>>>>> i also changed it so it prints the number by default, which may be
>>>>>>>>>> contentious. the manpage is slightly tweaked too.
>>>>>>>>>>
>>>>>>>>>> thoughts?
>>>>>>>>>
>>>>>>>>> I like it to be able to use a number instead of an address!
>>>>>>>>>
>>>>>>>>> It worked fine in my short test I performed.
>>>>>>>>>
>>>>>>>>> The output with the comment looks a bit strange to me.
>>>>>>>>
>>>>>>>> Are you sure it doesn't look... awesome?
>>>>>>>
>>>>>>> I like it!
>>>>>>
>>>>>> I don't really, but if we change this it needs to be displayed somehow
>>>>>> and I don't have an idea to make it look nicer than this (cisco's method
>>>>>> seems pretty horrible and wouldn't work for us anyway - looks like they
>>>>>> remember which format was used to configure an area and use that as
>>>>>> the output format...)
>>>>>>
>>>>>
>>>>> Maybe it's better when we just allow both input formats but don't change
>>>>> any output.
>>>>
>>>> Any opinions or comments on this? I think this would be a valuable addition
>>>> to ospfd.
>>>
>>> Yes, and diff is ok benno@
>>>
>>
>> David: ok remi@ for your diff without the printconf part.
>>
>>> What about ospf6d?
>>
>> I'll handle that.
>>
>>>
>>>>>
>>>>> Below diff changes ospfctl to accept the address and number format for
>>>>> "ospfct show database area XXX".
>>>>>
>>>>>
>>>>> Index: parser.c
>>>>> ===================================================================
>>>>> RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
>>>>> retrieving revision 1.20
>>>>> diff -u -p -r1.20 parser.c
>>>>> --- parser.c 9 May 2011 12:25:35 -0000 1.20
>>>>> +++ parser.c 30 Apr 2019 20:28:18 -0000
>>>>> @@ -39,7 +39,8 @@ enum token_type {
>>>>> ADDRESS,
>>>>> FLAG,
>>>>> PREFIX,
>>>>> - IFNAME
>>>>> + IFNAME,
>>>>> + AREA
>>>>> };
>>>>>
>>>>> struct token {
>>>>> @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
>>>>> };
>>>>>
>>>>> static const struct token t_show_area[] = {
>>>>> - {ADDRESS, "", NONE, NULL},
>>>>> + {AREA, "", NONE, NULL},
>>>>> {ENDTOKEN, "", NONE, NULL}
>>>>> };
>>>>>
>>>>> @@ -218,6 +219,14 @@ match_token(const char *word, const stru
>>>>> res->action = t->value;
>>>>> }
>>>>> break;
>>>>> + case AREA:
>>>>> + if (parse_area(word, &res->addr)) {
>>>>> + match++;
>>>>> + t = &table[i];
>>>>> + if (t->value)
>>>>> + res->action = t->value;
>>>>> + }
>>>>> + break;
>>>>> case PREFIX:
>>>>> if (parse_prefix(word, &res->addr, &res->prefixlen)) {
>>>>> match++;
>>>>> @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
>>>>> case ADDRESS:
>>>>> fprintf(stderr, "  <address>\n");
>>>>> break;
>>>>> + case AREA:
>>>>> + fprintf(stderr, "  <area>\n");
>>>>> + break;
>>>>> case PREFIX:
>>>>> fprintf(stderr, "  <address>[/<len>]\n");
>>>>> break;
>>>>> @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
>>>>> bzero(&ina, sizeof(ina));
>>>>>
>>>>> if (inet_pton(AF_INET, word, &ina)) {
>>>>> + addr->s_addr = ina.s_addr;
>>>>> + return (1);
>>>>> + }
>>>>> +
>>>>> + return (0);
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +parse_area(const char *word, struct in_addr *addr)
>>>>> +{
>>>>> + struct in_addr ina;
>>>>> + const char *errstr;
>>>>> +
>>>>> + if (word == NULL)
>>>>> + return (0);
>>>>> +
>>>>> + bzero(addr, sizeof(struct in_addr));
>>>>> + bzero(&ina, sizeof(ina));
>>>>> +
>>>>> + if (inet_pton(AF_INET, word, &ina)) {
>>>>> + addr->s_addr = ina.s_addr;
>>>>> + return (1);
>>>>> + }
>>>>> +
>>>>> + ina.s_addr = htonl(strtonum(word, 0, 0xffffffff, &errstr));
>>>>> + if (errstr == NULL) {
>>>>> addr->s_addr = ina.s_addr;
>>>>> return (1);
>>>>> }
>>>>> Index: parser.h
>>>>> ===================================================================
>>>>> RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
>>>>> retrieving revision 1.13
>>>>> diff -u -p -r1.13 parser.h
>>>>> --- parser.h 9 May 2011 12:25:35 -0000 1.13
>>>>> +++ parser.h 30 Apr 2019 20:28:52 -0000
>>>>> @@ -64,6 +64,7 @@ struct parse_result {
>>>>>
>>>>> struct parse_result *parse(int, char *[]);
>>>>> int parse_addr(const char *, struct in_addr *);
>>>>> +int parse_area(const char *, struct in_addr *);
>>>>> int parse_prefix(const char *, struct in_addr *,
>>>>>     u_int8_t *);
>>>>>
>>>>>
>>>>
>>>
>>