bird2: add new package - #30165
Conversation
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (5a43c6e, first review of this PR).
Nothing here blocks a merge. The commit message matches the diff, PKG_RELEASE:=1 is correct for a new package, PKG_BUILD_DIR is overridden for the bird- tarball prefix, BuildPackage is called with literal package names, and the Makefile follows the indentation conventions (two spaces for the metadata blocks, tabs for the install recipes, none for conffiles). The duplicate-package concern against the routing feed is already covered by the coordinated removal PR mentioned in the description, so I have not raised it.
Three inline remarks: two nits (stale conffiles entries, test-version.sh shape) and one question about the provenance of 000-ospf-bus-error.patch.
Generated by Claude Code
| /etc/bird4.conf | ||
| /etc/bird6.conf |
There was a problem hiding this comment.
nit: /etc/bird4.conf and /etc/bird6.conf are never installed by this package — Package/bird2/install only creates /etc/bird.conf from doc/bird.conf.example. These are leftovers from the bird1 bird4/bird6 packages (the same era as the CONFLICTS:=... bird4 bird6 lines above). Nothing breaks at build time — package-pack.mk skips conffile entries that do not exist — but since this is the move into packages, it is a good moment to drop the dead entries.
| /etc/bird4.conf | |
| /etc/bird6.conf |
Generated by Claude Code
| @@ -0,0 +1 @@ | |||
| bird --version 2>&1 | grep "$PKG_VERSION" | |||
There was a problem hiding this comment.
This script runs bird --version for all three packages, so the bird2c and bird2cl runs never exercise birdc/birdcl — they only pass because bird is pulled in as a dependency and happens to be on the image. It also has no shebang and a trailing space at end of line; every other test-version.sh in the feed starts with #!/bin/sh and dispatches on $PKG_NAME (see net/openvpn-openssl-style scripts, e.g. libs/openldap/test-version.sh).
grep -F also avoids the . in 2.19.2 being treated as a regex wildcard.
| bird --version 2>&1 | grep "$PKG_VERSION" | |
| #!/bin/sh | |
| # shellcheck shell=busybox | |
| case "$PKG_NAME" in | |
| bird2) | |
| bird --version 2>&1 | grep -F "$PKG_VERSION" | |
| ;; | |
| bird2c|bird2cl) | |
| # birdc/birdcl have no --version flag of their own. | |
| exit 0 | |
| ;; | |
| *) | |
| echo "Untested package: $PKG_NAME" >&2 | |
| exit 1 | |
| ;; | |
| esac |
Generated by Claude Code
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: Matt Reeve <matt@mreeve.com> | ||
| Date: Tue, 10 Aug 2021 12:41:04 +0100 | ||
| Subject: [PATCH] Fix bus error on OSPF on IPQ806X | ||
|
|
||
| --- |
There was a problem hiding this comment.
This patch carries an all-zero placeholder commit ID and an empty body, so nothing records why PACKED on struct top_hash_entry fixes the bus error, whether it was ever reported to the BIRD maintainers, or under what condition it can be dropped. Unlike 0001-birdc-Use-var-tmp-for-history-file.patch — a one-liner whose intent is obvious from the diff — this one changes the layout and access codegen of a core OSPF data structure, which a later reader cannot infer from the subject alone.
Since the package is moving into packages and will be carried here from now on, could you add a short body with an upstream/bug reference (or an Upstream-Status: line saying it was never submitted)? It also dates from 2021 against BIRD 2.0.x — has it been re-checked that it is still needed on 2.19.2?
Unrelated to the above: the two patches mix numbering styles (000- vs 0001-). Both occur in the feed, but they are usually consistent within one package.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits since my last review at 5a43c6e: 379e79d (an amended re-push of the original commit, which reworks test-version.sh) and 60ea038. Both commit messages match their diffs.
Both previous nits are addressed. The stale /etc/bird4.conf and /etc/bird6.conf conffile entries are gone, leaving only the /etc/bird.conf that Package/bird2/install actually creates. test-version.sh now has a shebang, the # shellcheck shell=busybox directive that 57 other scripts in the feed use, grep -F so the dots in 2.19.2 are not regex wildcards, and an explicit *) branch that fails on an unknown package — the same shape as libs/openldap/test-version.sh. Keeping bird2c/bird2cl on bird --version rather than trying the clients themselves is a reasonable call now that the reason is recorded in the script, and +bird2 in both DEPENDS guarantees the daemon is present.
Still open from the previous round, not repeated inline: the question on 000-ospf-bus-error.patch — empty body, all-zero placeholder commit ID, written in 2021 against BIRD 2.0.x — has not been answered. That is a documentation ask, not a merge blocker.
Nothing in this round blocks a merge; one packaging-hygiene nit inline.
Generated by Claude Code
|
|
||
| CONFIGURE_ARGS += --disable-libssh | ||
|
|
||
| define Package/bird2/conffiles |
There was a problem hiding this comment.
nit: this block is what 60ea038 ("bird2: drop stale conffiles entries") touches, and that commit does nothing but undo two lines 379e79d added earlier in the same PR. The test-version.sh rework from the same review round was folded into 379e79d by amending, so the two fixes are handled inconsistently — and as it stands master would gain a "drop stale conffiles entries" commit for a package that never shipped those entries from this repo. Worth squashing into the add commit before merge.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit since my last review at 60ea038: 96801d9, which fills in the header of 000-ospf-bus-error.patch. That answers the documentation question from the first round — the patch now carries a reproducer, an explanation and an Upstream-Status line, which is what I asked for.
Commit checks
96801d9"bird2: document the OSPF alignment patch" — the body says the rationale is restored from the routing-feed commit, but one paragraph (thebtime/NEON mechanism) is new material that is not in openwrt/routing@f4251cc, and the commit also rewrites the patch subject without saying so. Both flagged inline.
Nothing here blocks a merge. Two remarks on the new header (one substantive, one a nit) plus a nit on the subject rewrite, inline.
Still open from the previous round, not repeated inline: the squash suggestion for 60ea038 — master would otherwise gain a "drop stale conffiles entries" commit for a package that never shipped those entries here. Same applies to 96801d9: as a standalone commit it documents a patch that, from master's point of view, is introduced by this very PR.
CI on 96801d9 has no results yet (all checks pending), so nothing to report there.
Generated by Claude Code
| struct top_hash_entry contains a btime (64 bit) after a series of 16 and | ||
| 32 bit members, so the compiler emits NEON accesses that assume an | ||
| alignment the allocation does not guarantee on this architecture. |
There was a problem hiding this comment.
This mechanism is not in the commit the message says it was restored from. openwrt/routing@f4251cc only says "The problem is due to a struct not being properly aligned on the ARMv7 architecture" — it names no field, no btime, and no NEON. The kernel log lines above (lines 9-10) do come from that commit verbatim, so this paragraph is the one piece of the header that has no source.
It also does not obviously follow from the trap that is quoted: f44c0a1f is a NEON vst1.64 {dN-dN+1}, [rX:128], i.e. a 128-bit-aligned vector store of two doublewords, which is what the compiler emits when it vectorises a struct initialisation or copy — not how it would access a single 64-bit btime member. PACKED drops the type's alignment to 1, which stops the vectoriser from assuming the 16-byte alignment; that works regardless of where btime sits.
Could you either back the field-level explanation up (which struct member, which access), or fall back to the original commit's wording? A header that overstates what was diagnosed is worse than the empty one it replaces, since the next person to revisit this patch will trust it.
Generated by Claude Code
| Upstream-Status: not submitted. Upstream may prefer to fix the layout | ||
| itself, in which case this patch can be dropped; as of 2.19.2 the | ||
| structure still carries no alignment attribute. |
There was a problem hiding this comment.
Two header pieces are still missing now that this patch is being made upstream-ready.
Signed-off-by: Matt Reeve <matt@mreeve.com> exists in the source commit (openwrt/routing@f4251cc) and can be restored for free — the patch keeps his From: line, so the sign-off belongs with it.
The --- separator was also dropped, so the body now runs straight into the diff. patch/quilt do not care and git mailinfo recovers by treating --- a/... as the patch break, but the sibling patch keeps the separator (0001-birdc-Use-var-tmp-for-history-file.patch#L12-L13) and it costs nothing to keep both patches the same shape.
| Upstream-Status: not submitted. Upstream may prefer to fix the layout | |
| itself, in which case this patch can be dropped; as of 2.19.2 the | |
| structure still carries no alignment attribute. | |
| Upstream-Status: not submitted. Upstream may prefer to fix the layout | |
| itself, in which case this patch can be dropped; as of 2.19.2 the | |
| structure still carries no alignment attribute. | |
| Signed-off-by: Matt Reeve <matt@mreeve.com> | |
| --- |
Generated by Claude Code
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: Matt Reeve <matt@mreeve.com> | ||
| Date: Tue, 10 Aug 2021 12:41:04 +0100 | ||
| Subject: [PATCH] ospf: fix alignment fault on ARMv7 |
There was a problem hiding this comment.
nit: the commit message says this change only adds the rationale and the upstream status, but it also rewrites another author's subject line — Fix bus error on OSPF on IPQ806X became ospf: fix alignment fault on ARMv7, with the From: still pointing at Matt Reeve. Either mention the rename in the commit body or leave the subject as he wrote it.
Also, the file is still called 000-ospf-bus-error.patch, so the filename and the subject now describe the fault differently.
Generated by Claude Code
709f158 to
fd585eb
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed series (249357e, 886674f, fd585eb). Relative to my last review at 96801d9 the only content change under net/bird2 is the header of 000-ospf-bus-error.patch — the other two commits are tree-identical to 379e79d/60ea038, just rebased onto 91a6967. All three commit messages match their diffs.
Both substantive points from the previous round are addressed. The field-level btime/NEON mechanism that had no source in the routing-feed commit is gone, and the header now carries that commit's wording ("The problem is due to a struct not being properly aligned on the ARMv7 architecture") together with Matt Reeve's Signed-off-by. The subject rewrite is reverted, so From:, the subject line and the 000-ospf-bus-error.patch filename describe the same fault again, and the Upstream-Status line survives in a shorter form.
One leftover from that round, inline: the --- separator between header and diff is still missing — and it turns out fd585eb is what removed it, since the patch did carry one when 249357e added it.
Still open from the previous rounds, not repeated inline: the squash suggestion for the conffiles commit, which now applies to 886674f and fd585eb alike — from master's point of view both fix up a package that this same PR introduces.
Nothing here blocks a merge; the single inline remark is a nit.
CI on fd585eb has no results yet (all checks pending), so nothing to report there.
Generated by Claude Code
| Signed-off-by: Matt Reeve <matt@mreeve.com> | ||
|
|
||
| Upstream-Status: not submitted. As of 2.19.2 the structure still carries | ||
| no alignment attribute. |
There was a problem hiding this comment.
nit: the --- separator is gone, and this commit is what removed it. The patch as added in 249357e had Subject: / blank / --- / blank; fd585eb replaces that --- line with the body text instead of keeping it below, so the header now runs straight into --- a/proto/ospf/topology.h. The commit message describes the change as restoring the rationale and recording the upstream status, so this deletion is undocumented.
patch/quilt do not care and git mailinfo recovers by treating --- a/... as the patch break, but the sibling patch keeps the separator (0001-birdc-Use-var-tmp-for-history-file.patch#L12-L13) and it costs nothing to keep both patches the same shape.
While touching these lines: Signed-off-by conventionally comes last among the trailers, i.e. after Upstream-Status.
| Signed-off-by: Matt Reeve <matt@mreeve.com> | |
| Upstream-Status: not submitted. As of 2.19.2 the structure still carries | |
| no alignment attribute. | |
| Upstream-Status: not submitted. As of 2.19.2 the structure still carries | |
| no alignment attribute. | |
| Signed-off-by: Matt Reeve <matt@mreeve.com> | |
| --- |
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit since my last review at fd585eb: 7a47a78, which replaces it after a force-push. 249357e and 886674f are unchanged, and the whole delta under net/bird2 is four lines in the header of 000-ospf-bus-error.patch. The commit message matches the diff.
The remaining point from the previous round is fixed: the --- separator is back, so the header no longer runs straight into --- a/proto/ospf/topology.h, and Signed-off-by: Matt Reeve now sits last among the trailers, after Upstream-Status. The body text itself is untouched, so it still carries the routing-feed commit's wording and nothing beyond it. Nothing new to flag.
Still open from the earlier rounds, not repeated inline: the squash suggestion, which now covers 886674f and 7a47a78 — from master's point of view both fix up a package that 249357e introduces in the same series. That is series hygiene, not a defect.
CI on 7a47a78 is green: all ten Feeds Package Test Build targets and the three FormalityCheck jobs passed.
Generated by Claude Code
|
|
||
| PKG_NAME:=bird2 | ||
| PKG_VERSION:=2.19.2 | ||
| PKG_RELEASE:=1 |
There was a problem hiding this comment.
This is now lower than the package in the routing feed, we should bump it to the same version.
There was a problem hiding this comment.
Increased, so we can merge this one. :)
BIRD is an internet routing daemon with support of modern routing protocols (BGP, OSPF, RIP, Babel) for IPv4 and IPv6. This is the 2.x branch. Moved from the openwrt/routing feed, as discussed in openwrt/routing#184. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
/etc/bird4.conf and /etc/bird6.conf are left over from the bird1 packages, which shipped a separate configuration file per address family. bird2 is a single daemon handling both and installs /etc/bird.conf only, so the two entries refer to files this package never creates. Reported-by: openwrt-ai[bot] Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The patch carried only a subject line and a placeholder commit id, so nothing recorded why marking struct top_hash_entry as PACKED fixes a bus error, or whether it can be dropped. Restore the description and the sign-off from the commit that introduced it in the routing feed, keeping the original author's wording, and record the upstream status. Verified that BIRD 2.19.2 still declares the structure without any alignment attribute, so the patch is still needed. The header keeps the "---" separator the sibling patch uses, and orders Signed-off-by last among the trailers. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Formality Check: Suggestions AvailableWe checked this pull request against the contribution guidelines. Here is what needs your attention:
|
The package has been moved to the openwrt/packages feed, as discussed in openwrt#184. See openwrt/packages#30165. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Adds bird2 from the openwrt/routing feed — the routing packages are being moved into openwrt/packages one by one, as discussed in openwrt/routing#184.
Includes the SPDX license identifier cleanup pending in openwrt/routing#1193.
The content matches the current routing feed master. Once this is merged, the package will be removed from the routing feed (a coordinated removal PR is prepared there).
Maintainer: @tohojo, @PolynomialDivision