Date: Sat, 8 Jul 2023 17:29:40 -0700 From: Alexander Richardson <arichardson@freebsd.org> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Martin Matuska <mm@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: e64780fbc394 - main - xinstall: do not use copy_file_range(2) when BOOTSTRAPPING Message-ID: <CA%2BZ_v8qgXvKiAfbyLXE%2B=SpXB4ZZge0vZgaSius=Q29sc0PFkA@mail.gmail.com> In-Reply-To: <471FD215-B307-4116-9E92-4D84D6FB2B66@freebsd.org> References: <202307082348.368Nm3TN074666@gitrepo.freebsd.org> <471FD215-B307-4116-9E92-4D84D6FB2B66@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000d7a004060002f6da Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks for the quick fix. I'm not sure if it matters for performance but it is a bit too restrictive: modern Linux also has the function so it works just fine there (GitHub CI was happy with this commit on Linux just not MacOS). And as Jess pointed out we should probably use it on FreeBSD as well. Alex On Sat, 8 Jul 2023, 17:18 Jessica Clarke, <jrtc27@freebsd.org> wrote: > On 9 Jul 2023, at 00:48, Martin Matuska <mm@FreeBSD.org> wrote: > > > > The branch main has been updated by mm: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3De64780fbc394b10581e50a850cc06c1= c12a8e4f9 > > > > commit e64780fbc394b10581e50a850cc06c1c12a8e4f9 > > Author: Martin Matuska <mm@FreeBSD.org> > > AuthorDate: 2023-07-08 23:24:38 +0000 > > Commit: Martin Matuska <mm@FreeBSD.org> > > CommitDate: 2023-07-08 23:25:23 +0000 > > > > xinstall: do not use copy_file_range(2) when BOOTSTRAPPING > > > > Reported by: arichardson > > --- > > usr.bin/xinstall/Makefile | 5 +++++ > > usr.bin/xinstall/xinstall.c | 4 ++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/usr.bin/xinstall/Makefile b/usr.bin/xinstall/Makefile > > index 9969ef104e98..3b49cb39d27a 100644 > > --- a/usr.bin/xinstall/Makefile > > +++ b/usr.bin/xinstall/Makefile > > @@ -17,6 +17,11 @@ CFLAGS+=3D -I${SRCTOP}/lib/libnetbsd > > LIBADD=3D md > > CFLAGS+=3D -DWITH_MD5 -DWITH_RIPEMD160 > > > > +.ifdef BOOTSTRAPPING > > +# For the bootstrap we disable copy_file_range() > > +CFLAGS+=3D -DBOOTSTRAP_XINSTALL > > If the bootstrap xinstall is allowed to use copy_file_range on FreeBSD > (i.e. it=E2=80=99s guaranteed to exist) this is overly restrictive and sh= ould > be: > > .if defined(BOOTSTRAPPING) && ${.MAKE.OS} !=3D "FreeBSD" > > > +.endif > > + > > HAS_TESTS=3D > > SUBDIR.${MK_TESTS}+=3D tests > > > > diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c > > index 8dace862ef1e..6c269bbb5d91 100644 > > --- a/usr.bin/xinstall/xinstall.c > > +++ b/usr.bin/xinstall/xinstall.c > > @@ -1300,7 +1300,9 @@ copy(int from_fd, const char *from_name, int > to_fd, const char *to_name, > > static size_t bufsize; > > int nr, nw; > > int serrno; > > +#ifndef BOOTSTRAP_XINSTALL > > ssize_t ret; > > +#endif > > char *p; > > int done_copy; > > DIGEST_CTX ctx; > > @@ -1311,6 +1313,7 @@ copy(int from_fd, const char *from_name, int > to_fd, const char *to_name, > > if (lseek(to_fd, (off_t)0, SEEK_SET) =3D=3D (off_t)-1) > > err(EX_OSERR, "lseek: %s", to_name); > > > > +#ifndef BOOTSTRAP_XINSTALL > > /* Try copy_file_range() if no digest is requested */ > > if (digesttype =3D=3D DIGEST_NONE) { > > ret =3D 1; > > @@ -1331,6 +1334,7 @@ copy(int from_fd, const char *from_name, int > to_fd, const char *to_name, > > /* Fall back */ > > } > > > > +#endif > > This looks to be in the wrong place wrt whitespace? > > Jess > > > digest_init(&ctx); > > > > done_copy =3D 0; > > --000000000000d7a004060002f6da Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"auto"><div>Thanks for the quick fix. I'm not sure if it mat= ters for performance but it is a bit too restrictive: modern Linux also has= the function so it works just fine there (GitHub CI was happy with this co= mmit on Linux just not MacOS). And as Jess pointed out we should probably u= se it on FreeBSD as well.<div dir=3D"auto"><br></div><div dir=3D"auto">Alex= </div><br><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_at= tr">On Sat, 8 Jul 2023, 17:18 Jessica Clarke, <<a href=3D"mailto:jrtc27@= freebsd.org">jrtc27@freebsd.org</a>> wrote:<br></div><blockquote class= =3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padd= ing-left:1ex">On 9 Jul 2023, at 00:48, Martin Matuska <mm@FreeBSD.org>= ; wrote:<br> > <br> > The branch main has been updated by mm:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3De64780fbc394= b10581e50a850cc06c1c12a8e4f9" rel=3D"noreferrer noreferrer" target=3D"_blan= k">https://cgit.FreeBSD.org/src/commit/?id=3De64780fbc394b10581e50a850cc06c= 1c12a8e4f9</a><br> > <br> > commit e64780fbc394b10581e50a850cc06c1c12a8e4f9<br> > Author:=C2=A0 =C2=A0 =C2=A0Martin Matuska <mm@FreeBSD.org><br> > AuthorDate: 2023-07-08 23:24:38 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Martin Matuska <mm@FreeBSD.org><br> > CommitDate: 2023-07-08 23:25:23 +0000<br> > <br> >=C2=A0 =C2=A0 xinstall: do not use copy_file_range(2) when BOOTSTRAPPIN= G<br> > <br> >=C2=A0 =C2=A0 Reported by:=C2=A0 =C2=A0 arichardson<br> > ---<br> > usr.bin/xinstall/Makefile=C2=A0 =C2=A0| 5 +++++<br> > usr.bin/xinstall/xinstall.c | 4 ++++<br> > 2 files changed, 9 insertions(+)<br> > <br> > diff --git a/usr.bin/xinstall/Makefile b/usr.bin/xinstall/Makefile<br> > index 9969ef104e98..3b49cb39d27a 100644<br> > --- a/usr.bin/xinstall/Makefile<br> > +++ b/usr.bin/xinstall/Makefile<br> > @@ -17,6 +17,11 @@ CFLAGS+=3D -I${SRCTOP}/lib/libnetbsd<br> > LIBADD=3D md<br> > CFLAGS+=3D -DWITH_MD5 -DWITH_RIPEMD160<br> > <br> > +.ifdef BOOTSTRAPPING<br> > +# For the bootstrap we disable copy_file_range()<br> > +CFLAGS+=3D -DBOOTSTRAP_XINSTALL<br> <br> If the bootstrap xinstall is allowed to use copy_file_range on FreeBSD<br> (i.e. it=E2=80=99s guaranteed to exist) this is overly restrictive and shou= ld<br> be:<br> <br> .if defined(BOOTSTRAPPING) && ${.MAKE.OS} !=3D "FreeBSD"<= br> <br> > +.endif<br> > +<br> > HAS_TESTS=3D<br> > SUBDIR.${MK_TESTS}+=3D tests<br> > <br> > diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c= <br> > index 8dace862ef1e..6c269bbb5d91 100644<br> > --- a/usr.bin/xinstall/xinstall.c<br> > +++ b/usr.bin/xinstall/xinstall.c<br> > @@ -1300,7 +1300,9 @@ copy(int from_fd, const char *from_name, int to_= fd, const char *to_name,<br> > static size_t bufsize;<br> > int nr, nw;<br> > int serrno;<br> > +#ifndef BOOTSTRAP_XINSTALL<br> > ssize_t ret;<br> > +#endif<br> > char *p;<br> > int done_copy;<br> > DIGEST_CTX ctx;<br> > @@ -1311,6 +1313,7 @@ copy(int from_fd, const char *from_name, int to_= fd, const char *to_name,<br> > if (lseek(to_fd, (off_t)0, SEEK_SET) =3D=3D (off_t)-1)<br> > err(EX_OSERR, "lseek: %s", to_name);<br> > <br> > +#ifndef BOOTSTRAP_XINSTALL<br> > /* Try copy_file_range() if no digest is requested */<br> > if (digesttype =3D=3D DIGEST_NONE) {<br> > ret =3D 1;<br> > @@ -1331,6 +1334,7 @@ copy(int from_fd, const char *from_name, int to_= fd, const char *to_name,<br> > /* Fall back */<br> > }<br> > <br> > +#endif<br> <br> This looks to be in the wrong place wrt whitespace?<br> <br> Jess<br> <br> > digest_init(&ctx);<br> > <br> > done_copy =3D 0;<br> <br> </blockquote></div></div></div> --000000000000d7a004060002f6da--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2BZ_v8qgXvKiAfbyLXE%2B=SpXB4ZZge0vZgaSius=Q29sc0PFkA>