internal/poll: revise the determination about [handled] and improve the code readabil...

There were a bit too many conditional branches in the old code,
resulting in a poor readability. It could be more concise by reducing
and consolidating some of the conditions.

Furthermore, how we've determined whether or not the data transimission
was handled by sendfile(2) seems inappropriate, because it marked the
operation as unhandled whenever any non-retryable error occurs from
calling sendfile(2), it doesn't look like a right approach, at least
this is an inconsistent behavior with what we've done in Splice.

Related to #64044

Change-Id: Ieb65e0879a8841654d0e64a1263a4e43179df1ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/537275
TryBot-Result: Gopher Robot <gobot@golang.org>
Commit-Queue: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Andy Pan <panjf2000@gmail.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/internal/poll/sendfile_bsd.go b/src/internal/poll/sendfile_bsd.go
index 0f55cad73dc3ee7151cb8c5cc5b9addbacf573a9..8fcdb1c22e602336af5fa301bad8e5962d7f8360 100644 (file)
--- a/src/internal/poll/sendfile_bsd.go
+++ b/src/internal/poll/sendfile_bsd.go
@@ -13,51 +13,44 @@ import "syscall"
 const maxSendfileSize int = 4 << 20
 
 // SendFile wraps the sendfile system call.
-func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) {
+func SendFile(dstFD *FD, src int, pos, remain int64) (written int64, err error, handled bool) {
        if err := dstFD.writeLock(); err != nil {
                return 0, err, false
        }
        defer dstFD.writeUnlock()
+
        if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil {
                return 0, err, false
        }
 
        dst := dstFD.Sysfd
-       var (
-               written int64
-               err     error
-               handled = true
-       )
        for remain > 0 {
                n := maxSendfileSize
                if int64(n) > remain {
                        n = int(remain)
                }
                pos1 := pos
-               n, err1 := syscall.Sendfile(dst, src, &pos1, n)
+               n, err = syscall.Sendfile(dst, src, &pos1, n)
                if n > 0 {
                        pos += int64(n)
                        written += int64(n)
                        remain -= int64(n)
-               } else if n == 0 && err1 == nil {
-                       break
                }
-               if err1 == syscall.EINTR {
+               if err == syscall.EINTR {
                        continue
                }
-               if err1 == syscall.EAGAIN {
-                       if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil {
-                               continue
-                       }
+               // This includes syscall.ENOSYS (no kernel
+               // support) and syscall.EINVAL (fd types which
+               // don't implement sendfile), and other errors.
+               // We should end the loop when there is no error
+               // returned from sendfile(2) or it is not a retryable error.
+               if err != syscall.EAGAIN {
+                       break
                }
-               if err1 != nil {
-                       // This includes syscall.ENOSYS (no kernel
-                       // support) and syscall.EINVAL (fd types which
-                       // don't implement sendfile)
-                       err = err1
-                       handled = false
+               if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil {
                        break
                }
        }
-       return written, err, handled
+       handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL)
+       return
 }

diff --git a/src/internal/poll/sendfile_linux.go b/src/internal/poll/sendfile_linux.go
index cc31969a43f9f8737e31f5a110a4c7aaf32405bd..c2a065329431ecbb6ab3f3b2f7619bf67bb078c4 100644 (file)
--- a/src/internal/poll/sendfile_linux.go
+++ b/src/internal/poll/sendfile_linux.go
@@ -11,49 +11,42 @@ import "syscall"
 const maxSendfileSize int = 4 << 20
 
 // SendFile wraps the sendfile system call.
-func SendFile(dstFD *FD, src int, remain int64) (int64, error, bool) {
+func SendFile(dstFD *FD, src int, remain int64) (written int64, err error, handled bool) {
        if err := dstFD.writeLock(); err != nil {
                return 0, err, false
        }
        defer dstFD.writeUnlock()
+
        if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil {
                return 0, err, false
        }
 
        dst := dstFD.Sysfd
-       var (
-               written int64
-               err     error
-               handled = true
-       )
        for remain > 0 {
                n := maxSendfileSize
                if int64(n) > remain {
                        n = int(remain)
                }
-               n, err1 := syscall.Sendfile(dst, src, nil, n)
+               n, err = syscall.Sendfile(dst, src, nil, n)
                if n > 0 {
                        written += int64(n)
                        remain -= int64(n)
-               } else if n == 0 && err1 == nil {
+                       continue
+               } else if err != syscall.EAGAIN && err != syscall.EINTR {
+                       // This includes syscall.ENOSYS (no kernel
+                       // support) and syscall.EINVAL (fd types which
+                       // don't implement sendfile), and other errors.
+                       // We should end the loop when there is no error
+                       // returned from sendfile(2) or it is not a retryable error.
                        break
                }
-               if err1 == syscall.EINTR {
+               if err == syscall.EINTR {
                        continue
                }
-               if err1 == syscall.EAGAIN {
-                       if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil {
-                               continue
-                       }
-               }
-               if err1 != nil {
-                       // This includes syscall.ENOSYS (no kernel
-                       // support) and syscall.EINVAL (fd types which
-                       // don't implement sendfile)
-                       err = err1
-                       handled = false
+               if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil {
                        break
                }
        }
-       return written, err, handled
+       handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL)
+       return
 }

diff --git a/src/internal/poll/sendfile_solaris.go b/src/internal/poll/sendfile_solaris.go
index f9f685c64a6459da5f2797d13637671d6deffb07..1ba0c8d0643bce8325e3397f4ca0b31e3f7783e4 100644 (file)
--- a/src/internal/poll/sendfile_solaris.go
+++ b/src/internal/poll/sendfile_solaris.go
@@ -16,29 +16,25 @@ import "syscall"
 const maxSendfileSize int = 4 << 20
 
 // SendFile wraps the sendfile system call.
-func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) {
+func SendFile(dstFD *FD, src int, pos, remain int64) (written int64, err error, handled bool) {
        if err := dstFD.writeLock(); err != nil {
                return 0, err, false
        }
        defer dstFD.writeUnlock()
+
        if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil {
                return 0, err, false
        }
 
        dst := dstFD.Sysfd
-       var (
-               written int64
-               err     error
-               handled = true
-       )
        for remain > 0 {
                n := maxSendfileSize
                if int64(n) > remain {
                        n = int(remain)
                }
                pos1 := pos
-               n, err1 := syscall.Sendfile(dst, src, &pos1, n)
-               if err1 == syscall.EAGAIN || err1 == syscall.EINTR {
+               n, err = syscall.Sendfile(dst, src, &pos1, n)
+               if err == syscall.EAGAIN || err == syscall.EINTR {
                        // partial write may have occurred
                        n = int(pos1 - pos)
                }
@@ -46,25 +42,22 @@ func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) {
                        pos += int64(n)
                        written += int64(n)
                        remain -= int64(n)
-               } else if n == 0 && err1 == nil {
+                       continue
+               } else if err != syscall.EAGAIN && err != syscall.EINTR {
+                       // This includes syscall.ENOSYS (no kernel
+                       // support) and syscall.EINVAL (fd types which
+                       // don't implement sendfile), and other errors.
+                       // We should end the loop when there is no error
+                       // returned from sendfile(2) or it is not a retryable error.
                        break
                }
-               if err1 == syscall.EAGAIN {
-                       if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil {
-                               continue
-                       }
-               }
-               if err1 == syscall.EINTR {
+               if err == syscall.EINTR {
                        continue
                }
-               if err1 != nil {
-                       // This includes syscall.ENOSYS (no kernel
-                       // support) and syscall.EINVAL (fd types which
-                       // don't implement sendfile)
-                       err = err1
-                       handled = false
+               if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil {
                        break
                }
        }
-       return written, err, handled
+       handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL)
+       return
 }