Bug 232231

Created attachment 198107 [details]
add birdvty user

Created attachment 198108 [details]
Fix bird/bird6 wrong LSA collision detection

Patch attributions: Julien Charbon <jcharbon@verisign.com>, Asbjorn Mikkelsen <amikkelsen@verisign.com>

Comment 4 Olivier Cochard 2018-10-14 08:54:22 UTC

Thanks for your patches!

Few questions:
You've choose to create a net/bird/pkg-install to create birdvty group.
1. Why not declaring this new group into /usr/ports/GIDs in place of the pkg-install ?
2. Do you think it's a good idea to reuse the same group for net/bird2 ?

I believe you did all this works during your working time for Verisign.
3. Are you agree if I add a "Sponsored by: Verisign" to the commit log?

Comment 5 Olivier Cochard 2018-10-18 04:21:01 UTC

and another question:
4. Patch fixing LSA collision: Does this patch was send upstream ? (if it apply to the branch 2.0)
5. Patch fixing LSA collision: It doesn't apply well.
File proto/ospf/topology.c of bird 1.6.4, line 281 is:
if (!en->nf || !en->lsa_body)

But the patch is referencing an original line 281 which is:
if (en->lsa_body == NULL)

Comment 6 Julien Charbon 2018-10-18 07:25:05 UTC

Hi Olivier, thanks for the followup:

> 1. Why not declaring this new group into /usr/ports/GIDs in place of the pkg-install ?

 No reasons, we should /usr/ports/GIDs instead right.  Will update the corresponding patch.

> 2. Do you think it's a good idea to reuse the same group for net/bird2 ?

 It is a good idea for bird2 too.

> I believe you did all this works during your working time for Verisign.
> 3. Are you agree if I add a "Sponsored by: Verisign" to the commit log?

 Perfect.

> 4. Patch fixing LSA collision: Does this patch was send upstream ? (if it apply to the branch 2.0)

 The patch was not sent upstream. Will see if the patch needs update for bird branch 2.0.

> 5. Patch fixing LSA collision: It doesn't apply well.
> File proto/ospf/topology.c of bird 1.6.4, line 281 is:
> if (!en->nf || !en->lsa_body)
> But the patch is referencing an original line 281 which is:
> if (en->lsa_body == NULL)

 Good catch. Interesting actually 'make patch' does not complains:

$ make clean distclean
===>  Cleaning for bird-1.6.4_1
===>  Cleaning for bird6-1.6.4_1
===>  Deleting distfiles for bird-1.6.4_1
$ ls files/patch-proto__ospf__topology.c
files/patch-proto__ospf__topology.c
$ make patch          
===>  License GPLv2 accepted by the user
===>   bird-1.6.4_1 depends on file: /usr/local/sbin/pkg - found
=> bird-1.6.4.tar.gz doesn't seem to exist in /app/jcharbon/freebsd-ports/distfiles/.
=> Attempting to fetch ftp://bird.network.cz/pub/bird/bird-1.6.4.tar.gz
bird-1.6.4.tar.gz                             100% of  994 kB  516 kBps 00m02s
===> Fetching all distfiles required by bird-1.6.4_1 for building
===>  Extracting for bird-1.6.4_1
=> SHA256 Checksum OK for bird-1.6.4.tar.gz.
===>  Patching for bird-1.6.4_1
===>  Applying FreeBSD patches for bird-1.6.4_1

 But still, the patch is not clean against bird-1.6.4, will update the patch

Created attachment 198963 [details]
add birdvty user - v2

Created attachment 198965 [details]
Fix bird/bird6 wrong LSA collision detection - v2

Hi Olivier,

I have updated the two patches with a "- v2" suffix. We have also submitted the patch upstream:
https://github.com/BIRD/bird/pull/3

Please let me know if you have any further comments or questions.

Best regards

Comment 10 commit-hook 2018-11-10 23:03:44 UTC

A commit references this bug:

Author: olivier
Date: Sat Nov 10 23:02:45 UTC 2018
New revision: 484648
URL: https://svnweb.freebsd.org/changeset/ports/484648

Log:
  Fixes wrong LSA collision detection and add birdvty group

  PR:		232231
  Submitted by:	Julien Charbon <jcharbon@verisign.com>
  Submitted by:	Asbjorn Mikkelsen <amikkelsen@verisign.com>
  Sponsored by:	Verisign

Changes:
  head/GIDs
  head/net/bird/Makefile
  head/net/bird/files/bird.in
  head/net/bird/files/bird6.in
  head/net/bird/files/patch-proto__ospf__lsupd.c
  head/net/bird/files/patch-proto__ospf__topology.c

Comment 11 Olivier Cochard 2018-11-10 23:05:38 UTC

Thanks for your patches!