[dev.cc] code review 179130043: runtime: fix error handling in file operations on darwin

runtime: fix error handling in file operations on darwin
Currently it's impossible to understand whether a file operation
succeeds or not, because functions always return positive values.
Discovered while working on the tracing functionality.
The traced ended up not needing it, but I think it's still worth fixing.

[dvyukov, 2014-11-21 17:45:25]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to the dev.cc branch of
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2014-11-21 18:38:23]

After Go 1.4, of course. But you might want to file a bug so this isn't
lost in the hg->git move.

Well, we'll probably want to manually go over
https://go-dev.appspot.com/#all and make sure we move all pending CLs there
to git, or nag the authors to.


On Fri, Nov 21, 2014 at 9:45 AM, dvyukov via golang-codereviews <
golang-codereviews@googlegroups.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to the dev.cc branch of
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: fix error handling in file operations on darwin
> Currently it's impossible to understand whether a file operation
> succeeds or not, because functions always return positive values.
> Discovered while working on the tracing functionality.
> The traced ended up not needing it, but I think it's still worth fixing.
>
> Please review this at https://codereview.appspot.com/179130043/
>
> Affected files (+9, -0 lines):
>   M src/runtime/sys_darwin_amd64.s
>
>
> Index: src/runtime/sys_darwin_amd64.s
> ===================================================================
> --- a/src/runtime/sys_darwin_amd64.s
> +++ b/src/runtime/sys_darwin_amd64.s
> @@ -38,6 +38,9 @@
>         MOVL    perm+12(FP), DX         // arg 3 mode
>         MOVL    $(0x2000000+5), AX      // syscall entry
>         SYSCALL
> +       JCC     ok
> +       NEGQ    AX
> +ok:
>         MOVL    AX, ret+16(FP)
>         RET
>
> @@ -54,6 +57,9 @@
>         MOVL    n+16(FP), DX            // arg 3 count
>         MOVL    $(0x2000000+3), AX      // syscall entry
>         SYSCALL
> +       JCC     ok
> +       NEGQ    AX
> +ok:
>         MOVL    AX, ret+24(FP)
>         RET
>
> @@ -63,6 +69,9 @@
>         MOVL    n+16(FP), DX            // arg 3 count
>         MOVL    $(0x2000000+4), AX      // syscall entry
>         SYSCALL
> +       JCC     ok
> +       NEGQ    AX
> +ok:
>         MOVL    AX, ret+24(FP)
>         RET
>
>
>
> --
> 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.
>

[dvyukov, 2014-11-22 07:02:54]

This change is for dev.cc branch, the branch is not frozen for release.

On Fri, Nov 21, 2014 at 9:38 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> After Go 1.4, of course. But you might want to file a bug so this isn't lost
> in the hg->git move.
>
> Well, we'll probably want to manually go over
> https://go-dev.appspot.com/#all and make sure we move all pending CLs there
> to git, or nag the authors to.
>
>
> On Fri, Nov 21, 2014 at 9:45 AM, dvyukov via golang-codereviews
> <golang-codereviews@googlegroups.com> wrote:
>>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to the dev.cc branch of
>> https://dvyukov%40google.com@code.google.com/p/go/
>>
>>
>> Description:
>> runtime: fix error handling in file operations on darwin
>> Currently it's impossible to understand whether a file operation
>> succeeds or not, because functions always return positive values.
>> Discovered while working on the tracing functionality.
>> The traced ended up not needing it, but I think it's still worth fixing.
>>
>> Please review this at https://codereview.appspot.com/179130043/
>>
>> Affected files (+9, -0 lines):
>>   M src/runtime/sys_darwin_amd64.s
>>
>>
>> Index: src/runtime/sys_darwin_amd64.s
>> ===================================================================
>> --- a/src/runtime/sys_darwin_amd64.s
>> +++ b/src/runtime/sys_darwin_amd64.s
>> @@ -38,6 +38,9 @@
>>         MOVL    perm+12(FP), DX         // arg 3 mode
>>         MOVL    $(0x2000000+5), AX      // syscall entry
>>         SYSCALL
>> +       JCC     ok
>> +       NEGQ    AX
>> +ok:
>>         MOVL    AX, ret+16(FP)
>>         RET
>>
>> @@ -54,6 +57,9 @@
>>         MOVL    n+16(FP), DX            // arg 3 count
>>         MOVL    $(0x2000000+3), AX      // syscall entry
>>         SYSCALL
>> +       JCC     ok
>> +       NEGQ    AX
>> +ok:
>>         MOVL    AX, ret+24(FP)
>>         RET
>>
>> @@ -63,6 +69,9 @@
>>         MOVL    n+16(FP), DX            // arg 3 count
>>         MOVL    $(0x2000000+4), AX      // syscall entry
>>         SYSCALL
>> +       JCC     ok
>> +       NEGQ    AX
>> +ok:
>>         MOVL    AX, ret+24(FP)
>>         RET
>>
>>
>>
>>
>> --
>> 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.
>
>

