Date: Thu, 07 Jul 2022 19:59:29 +0200 From: Kristof Provost <kp@FreeBSD.org> To: Steve Kargl <sgk@troutmask.apl.washington.edu> Cc: Warner Losh <imp@bsdimp.com>, Ryan Stone <rysto32@gmail.com>, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: buildkernel is broken Message-ID: <4FE3FF3B-9F2C-41BF-9F96-B8036055A9F6@FreeBSD.org> In-Reply-To: <YscRLGLeeN941jGa@troutmask.apl.washington.edu> References: <YsZptHF8tSttpTVy@troutmask.apl.washington.edu> <CAFMmRNz_FjszHNx92vbV9wezBHwT222w2PKO9WwN1zpWciRo5A@mail.gmail.com> <YsbtBZwfBMmltllg@troutmask.apl.washington.edu> <CAFMmRNyWYGD1UoyqMvTnyCnytKFrH9rPcM01Q2YMxhOQkWNd=A@mail.gmail.com> <Ysbyl28nKKlr5Kr2@troutmask.apl.washington.edu> <CAFMmRNyCDzMAcR6dfUAo53399YyaB%2B70ME20oBnOdLAqT-SBEQ@mail.gmail.com> <YscLgscTJlp6I4gf@troutmask.apl.washington.edu> <CANCZdfoA2OGPOMT%2BeD4cjDnQ5DvYeG1Z2iXv-okDB3o_KCSjng@mail.gmail.com> <YscRLGLeeN941jGa@troutmask.apl.washington.edu>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On 7 Jul 2022, at 19:00, Steve Kargl wrote: > On Thu, Jul 07, 2022 at 10:37:40AM -0600, Warner Losh wrote: >> On Thu, Jul 7, 2022 at 10:37 AM Steve Kargl < >> sgk@troutmask.apl.washington.edu> wrote: >> >>> Thanks, but >>> >>> root[216] git cherry-pick -n 37f604b49d4a >>> fatal: bad revision '37f604b49d4a' >>> root[217] pwd >>> /usr/src >> >> >> git fetch maybe? >> > > A cursory google search suggests that 'git fetch' > works on repositories not single files. > > I did look at the diff associated with 37f604b49d4a. > I am surprised that the commit that broke buildkernel > for me was allowed to be committed. It was posted for review in https://reviews.freebsd.org/D35716 I’ll also point out that this commit works just fine in nearly all of our kernel configs, because there are very few (only one powerpc config, as far as I can tell) that do not have VIMAGE. Arguably we should have a non-VIMAGE kernel config around (probably for amd64) so it’s more likely we spot these issues prior to commit. Arbitrary non-default kernel configs are more likely to see issues like this one. I don’t think that can be avoided. > The fix in > 37f604b49d4a seems rather questionable especially given > that there is no comment about why the macro is expanded > to a zero-trip loop. > I’m not sure how I could have been much more clear than this: VNET_FOREACH() is a LIST_FOREACH if VIMAGE is set, but empty if it's not. This means that users of the macro couldn't use 'continue' or 'break' as one would expect of a loop. I welcome suggestions on how to improve my future commit messages. To rephrase it a bit: VNET_FOREACH() used to be very misleading, in that it was only a loop with options VIMAGE, and empty (so any code within would be its own block, and be executed exactly once, for the only vnet that exists without VIMAGE). That’s fine, unless you want to ‘continue’ or ‘break’ the loop. That worked with VIMAGE (so the issue in the dummynet fix was not seen) but not without it. Kristof [-- Attachment #2 --] <!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8"> </head> <body><div style="font-family: sans-serif;"><div class="markdown" style="white-space: normal;"> <p dir="auto">On 7 Jul 2022, at 19:00, Steve Kargl wrote:</p> </div><div class="plaintext" style="white-space: normal;"><blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;"><p dir="auto">On Thu, Jul 07, 2022 at 10:37:40AM -0600, Warner Losh wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"><p dir="auto">On Thu, Jul 7, 2022 at 10:37 AM Steve Kargl < <br> sgk@troutmask.apl.washington.edu> wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"><p dir="auto">Thanks, but</p> <p dir="auto">root[216] git cherry-pick -n 37f604b49d4a <br> fatal: bad revision '37f604b49d4a' <br> root[217] pwd <br> /usr/src</p> </blockquote><p dir="auto">git fetch maybe?</p> </blockquote><p dir="auto">A cursory google search suggests that 'git fetch' <br> works on repositories not single files.</p> <p dir="auto">I did look at the diff associated with 37f604b49d4a. <br> I am surprised that the commit that broke buildkernel <br> for me was allowed to be committed.</p> </blockquote></div> <div class="markdown" style="white-space: normal;"> <p dir="auto">It was posted for review in <a href="https://reviews.freebsd.org/D35716">https://reviews.freebsd.org/D35716</a></p>; <p dir="auto">I’ll also point out that this commit works just fine in nearly all of our kernel configs, because there are very few (only one powerpc config, as far as I can tell) that do not have VIMAGE.<br> Arguably we should have a non-VIMAGE kernel config around (probably for amd64) so it’s more likely we spot these issues prior to commit.<br> Arbitrary non-default kernel configs are more likely to see issues like this one. I don’t think that can be avoided.</p> </div><div class="plaintext" style="white-space: normal;"><blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;"><p dir="auto">The fix in <br> 37f604b49d4a seems rather questionable especially given <br> that there is no comment about why the macro is expanded <br> to a zero-trip loop.</p> <br></blockquote></div> <div class="markdown" style="white-space: normal;"> <p dir="auto">I’m not sure how I could have been much more clear than this:</p> <pre style="margin-left: 15px; margin-right: 15px; padding: 5px; border: thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #E4E4E4;"><code>VNET_FOREACH() is a LIST_FOREACH if VIMAGE is set, but empty if it's not. This means that users of the macro couldn't use 'continue' or 'break' as one would expect of a loop. </code></pre> <p dir="auto">I welcome suggestions on how to improve my future commit messages.</p> <p dir="auto">To rephrase it a bit: VNET_FOREACH() used to be very misleading, in that it was only a loop with options VIMAGE, and empty (so any code within would be its own block, and be executed exactly once, for the only vnet that exists without VIMAGE). That’s fine, unless you want to ‘continue’ or ‘break’ the loop. That worked with VIMAGE (so the issue in the dummynet fix was not seen) but not without it.</p> <p dir="auto">Kristof</p> </div></div></body> </html>help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FE3FF3B-9F2C-41BF-9F96-B8036055A9F6>