code review 76580044: src/cmd/ld/lib.c: don't delete output binary if not "or...

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

[mra, 2014-03-17 16:16:03]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[mra, 2014-03-17 16:20:31]

thanks in advance, and my motivation for submitting this is that when
editing golang code in emacs as root user (as i usually
do), misc/emacs/go-mode.el causes deletion of /dev/null. this was
completely unexpected and unusual to encounter. best regards, mike.


On Mon, Mar 17, 2014 at 12:16 PM, <mra@xoba.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc:
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> src/cmd/ld/lib.c: don't delete /dev/null if that's the binary's output
> path
>
> Fixes issue 7563
>
> Please review this at https://codereview.appspot.com/76580044/
>
> Affected files (+3, -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
> @@ -107,7 +107,9 @@
>         // 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);
> +       if (strcmp(outfile, "/dev/null") != 0) { // please, never remove
> /dev/null :-)
> +               remove(outfile);
> +       }
>  #endif
>         cout = create(outfile, 1, 0775);
>         if(cout < 0) {
>
>
>

[iant, 2014-03-17 17:58:26]

Thanks, but this is not the right fix.  I outlined the right fix in the bug
report.

[mra, 2014-03-17 19:50:08]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[mra, 2014-03-20 21:39:31]

i forgot to say PTAL, thanks, & i'll be happy to revise as folks think best.

On 2014/03/17 19:50:08, mra wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org (cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

[iant, 2014-03-20 22:41:51]

The CL description should start with just cmd/ld (you can change it with "hg
change", or using the web interface).

https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111
src/cmd/ld/lib.c:111: struct stat st;
Please start a new block so that the local variable is in a block by itself.  In
this code we don't assume C99.

https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112
src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG (st.st_mode) ||
S_ISLNK (st.st_mode)))
No space before left parenthesis, four times.

[mra, 2014-03-22 00:14:48]

my apologies, i'm abandoning this CL and have replaced it with
https://codereview.appspot.com/76810045 because it seems this one can not be
edited anymore?

On 2014/03/20 22:41:51, iant wrote:
> The CL description should start with just cmd/ld (you can change it with "hg
> change", or using the web interface).
> 
> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c
> File src/cmd/ld/lib.c (right):
> 
> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111
> src/cmd/ld/lib.c:111: struct stat st;
> Please start a new block so that the local variable is in a block by itself. 
In
> this code we don't assume C99.
> 
> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112
> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG (st.st_mode)
||
> S_ISLNK (st.st_mode)))
> No space before left parenthesis, four times.

[dave_cheney.net, 2014-03-22 10:18:24]

hg change -d 6810045 <https://codereview.appspot.com/76810045>


On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote:

> my apologies, i'm abandoning this CL and have replaced it with
> https://codereview.appspot.com/76810045 because it seems this one can
> not be edited anymore?
>
>
> On 2014/03/20 22:41:51, iant wrote:
>
>> The CL description should start with just cmd/ld (you can change it
>>
> with "hg
>
>> change", or using the web interface).
>>
>
>  https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c
>> File src/cmd/ld/lib.c (right):
>>
>
>
> https://codereview.appspot.com/76580044/diff/70001/src/
> cmd/ld/lib.c#newcode111
>
>> src/cmd/ld/lib.c:111: struct stat st;
>> Please start a new block so that the local variable is in a block by
>>
> itself.  In
>
>> this code we don't assume C99.
>>
>
>
> https://codereview.appspot.com/76580044/diff/70001/src/
> cmd/ld/lib.c#newcode112
>
>> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG
>>
> (st.st_mode) ||
>
>> S_ISLNK (st.st_mode)))
>> No space before left parenthesis, four times.
>>
>
>
> https://codereview.appspot.com/76580044/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[dave_cheney.net, 2014-03-22 10:19:17]

I'm sorry, the correct command is

hg change -d 76580044

On Sat, Mar 22, 2014 at 9:18 PM, Dave Cheney <dave@cheney.net> wrote:
> hg change -d 6810045
>
>
> On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote:
>>
>> my apologies, i'm abandoning this CL and have replaced it with
>> https://codereview.appspot.com/76810045 because it seems this one can
>> not be edited anymore?
>>
>>
>> On 2014/03/20 22:41:51, iant wrote:
>>>
>>> The CL description should start with just cmd/ld (you can change it
>>
>> with "hg
>>>
>>> change", or using the web interface).
>>
>>
>>> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c
>>> File src/cmd/ld/lib.c (right):
>>
>>
>>
>>
>>
https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111
>>>
>>> src/cmd/ld/lib.c:111: struct stat st;
>>> Please start a new block so that the local variable is in a block by
>>
>> itself.  In
>>>
>>> this code we don't assume C99.
>>
>>
>>
>>
>>
https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112
>>>
>>> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG
>>
>> (st.st_mode) ||
>>>
>>> S_ISLNK (st.st_mode)))
>>> No space before left parenthesis, four times.
>>
>>
>>
>> https://codereview.appspot.com/76580044/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>
>

[mra, 2014-03-22 11:57:32]

thanks, i did try to delete it yesterday, but got the error "abort: cannot
change non-local CL 76580044", even after running "hg clpatch 76580044" on
a fresh repository, which also gave an error: "abort: codereview issue
76580044 has no diff". so, i then started to suspect that somehow the CL
got into an unusual state preventing further edits or deletion? perhaps
some sort of mistake i made earlier removed the diff per se, causing this?


On Sat, Mar 22, 2014 at 6:19 AM, Dave Cheney <dave@cheney.net> wrote:

> I'm sorry, the correct command is
>
> hg change -d 76580044
>
> On Sat, Mar 22, 2014 at 9:18 PM, Dave Cheney <dave@cheney.net> wrote:
> > hg change -d 6810045
> >
> >
> > On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote:
> >>
> >> my apologies, i'm abandoning this CL and have replaced it with
> >> https://codereview.appspot.com/76810045 because it seems this one can
> >> not be edited anymore?
> >>
> >>
> >> On 2014/03/20 22:41:51, iant wrote:
> >>>
> >>> The CL description should start with just cmd/ld (you can change it
> >>
> >> with "hg
> >>>
> >>> change", or using the web interface).
> >>
> >>
> >>> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c
> >>> File src/cmd/ld/lib.c (right):
> >>
> >>
> >>
> >>
> >>
>
https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111
> >>>
> >>> src/cmd/ld/lib.c:111: struct stat st;
> >>> Please start a new block so that the local variable is in a block by
> >>
> >> itself.  In
> >>>
> >>> this code we don't assume C99.
> >>
> >>
> >>
> >>
> >>
>
https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112
> >>>
> >>> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG
> >>
> >> (st.st_mode) ||
> >>>
> >>> S_ISLNK (st.st_mode)))
> >>> No space before left parenthesis, four times.
> >>
> >>
> >>
> >> https://codereview.appspot.com/76580044/
> >>
> >> --
> >> You received this message because you are subscribed to the Google
> Groups
> >> "golang-codereviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send
> an
> >> email to golang-codereviews+unsubscribe@googlegroups.com.
> >> For more options, visit https://groups.google.com/d/optout.
> >
> >
>

[rsc, 2014-04-17 02:30:48]

R=close