Date: Tue, 22 Sep 2020 15:50:29 -0600 From: Warner Losh <imp@bsdimp.com> To: Alan Somers <asomers@freebsd.org> Cc: Kyle Evans <kevans@freebsd.org>, Mateusz Guzik <mjguzik@gmail.com>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r365643 - head/bin/cp Message-ID: <CANCZdfqs8Hrmm-mHConJCW29PRHAPByTwP8J0aTR6naxNL7Ncg@mail.gmail.com> In-Reply-To: <CAOtMX2iNhii4tCwvTSn7JGNAJ_afoyjwQYM=8MUo4fitt992Kg@mail.gmail.com> References: <202009112049.08BKnavL032212@repo.freebsd.org> <CACNAnaGXCt=PezNX-5KU7xTk%2By-um37p8sk1c1KUEc4ywDQ5uQ@mail.gmail.com> <CAOtMX2gtCmyRrhUVia6Arc_PCbgoasG61Xwt=cUaDd1Q1Gtw8g@mail.gmail.com> <CAGudoHEoF3T=hD7-tSNzz0--xKB3LwbjZQH1Xtxujq19hdm2vg@mail.gmail.com> <CAOtMX2i9v73ydzCNz-ZWEZvTot68%2BoT7c0UBE4soOMOcRjBiyA@mail.gmail.com> <CACNAnaGO2FJeKOPXJZ3-etyQf03hvk-kuqj75hKCpg3JQD54=g@mail.gmail.com> <CAOtMX2geGiny1rDMKE7v94R33gb0NcpcJ9k%2B3TrHzsdc5DC8hQ@mail.gmail.com> <CACNAnaGDULoCpp7Rh%2B9mSfC-=uSXiHWOPh9_STpy2yA%2BtSk85A@mail.gmail.com> <CAOtMX2iPqRh6P3JSX3V0erEAzaeN1g4jCAyhTz5O3__ffsBtkA@mail.gmail.com> <CANCZdfqx%2BOboQFZ4xzCeOdNahPkNP3X8yeMD6uVSwHvv2=cinA@mail.gmail.com> <CACNAnaH-L%2Bw=SJW1Cw1M63LQi19d%2Bf_8Sv%2BdYUTyE%2BjYQU6nbA@mail.gmail.com> <CANCZdfrfOrFAkcRWbhQSW9qgyJ6JFyMrgMqrN=YS7eKoT1FmEA@mail.gmail.com> <CAOtMX2iNhii4tCwvTSn7JGNAJ_afoyjwQYM=8MUo4fitt992Kg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I think it's a great leap sideways, but I've done cp /dev/null foo to clear
it out for 35 years now... It's why it feels like a workaround.
Though it is a legit optimization, no matter the feelings. As for clearer,
I'm less sure since then I have to remember what the : operator does.
Warner
On Tue, Sep 22, 2020 at 3:48 PM Alan Somers <asomers@freebsd.org> wrote:
> It doesn't feel like a workaround to me. I think Kyle's version is
> clearer than the original.
>
> On Tue, Sep 22, 2020 at 3:45 PM Warner Losh <imp@bsdimp.com> wrote:
>
>>
>>
>> On Tue, Sep 22, 2020 at 3:42 PM Kyle Evans <kevans@freebsd.org> wrote:
>>
>>> cp is already fixed, people are still feeling the fallout of being
>>> within those revisions and needing to bootstrap their own cp. We can
>>> reduce the number of components these invocations rely on trivially to
>>> shell built-in mechanics, why not do so?
>>>
>>
>> Fair point. I just bristle at putting workarounds in for valid /bin/sh
>> syntax because we opposed for a few days. so long as it's an unconditional
>> clearing of the file to be zero length, I'm OK with that.
>>
>> Warner
>>
>>
>>> On Tue, Sep 22, 2020 at 4:40 PM Warner Losh <imp@bsdimp.com> wrote:
>>> >
>>> > So why do we need a workaround at all? cp /dev/null has been fixed,
>>> and that's way more important to get right.
>>> >
>>> > I don't want to paper-over issues with this at all, though if we use
>>> the host's (now broken) cp, I suppose that might be OK in the short term.
>>> If that's the case, then maybe this is OK.
>>> >
>>> > Otherwise, I'd strongly prefer we fix cp.
>>> >
>>> > Warner
>>> >
>>> > On Tue, Sep 22, 2020 at 3:31 PM Alan Somers <asomers@freebsd.org>
>>> wrote:
>>> >>
>>> >> +1.
>>> >>
>>> >> On Tue, Sep 22, 2020 at 3:27 PM Kyle Evans <kevans@freebsd.org>
>>> wrote:
>>> >>>
>>> >>> I'm running a build at the suggestion of mjg to confirm there aren't
>>> >>> any others hiding that can be converted, and I will commit after I've
>>> >>> verified that this is it.
>>> >>>
>>> >>> On Tue, Sep 22, 2020 at 4:02 PM Alan Somers <asomers@freebsd.org>
>>> wrote:
>>> >>> >
>>> >>> > LGTM.
>>> >>> >
>>> >>> > On Tue, Sep 22, 2020 at 2:59 PM Kyle Evans <kevans@freebsd.org>
>>> wrote:
>>> >>> >>
>>> >>> >> Perhaps:
>>> >>> >>
>>> >>> >> diff --git a/stand/i386/zfsboot/Makefile
>>> b/stand/i386/zfsboot/Makefile
>>> >>> >> index ff315abc0ef..7e362b43a39 100644
>>> >>> >> --- a/stand/i386/zfsboot/Makefile
>>> >>> >> +++ b/stand/i386/zfsboot/Makefile
>>> >>> >> @@ -81,7 +81,7 @@ zfsboot.ld: zfsboot.ldr zfsboot.bin ${BTXKERN}
>>> >>> >> -o ${.TARGET} -P 1 zfsboot.bin
>>> >>> >>
>>> >>> >> zfsboot.ldr:
>>> >>> >> - cp /dev/null ${.TARGET}
>>> >>> >> + :> ${.TARGET}
>>> >>> >>
>>> >>> >> zfsboot.bin: zfsboot.out
>>> >>> >> ${OBJCOPY} -S -O binary zfsboot.out ${.TARGET}
>>> >>> >> diff --git a/stand/libsa/Makefile b/stand/libsa/Makefile
>>> >>> >> index effece9e01b..63cd46a9c54 100644
>>> >>> >> --- a/stand/libsa/Makefile
>>> >>> >> +++ b/stand/libsa/Makefile
>>> >>> >> @@ -122,7 +122,7 @@ beforedepend:
>>> >>> >> ln -sf ${SRCTOP}/include/arpa/inet.h arpa/inet.h; \
>>> >>> >> ln -sf ${SRCTOP}/include/arpa/tftp.h arpa/tftp.h; \
>>> >>> >> for i in _time.h _strings.h _string.h; do \
>>> >>> >> - [ -f xlocale/$$i ] || cp /dev/null xlocale/$$i; \
>>> >>> >> + [ -f xlocale/$$i ] || :> xlocale/$$i; \
>>> >>> >> done; \
>>> >>> >> for i in ${STAND_H_INC}; do \
>>> >>> >> ln -sf ${SASRC}/stand.h $$i; \
>>> >>> >>
>>> >>> >>
>>> >>> >> On Tue, Sep 22, 2020 at 3:58 PM Alan Somers <asomers@freebsd.org>
>>> wrote:
>>> >>> >> >
>>> >>> >> > Looks like two places in stand. Is there any reason why
>>> Mateusz's suggestion wouldn't work?
>>> >>> >> >
>>> >>> >> > > rg -g Makefile 'cp.*/dev/null'
>>> >>> >> > stand/libsa/Makefile
>>> >>> >> > 125: [ -f xlocale/$$i ] || cp /dev/null xlocale/$$i; \
>>> >>> >> >
>>> >>> >> > stand/i386/zfsboot/Makefile
>>> >>> >> > 82: cp /dev/null ${.TARGET}
>>> >>> >> >
>>> >>> >> > On Tue, Sep 22, 2020 at 2:54 PM Mateusz Guzik <
>>> mjguzik@gmail.com> wrote:
>>> >>> >> >>
>>> >>> >> >> Can we instead add a workaround to the build tree?
>>> >>> >> >>
>>> >>> >> >> Where is cp /dev/null coming from anyway? Perhaps this can be
>>> patched
>>> >>> >> >> to touch the target file.
>>> >>> >> >>
>>> >>> >> >> On 9/22/20, Alan Somers <asomers@freebsd.org> wrote:
>>> >>> >> >> > On Tue, Sep 22, 2020 at 2:48 PM Kyle Evans <
>>> kevans@freebsd.org> wrote:
>>> >>> >> >> >
>>> >>> >> >> >> On Fri, Sep 11, 2020 at 3:49 PM Alan Somers <
>>> asomers@freebsd.org> wrote:
>>> >>> >> >> >> >
>>> >>> >> >> >> > Author: asomers
>>> >>> >> >> >> > Date: Fri Sep 11 20:49:36 2020
>>> >>> >> >> >> > New Revision: 365643
>>> >>> >> >> >> > URL: https://svnweb.freebsd.org/changeset/base/365643
>>> >>> >> >> >> >
>>> >>> >> >> >> > Log:
>>> >>> >> >> >> > cp: fall back to read/write if copy_file_range fails
>>> >>> >> >> >> >
>>> >>> >> >> >> > Even though copy_file_range has a file-system agnostic
>>> version, it
>>> >>> >> >> >> still
>>> >>> >> >> >> > fails on devfs (perhaps because the file descriptor is
>>> non-seekable?)
>>> >>> >> >> >> In
>>> >>> >> >> >> > that case, fallback to old-fashioned read/write. Fixes
>>> >>> >> >> >> > "cp /dev/null /tmp/null"
>>> >>> >> >> >> >
>>> >>> >> >> >>
>>> >>> >> >> >> Hi,
>>> >>> >> >> >>
>>> >>> >> >> >> Any objection to adding a quick UPDATING entry for this?
>>> I'm seeing
>>> >>> >> >> >> occasional reports of this breakage as recent as today on
>>> IRC from
>>> >>> >> >> >> folks that were a little bit thrown off by this because it
>>> throws up
>>> >>> >> >> >> fairly far into the build and looks like a stand build
>>> regression
>>> >>> >> >> >> instead of a cp regression.
>>> >>> >> >> >>
>>> >>> >> >> >> Thanks,
>>> >>> >> >> >>
>>> >>> >> >> >> Kyle Evans
>>> >>> >> >> >>
>>> >>> >> >> >
>>> >>> >> >> > No objection. Can you suggest the proper wording?
>>> >>> >> >> > _______________________________________________
>>> >>> >> >> > svn-src-all@freebsd.org mailing list
>>> >>> >> >> > https://lists.freebsd.org/mailman/listinfo/svn-src-all
>>> >>> >> >> > To unsubscribe, send any mail to "
>>> svn-src-all-unsubscribe@freebsd.org"
>>> >>> >> >> >
>>> >>> >> >>
>>> >>> >> >>
>>> >>> >> >> --
>>> >>> >> >> Mateusz Guzik <mjguzik gmail.com>
>>>
>>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqs8Hrmm-mHConJCW29PRHAPByTwP8J0aTR6naxNL7Ncg>