| Message ID | 20231015174110.2083521-1-bjorn@mork.no |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [master,23.05] ramips: fix ZyXEL NR7101 bricking typo | expand |
On Sun, Oct 15, 2023 at 09:59:57PM +0200, Paul D wrote: > On 2023-10-15 19:41, Bjørn Mork wrote: > > A typo snuck in with the addition of Cudy M1800, changing > > "nr7101" to "nt7101". The result is a default network config > > for NR7101 without the only ethernet interface on the NR7101, > > thereby soft bricking it. > > > > Fixes: f6d394e9f2fd ("ramips: add support for Cudy M1800") > > Signed-off-by: Bjørn Mork <bjorn@mork.no> > > --- > > Ref https://forum.openwrt.org/t/zyxel-nr7101-not-responding-after-flashing-initramfs/174409 > > and https://github.com/openwrt/openwrt/pull/13699 > > > > This needs to be applied to 23.05 and master ASAP. It is already > > bricking devices. > > > > And it would be great if we could have some automated check > > to help us spot these kinds of unrelated and unexpected > > changes. I don't think the regular review process will ever > > be able to catch this, as that is mostly focues on the newly > > added device. > While I second the urgency of this, I venture the question of how one might > otherwise catch these things, but for sharp eyes. There is an attention > deficit with respect to the volume of patches and PRs that come in. > > Well the only solution is being more aggressive with nitpicking and request very dry patch with less patch delta as possible. The PR was introducing a new device so it's understandable to not think that on moving code, the guy introduced a typo in rewiting the entry in the shell... It's both trusting the submitter and not expecting these kind of error :(
While I second the urgency of this, I venture the question of how one might otherwise catch these things, but for sharp eyes. There is an attention deficit with respect to the volume of patches and PRs that come in. On 2023-10-15 19:41, Bjørn Mork wrote: > A typo snuck in with the addition of Cudy M1800, changing > "nr7101" to "nt7101". The result is a default network config > for NR7101 without the only ethernet interface on the NR7101, > thereby soft bricking it. > > Fixes: f6d394e9f2fd ("ramips: add support for Cudy M1800") > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > Ref https://forum.openwrt.org/t/zyxel-nr7101-not-responding-after-flashing-initramfs/174409 > and https://github.com/openwrt/openwrt/pull/13699 > > This needs to be applied to 23.05 and master ASAP. It is already > bricking devices. > > And it would be great if we could have some automated check > to help us spot these kinds of unrelated and unexpected > changes. I don't think the regular review process will ever > be able to catch this, as that is mostly focues on the newly > added device. > > > Bjørn > > target/linux/ramips/mt7621/base-files/etc/board.d/02_network | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network > index 67fe45f63360..b4c2c6dd68a8 100644 > --- a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network > +++ b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network > @@ -95,7 +95,7 @@ ramips_setup_interfaces() > ;; > cudy,m1800|\ > yuncore,ax820|\ > - zyxel,nt7101) > + zyxel,nr7101) > ucidef_set_interfaces_lan_wan "lan" "wan" > ;; > gnubee,gb-pc1)
On 15.10.2023 21:59, Paul D wrote: > This needs to be applied to 23.05 and master ASAP. It is already > bricking devices. how about quickly removing the erroneous image from https://downloads.openwrt.org/releases/23.05.0/ plus adding a prominent red NOTE on https://openwrt.org/toh/zyxel/nr7101 not to flash 23.05 ? preventing head aches is the best way to treat 'em ..ede
Christian Marangi <ansuelsmth@gmail.com> writes: > On Sun, Oct 15, 2023 at 09:59:57PM +0200, Paul D wrote: >> On 2023-10-15 19:41, Bjørn Mork wrote: >> >> > And it would be great if we could have some automated check >> > to help us spot these kinds of unrelated and unexpected >> > changes. I don't think the regular review process will ever >> > be able to catch this, as that is mostly focues on the newly >> > added device. >> While I second the urgency of this, I venture the question of how one might >> otherwise catch these things, but for sharp eyes. There is an attention >> deficit with respect to the volume of patches and PRs that come in. >> >> > > Well the only solution is being more aggressive with nitpicking and > request very dry patch with less patch delta as possible. > > The PR was introducing a new device so it's understandable to not think > that on moving code, the guy introduced a typo in rewiting the entry in > the shell... > > It's both trusting the submitter and not expecting these kind of error > :( I believe it's understandable how such an error can happen with humans involved. The error is probably a simple matter of "spurious typing". I don't think we can expect any reviewer, including the submitter, to spot this. It's a single character in an legitimately modified part of a file. The resulting diff is pretty much as expected, without any extra blobs. And the expected new-device change is trivial, so it's not where any reviewer would (or should) spend their time. Their focus is obviously on the new device anyway. It's not uncommon to see similar errors slip through and end up in "main". The only difference with this one was that it was so subtle that it only broke new installs of a single, rare, device. I have no solution to offer, unfortunately. Which was why the question was open without actual suggestions on how to improve this. Hoping that smarter people than me can come up with something. But a solution has to be scripted/automated IMHO. Making it a reviewer problem will not solve anything. It will only create more problems by abusing reviewer resources. Bjørn
Hi, that was my mistake - sorry! Thanks for finding and fixing it :-) I keep accidentally tapping the R key when jumping over words (E) for some reason. "Purging the r's" is something I do quite regularly. But a 't'? :-D Regarding the detection of such mistakes: To retain alphabetical ordering, there have to be collateral line changes. This can probably be alleviated by using to commits, one for adding the pattern to the case expression and one for moving the existing patterns to restore ordering. I would argue that the brain is actually very good at spotting differences if it's not occupied with "and this is the part where they've assigned the interface roles". It isn't much but I don't think automated tests or "more discipline" (coding or reviewing) are appropriate or easier to implement reliably. kind regards On Sun, 15 Oct 2023 19:41:10 +0200 Bjørn Mork <bjorn@mork.no> wrote: > A typo snuck in with the addition of Cudy M1800, changing > "nr7101" to "nt7101". The result is a default network config > for NR7101 without the only ethernet interface on the NR7101, > thereby soft bricking it. > > Fixes: f6d394e9f2fd ("ramips: add support for Cudy M1800") > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > Ref > https://forum.openwrt.org/t/zyxel-nr7101-not-responding-after-flashing-initramfs/174409 > and https://github.com/openwrt/openwrt/pull/13699 > > This needs to be applied to 23.05 and master ASAP. It is already > bricking devices. > > And it would be great if we could have some automated check > to help us spot these kinds of unrelated and unexpected > changes. I don't think the regular review process will ever > be able to catch this, as that is mostly focues on the newly > added device. > > > Bjørn > > target/linux/ramips/mt7621/base-files/etc/board.d/02_network | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git > a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network > b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network index > 67fe45f63360..b4c2c6dd68a8 100644 --- > a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network +++ > b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network @@ > -95,7 +95,7 @@ ramips_setup_interfaces() ;; cudy,m1800|\ > yuncore,ax820|\ > - zyxel,nt7101) > + zyxel,nr7101) > ucidef_set_interfaces_lan_wan "lan" "wan" > ;; > gnubee,gb-pc1)
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. Em 16/10/2023 04:32, Bjørn Mork escreveu: > I have no solution to offer, unfortunately. Which was why the question > was open without actual suggestions on how to improve this. Hoping that > smarter people than me can come up with something. GNU config (from automake/autoconf "fame") has a similar problem for the "config.sub" and "config.guess" scripts. And it does have testing coverage. Doing something similar to what they did might work, perhaps? The implementation would be vastly different, though. There is no need to attempt full coverage, it is likely well worth it if the risk of *regressions* is greatly reduced.
diff --git a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network index 67fe45f63360..b4c2c6dd68a8 100644 --- a/target/linux/ramips/mt7621/base-files/etc/board.d/02_network +++ b/target/linux/ramips/mt7621/base-files/etc/board.d/02_network @@ -95,7 +95,7 @@ ramips_setup_interfaces() ;; cudy,m1800|\ yuncore,ax820|\ - zyxel,nt7101) + zyxel,nr7101) ucidef_set_interfaces_lan_wan "lan" "wan" ;; gnubee,gb-pc1)
A typo snuck in with the addition of Cudy M1800, changing "nr7101" to "nt7101". The result is a default network config for NR7101 without the only ethernet interface on the NR7101, thereby soft bricking it. Fixes: f6d394e9f2fd ("ramips: add support for Cudy M1800") Signed-off-by: Bjørn Mork <bjorn@mork.no> --- Ref https://forum.openwrt.org/t/zyxel-nr7101-not-responding-after-flashing-initramfs/174409 and https://github.com/openwrt/openwrt/pull/13699 This needs to be applied to 23.05 and master ASAP. It is already bricking devices. And it would be great if we could have some automated check to help us spot these kinds of unrelated and unexpected changes. I don't think the regular review process will ever be able to catch this, as that is mostly focues on the newly added device. Bjørn target/linux/ramips/mt7621/base-files/etc/board.d/02_network | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)