[minux, 2014-11-22 07:24:07]

In general, all the functions in sys_$GOOS_$GOARCH.s only checks for error
if their callers need to. (None of the runtime.open function checks for
error, for
example.)

And as the existing callers don't check for errors and the tracing
functionality
also don't need that, I don't think we need to add these checks.

[bradfitz, 2014-11-22 08:13:25]

Oh, I missed that before, and I was even looking for it. I guess I'm
already getting confused by seeing a new code review template coming in
(for Gerrit).



On Fri, Nov 21, 2014 at 11:02 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> This change is for dev.cc branch, the branch is not frozen for release.
>
> On Fri, Nov 21, 2014 at 9:38 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > After Go 1.4, of course. But you might want to file a bug so this isn't
> lost
> > in the hg->git move.
> >
> > Well, we'll probably want to manually go over
> > https://go-dev.appspot.com/#all and make sure we move all pending CLs
> there
> > to git, or nag the authors to.
> >
> >
> > On Fri, Nov 21, 2014 at 9:45 AM, dvyukov via golang-codereviews
> > <golang-codereviews@googlegroups.com> wrote:
> >>
> >> Reviewers: golang-codereviews,
> >>
> >> Message:
> >> Hello golang-codereviews@googlegroups.com,
> >>
> >> I'd like you to review this change to the dev.cc branch of
> >> https://dvyukov%40google.com@code.google.com/p/go/
> >>
> >>
> >> Description:
> >> runtime: fix error handling in file operations on darwin
> >> Currently it's impossible to understand whether a file operation
> >> succeeds or not, because functions always return positive values.
> >> Discovered while working on the tracing functionality.
> >> The traced ended up not needing it, but I think it's still worth fixing.
> >>
> >> Please review this at https://codereview.appspot.com/179130043/
> >>
> >> Affected files (+9, -0 lines):
> >>   M src/runtime/sys_darwin_amd64.s
> >>
> >>
> >> Index: src/runtime/sys_darwin_amd64.s
> >> ===================================================================
> >> --- a/src/runtime/sys_darwin_amd64.s
> >> +++ b/src/runtime/sys_darwin_amd64.s
> >> @@ -38,6 +38,9 @@
> >>         MOVL    perm+12(FP), DX         // arg 3 mode
> >>         MOVL    $(0x2000000+5), AX      // syscall entry
> >>         SYSCALL
> >> +       JCC     ok
> >> +       NEGQ    AX
> >> +ok:
> >>         MOVL    AX, ret+16(FP)
> >>         RET
> >>
> >> @@ -54,6 +57,9 @@
> >>         MOVL    n+16(FP), DX            // arg 3 count
> >>         MOVL    $(0x2000000+3), AX      // syscall entry
> >>         SYSCALL
> >> +       JCC     ok
> >> +       NEGQ    AX
> >> +ok:
> >>         MOVL    AX, ret+24(FP)
> >>         RET
> >>
> >> @@ -63,6 +69,9 @@
> >>         MOVL    n+16(FP), DX            // arg 3 count
> >>         MOVL    $(0x2000000+4), AX      // syscall entry
> >>         SYSCALL
> >> +       JCC     ok
> >> +       NEGQ    AX
> >> +ok:
> >>         MOVL    AX, ret+24(FP)
> >>         RET
> >>
> >>
> >>
> >>
> >> --
> >> 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.
> >
> >
>

[gobot, 2014-12-19 05:16:42]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/179130043 is best) in the description in your
new CL.

Thanks very much.