Issue 76810045: code review 76810045: cmd/ld: don't delete output binary if not "ordinary" file. (Closed)
Descriptioncmd/ld: don't delete output binary if not "ordinary" file.
e.g., don't delete /dev/null. this fix inspired by gnu libiberty,
unlink-if-ordinary.c.
Fixes issue 7563
Patch Set 1 #Patch Set 2 : diff -r 9cd3a4758b94 https://code.google.com/p/go #Patch Set 3 : diff -r 9cd3a4758b94 https://code.google.com/p/go #
Total comments: 2
Patch Set 4 : diff -r 8645daeb2f82 https://code.google.com/p/go #Patch Set 5 : diff -r b1f6ff3c9f0e https://code.google.com/p/go #
Total comments: 1
Patch Set 6 : diff -r b1f6ff3c9f0e https://code.google.com/p/go #MessagesTotal messages: 19
Hello golang-codereviews@googlegroups.com (cc: iant@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
my apologies, this new CL just replaces https://codereview.appspot.com/76580044 which seems to have gotten into a state where i can't edit anymore? iant: i've made your three suggested changes there, thanks. On Fri, Mar 21, 2014 at 8:12 PM, <mra@xoba.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com (cc: iant@golang.org), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > cmd/ld: don't delete output binary if not "ordinary" file. > > e.g., don't delete /dev/null. this fix inspired by gnu libiberty, > unlink-if-ordinary.c. > > Fixes issue 7563 > > Please review this at https://codereview.appspot.com/76810045/ > > Affected files (+6, -1 lines): > M src/cmd/ld/lib.c > > > Index: src/cmd/ld/lib.c > =================================================================== > --- a/src/cmd/ld/lib.c > +++ b/src/cmd/ld/lib.c > @@ -37,6 +37,7 @@ > #include "../../pkg/runtime/funcdata.h" > > #include <ar.h> > +#include <sys/stat.h> > > enum > { > @@ -107,7 +108,11 @@ > // recently run) binary, so remove the output file before writing > it. > // On Windows 7, remove() can force the following create() to fail. > #ifndef _WIN32 > - remove(outfile); > + { > + struct stat st; > + if(lstat(outfile, &st) == 0 && (S_ISREG(st.st_mode) || > S_ISLNK(st.st_mode))) > + remove(outfile); > + } > #endif > cout = create(outfile, 1, 0775); > if(cout < 0) { > > >
Sign in to reply to this message.
https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c File src/cmd/ld/lib.c (right): https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c#newcode110 src/cmd/ld/lib.c:110: #ifndef _WIN32 this is linux-specific, not anti-windows. https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c#newcode113 src/cmd/ld/lib.c:113: if(lstat(outfile, &st) == 0 && (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) this looks wrong. if it's a symlink, i don't think it should be removed either.
Sign in to reply to this message.
On Sat, Mar 22, 2014 at 3:18 PM, <r@golang.org> wrote: > > https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c > File src/cmd/ld/lib.c (right): > > https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c#newcode110 > src/cmd/ld/lib.c:110: #ifndef _WIN32 > this is linux-specific, not anti-windows. No, it's Unix in general. On Unix it's generally unwise to remove a non-regular file. It's better to let open fail or succeed as desired. > https://codereview.appspot.com/76810045/diff/40001/src/cmd/ld/lib.c#newcode113 > src/cmd/ld/lib.c:113: if(lstat(outfile, &st) == 0 && > (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))) > this looks wrong. if it's a symlink, i don't think it should be removed > either. This was my suggestion, because it is what the GNU binutils do. But interestingly when it was added to the GNU binutils, I said it should act the same as GCC (https://sourceware.org/ml/binutils/2004-10/msg00077.html) but GCC does *not* delete a symlink. I never noticed that Jan changed the meaning from how GCC behaves. So I agree: this should only test S_ISREG. Ian
Sign in to reply to this message.
By Linux-specific, I meant Unix-specific. What I really meant is that it won't work on Plan 9 - there's no such thing as a symbolic link there. -rob
Sign in to reply to this message.
(More generally, none of these constants are defined there.) -rob
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org (cc: golang-codereviews@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
On 2014/03/23 22:12:49, mra wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org (cc: > mailto:golang-codereviews@googlegroups.com, mailto:r@golang.org), > > Please take another look. my understanding of this email thread is that we just need to remove the S_ISREG test, so that's what i did, but i'm happy to make any other changes folks want too.
Sign in to reply to this message.
On 2014/03/23 22:15:15, mra wrote: > On 2014/03/23 22:12:49, mra wrote: > > Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org (cc: > > mailto:golang-codereviews@googlegroups.com, mailto:r@golang.org), > > > > Please take another look. > > my understanding of this email thread is that we just need to remove the S_ISREG > test, so that's what i did, but i'm happy to make any other changes folks want > too. sorry, my mistake, i meant "just need to remove S_ISLNK test", not S_ISREG, which is what i did.
Sign in to reply to this message.
The issue here is that neither sys/stat.h, nor lstat(), nor S_ISREG() exist on Plan 9. So it should be more fine grained than just "not Windows". An ifdef will also be required around the inclusion of the header file.
Sign in to reply to this message.
sounds right to me, thanks 0intro, i will have to get up to speed on plan 9 first, create a VM and test there as well, after incorporating your suggestions... but if folks can't wait for that, and i'm not sure scheduling-wise yet when it'd be, i'd be happy to transfer the CL to anybody who wants to take over in the meantime? best regards, mike. On Sun, Mar 23, 2014 at 6:26 PM, <0intro@gmail.com> wrote: > The issue here is that neither sys/stat.h, nor lstat(), nor S_ISREG() > exist on Plan 9. So it should be more fine grained than just "not > Windows". An ifdef will also be required around the inclusion of the > header file. > > https://codereview.appspot.com/76810045/ >
Sign in to reply to this message.
On Tue, Mar 25, 2014 at 5:36 PM, Mike Andrews <mra@xoba.com> wrote: > sounds right to me, thanks 0intro, i will have to get up to speed on plan 9 > first, create a VM and test there as well, after incorporating your > suggestions... but if folks can't wait for that, and i'm not sure > scheduling-wise yet when it'd be, i'd be happy to transfer the CL to anybody > who wants to take over in the meantime? best regards, mike. I think you can just #ifndef PLAN9. Ian
Sign in to reply to this message.
> sounds right to me, thanks 0intro, i will have to get up to speed on > plan 9 first, create a VM and test there as well, after incorporating > your suggestions... but if folks can't wait for that, and i'm not sure > scheduling-wise yet when it'd be, i'd be happy to transfer the CL to > anybody who wants to take over in the meantime? best regards, mike. No need to mess with Plan 9. Just make the changes and I will test for you. I believe #ifndef _WIN32 and PLAN9 should be enough. -- David du Colombier
Sign in to reply to this message.
thanks, will do. On Wed, Mar 26, 2014 at 2:15 AM, David du Colombier <0intro@gmail.com>wrote: > > sounds right to me, thanks 0intro, i will have to get up to speed on > > plan 9 first, create a VM and test there as well, after incorporating > > your suggestions... but if folks can't wait for that, and i'm not sure > > scheduling-wise yet when it'd be, i'd be happy to transfer the CL to > > anybody who wants to take over in the meantime? best regards, mike. > > No need to mess with Plan 9. Just make the changes > and I will test for you. > > I believe #ifndef _WIN32 and PLAN9 should be enough. > > -- > David du Colombier >
Sign in to reply to this message.
David (0intro), i've uploaded a new patch set that works on linux amd64, so could you let me know if it also works on plan9? thanks! best regards, mike On 2014/03/26 13:42:54, mra wrote: > On Wed, Mar 26, 2014 at 2:15 AM, David du Colombier <mailto:0intro@gmail.com>wrote: > > > No need to mess with Plan 9. Just make the changes > > and I will test for you. > > > > I believe #ifndef _WIN32 and PLAN9 should be enough. > > > > -- > > David du Colombier
Sign in to reply to this message.
It seems fine on Plan 9. https://codereview.appspot.com/76810045/diff/80001/src/cmd/ld/lib.c File src/cmd/ld/lib.c (right): https://codereview.appspot.com/76810045/diff/80001/src/cmd/ld/lib.c#newcode40 src/cmd/ld/lib.c:40: #ifndef PLAN9 We also don't want to include sys/stat.h on Windows. Please replace with: #if !(defined(_WIN32) || defined(PLAN9))
Sign in to reply to this message.
thanks, i made that change, PTAL? also, do we need to test or #if on any other specific platforms as well? best, mike. On Sat, Mar 29, 2014 at 9:42 AM, <0intro@gmail.com> wrote: > It seems fine on Plan 9. > > > https://codereview.appspot.com/76810045/diff/80001/src/cmd/ld/lib.c > File src/cmd/ld/lib.c (right): > > https://codereview.appspot.com/76810045/diff/80001/src/ > cmd/ld/lib.c#newcode40 > src/cmd/ld/lib.c:40: #ifndef PLAN9 > We also don't want to include sys/stat.h on Windows. Please replace > with: > #if !(defined(_WIN32) || defined(PLAN9)) > > https://codereview.appspot.com/76810045/ >
Sign in to reply to this message.
LGTM Thanks.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=dee4c7a24398 *** cmd/ld: don't delete output binary if not "ordinary" file. e.g., don't delete /dev/null. this fix inspired by gnu libiberty, unlink-if-ordinary.c. Fixes issue 7563 LGTM=iant R=golang-codereviews, iant, 0intro CC=golang-codereviews, r https://codereview.appspot.com/76810045 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
|
