ronantakizawa/codereview-bench huggingface dataset:
Repository: containerd/containerd
File: cmd/ctr/commands/images/pull.go (Go)

Code before:
		for {
			select {
			case p := <-pc:
				if p.Name == "" {
					status = p.Event
					continue
				}
				if node, ok := statuses[p.Name]; !ok {
					node = &progressNode{
						Progress: p,
						root:     true,
					}

					if len(p.Parents) == 0 {
						roots = append(roots, node)
					} else {
						var parents []string
						for _, parent := range p.Parents {
							pStatus, ok := statuses[parent]
							if ok {
								parents = append(parents, parent)
								pStatus.children = append(pStatus.children, node)
								node.root = false
							}
						}
						node.Parents = parents
						if node.root {
							roots = append(roots, node)
						}
					}
					statuses[p.Name] = node
				} else {
					if len(node.Parents) != len(p.Parents) {
						var parents []string
						var removeRoot bool
						for _, parent := range p.Parents {
							pStatus, ok := statuses[parent]
							if ok {
								parents = append(parents, parent)
								var found bool
								for _, child := range pStatus.children {

									if child.Name == p.Name {
										found = true
										break
									}
								}
								if !found {
									pStatus.children = append(pStatus.children, node)

								}

Review comment (suggestion):
FWIW; I'm a bit on the fence on the `QF1008` one ("Omit embedded fields from selector expression"); while there are cases where it's OK to directly access an embedded field, I think it's not a one-size-fits-all rule; in some cases it's better to be explicit. In moby/sys we decided to disable that specific "quick fix", and we'll likely do the same in moby (docker) once we upgrade to golangci-lint 2.x; https://github.com/moby/sys/blob/c9593b2d4e2ac2478a7d1e1cdaa08436a89ca079/.golangci.yml#L14-L20



Code after:
		case <-ctx.Done():
		}
	}

	done := func() {
		cancel()
		<-closeC
	}
	go func() {
		defer close(closeC)
		for {
			select {
			case p := <-pc:
				if p.Name == "" {
					status = p.Event
					continue
				}
				if node, ok := statuses[p.Name]; !ok {
					node = &progressNode{
						Progress: p,
						root:     true,
					}

					if len(p.Parents) == 0 {
						roots = append(roots, node)
					} else {
						var parents []string
						for _, parent := range p.Parents {
							pStatus, ok := statuses[parent]
							if ok {
								parents = append(parents, parent)
								pStatus.children = append(pStatus.children, node)
								node.root = false
							}
						}
						node.Progress.Parents = parents
						if node.root {
							roots = append(roots, node)
						}
					}
					statuses[p.Name] = node
				} else {
					if len(node.Progress.Parents) != len(p.Parents) {
						var parents []string
						var removeRoot bool
						for _, parent := range p.Parents {
							pStatus, ok := statuses[parent]
							if ok {
								parents = append(parents, parent)
								var found bool
								for _, child := range pStatus.children {
Repository: containerd/containerd
File: internal/cri/config/streaming.go (Go)

Code before:

import (
	"crypto/tls"
	"errors"
	"fmt"
	"net"
	"os"
	"time"

	k8snet "k8s.io/apimachinery/pkg/util/net"
	k8scert "k8s.io/client-go/util/cert"

	"k8s.io/kubelet/pkg/cri/streaming"
)

type streamListenerMode int

const (
	x509KeyPairTLS streamListenerMode = iota
	selfSignTLS
	withoutTLS
)

func getStreamListenerMode(config *ServerConfig) (streamListenerMode, error) {
	if config.EnableTLSStreaming {
		if config.TLSCertFile != "" && config.TLSKeyFile != "" {
			return x509KeyPairTLS, nil
		}
		if config.TLSCertFile != "" && config.TLSKeyFile == "" {
			return -1, errors.New("must set X509KeyPairStreaming.TLSKeyFile")
		}
		if config.TLSCertFile == "" && config.TLSKeyFile != "" {
			return -1, errors.New("must set X509KeyPairStreaming.TLSCertFile")
		}
		return selfSignTLS, nil
	}
	if config.TLSCertFile != "" {
		return -1, errors.New("X509KeyPairStreaming.TLSCertFile is set but EnableTLSStreaming is not set")
	}
	if config.TLSKeyFile != "" {
		return -1, errors.New("X509KeyPairStreaming.TLSKeyFile is set but EnableTLSStreaming is not set")
	}
	return withoutTLS, nil
}

func (c *ServerConfig) StreamingConfig() (streaming.Config, error) {
	var (
		addr              = c.StreamServerAddress
		port              = c.StreamServerPort
		streamIdleTimeout = c.StreamIdleTimeout
	)

Review comment (suggestion):
See my other comment on the `QF1008` "quick fix"; I think we should consider disabling it, as sometimes explicit > implicit; also to reduce unneeded code-churn.

Code after:

import (
	"crypto/tls"
	"errors"
	"fmt"
	"net"
	"os"
	"time"

	k8snet "k8s.io/apimachinery/pkg/util/net"
	k8scert "k8s.io/client-go/util/cert"

	streaming "github.com/containerd/containerd/v2/internal/cri/streamingserver"
)

type streamListenerMode int

const (
	x509KeyPairTLS streamListenerMode = iota
	selfSignTLS
	withoutTLS
)

func getStreamListenerMode(config *ServerConfig) (streamListenerMode, error) {
	if config.EnableTLSStreaming {
		if config.X509KeyPairStreaming.TLSCertFile != "" && config.X509KeyPairStreaming.TLSKeyFile != "" {
			return x509KeyPairTLS, nil
		}
		if config.X509KeyPairStreaming.TLSCertFile != "" && config.X509KeyPairStreaming.TLSKeyFile == "" {
			return -1, errors.New("must set X509KeyPairStreaming.TLSKeyFile")
		}
		if config.X509KeyPairStreaming.TLSCertFile == "" && config.X509KeyPairStreaming.TLSKeyFile != "" {
			return -1, errors.New("must set X509KeyPairStreaming.TLSCertFile")
		}
		return selfSignTLS, nil
	}
	if config.X509KeyPairStreaming.TLSCertFile != "" {
		return -1, errors.New("X509KeyPairStreaming.TLSCertFile is set but EnableTLSStreaming is not set")
	}
	if config.X509KeyPairStreaming.TLSKeyFile != "" {
		return -1, errors.New("X509KeyPairStreaming.TLSKeyFile is set but EnableTLSStreaming is not set")
	}
	return withoutTLS, nil
}

func (c *ServerConfig) StreamingConfig() (streaming.Config, error) {
	var (
		addr              = c.StreamServerAddress
		port              = c.StreamServerPort
		streamIdleTimeout = c.StreamIdleTimeout
	)
Repository: containerd/containerd
File: pkg/shim/util_unix.go (Go)

Code before:
		return errors.Is(err, syscall.EADDRINUSE)
	}
	return false
}

// CanConnect returns true if the socket provided at the address
// is accepting new connections
func CanConnect(address string) bool {
	conn, err := AnonDialer(address, 100*time.Millisecond)
	if err != nil {
		return false
	}
	conn.Close()
	return true
}

func hybridVsockDialer(addr string, port uint64, timeout time.Duration) (net.Conn, error) {
	timeoutCh := time.After(timeout)
	// Do 10 retries before timeout
	retryInterval := timeout / 10
	for {
		conn, err := net.DialTimeout("unix", addr, timeout)
		if err != nil {
			return nil, err
		}
		if _, err = fmt.Fprintf(conn, "CONNECT %d\n", port); err != nil {
			conn.Close()
			return nil, err
		}
		errChan := make(chan error, 1)
		go func() {
			reader := bufio.NewReader(conn)
			response, err := reader.ReadString('\n')
			if err != nil {
				errChan <- err
				return
			}
			if strings.Contains(response, "OK") {
				errChan <- nil
			} else {
				errChan <- fmt.Errorf("hybrid vsock handshake response error: %s", response)
			}
		}()
		select {
		case err = <-errChan:
			if err != nil {
				conn.Close()
				// When it is EOF, maybe the server side is not ready.
				if err == io.EOF {
					log.G(context.Background()).Warnf("Read hybrid vsock got EOF, server may not ready")
					time.Sleep(retryInterval)

Review comment (performance):
Alternatively, if performance is important; `Fprintln` / `Println` do handle integers, and print passed values space-separated, so this could be;

```go
if _, err = fmt.Fprintln(conn, "CONNECT", port); err != nil {
```


Code after:
		return errors.Is(err, syscall.EADDRINUSE)
	}
	return false
}

// CanConnect returns true if the socket provided at the address
// is accepting new connections
func CanConnect(address string) bool {
	conn, err := AnonDialer(address, 100*time.Millisecond)
	if err != nil {
		return false
	}
	conn.Close()
	return true
}

func hybridVsockDialer(addr string, port uint64, timeout time.Duration) (net.Conn, error) {
	timeoutCh := time.After(timeout)
	// Do 10 retries before timeout
	retryInterval := timeout / 10
	for {
		conn, err := net.DialTimeout("unix", addr, timeout)
		if err != nil {
			return nil, err
		}
		if _, err = fmt.Fprintln(conn, "CONNECT", port); err != nil {
			conn.Close()
			return nil, err
		}
		errChan := make(chan error, 1)
		go func() {
			reader := bufio.NewReader(conn)
			response, err := reader.ReadString('\n')
			if err != nil {
				errChan <- err
				return
			}
			if strings.Contains(response, "OK") {
				errChan <- nil
			} else {
				errChan <- fmt.Errorf("hybrid vsock handshake response error: %s", response)
			}
		}()
		select {
		case err = <-errChan:
			if err != nil {
				conn.Close()
				// When it is EOF, maybe the server side is not ready.
				if err == io.EOF {
					log.G(context.Background()).Warnf("Read hybrid vsock got EOF, server may not ready")
					time.Sleep(retryInterval)
Repository: containerd/containerd
File: core/metadata/migrations.go (Go)

Code before:
				return cerr
			}

			return sbbkt.ForEachBucket(func(subbkt []byte) error {
				tsubbkt, err := tsbbkt.CreateBucketIfNotExists(subbkt)
				if err != nil {
					return fmt.Errorf("failed to create subbucket %s in sandbox %s (namespace %s): %w",
						subbkt, sb, ns, err)
				}

				return sbbkt.Bucket(subbkt).ForEach(func(key, value []byte) error {
					if value == nil {
						return fmt.Errorf("unexpected bucket %s", key)
					}
					return tsubbkt.Put(key, value)
				})
			})
		})
	}); merr != nil {
		return fmt.Errorf("failed to copy sandboxes into v1 bucket: %w", merr)
	}

	for _, ns := range deletingBuckets {
		derr := tx.DeleteBucket(ns)
		if derr != nil {
			return fmt.Errorf("failed to cleanup bucket %s in root: %w", ns, derr)
		}
	}
	return nil
}

// noOpMigration was for a database change from boltdb/bolt which is no
// longer being supported, to go.etcd.io/bbolt which is the currently
// maintained repo for boltdb.
func noOpMigration(tx *bolt.Tx) error {
	return nil
}


Review comment (suggestion):
Perhaps instead we should rename all these errors to just `err`.

Looking at this function, I see a `cerr`, a `derr` and an `merr`, and none of them seem to be special.

Using a special name not only makes it more err-prone (as evident from this PR), but also adds cognitive load.

Code after:
				return err
			}

			return sbbkt.ForEachBucket(func(subbkt []byte) error {
				tsubbkt, err := tsbbkt.CreateBucketIfNotExists(subbkt)
				if err != nil {
					return fmt.Errorf("failed to create subbucket %s in sandbox %s (namespace %s): %w",
						subbkt, sb, ns, err)
				}

				return sbbkt.Bucket(subbkt).ForEach(func(key, value []byte) error {
					if value == nil {
						return fmt.Errorf("unexpected bucket %s", key)
					}
					return tsubbkt.Put(key, value)
				})
			})
		})
	}); err != nil {
		return fmt.Errorf("failed to copy sandboxes into v1 bucket: %w", err)
	}

	for _, ns := range deletingBuckets {
		err := tx.DeleteBucket(ns)
		if err != nil {
			return fmt.Errorf("failed to cleanup bucket %s in root: %w", ns, err)
		}
	}
	return nil
}

// noOpMigration was for a database change from boltdb/bolt which is no
// longer being supported, to go.etcd.io/bbolt which is the currently
// maintained repo for boltdb.
func noOpMigration(tx *bolt.Tx) error {
	return nil
}
Repository: containerd/containerd
File: core/metadata/migrations.go (Go)

Code before:
				return tsbbkt.Put(key, value)
			}); cerr != nil {
				return cerr
			}

			return sbbkt.ForEachBucket(func(subbkt []byte) error {
				tsubbkt, err := tsbbkt.CreateBucketIfNotExists(subbkt)
				if err != nil {
					return fmt.Errorf("failed to create subbucket %s in sandbox %s (namespace %s): %w",
						subbkt, sb, ns, err)
				}

				return sbbkt.Bucket(subbkt).ForEach(func(key, value []byte) error {
					if value == nil {
						return fmt.Errorf("unexpected bucket %s", key)
					}
					return tsubbkt.Put(key, value)
				})
			})
		})
	}); merr != nil {
		return fmt.Errorf("failed to copy sandboxes into v1 bucket: %w", merr)
	}

	for _, ns := range deletingBuckets {
		derr := tx.DeleteBucket(ns)
		if derr != nil {
			return fmt.Errorf("failed to cleanup bucket %s in root: %w", ns, derr)
		}
	}
	return nil
}

// noOpMigration was for a database change from boltdb/bolt which is no
// longer being supported, to go.etcd.io/bbolt which is the currently
// maintained repo for boltdb.
func noOpMigration(tx *bolt.Tx) error {
	return nil
}


Review comment (suggestion):
if we change; probably would be good to also inline this err in the if-statement (`if err := tx.DeleteBucket(ns); err != nil {`). lts slightly more idiomatic, and clearly indicates that it's a local var.

Code after:
				return tsbbkt.Put(key, value)
			}); err != nil {
				return err
			}

			return sbbkt.ForEachBucket(func(subbkt []byte) error {
				tsubbkt, err := tsbbkt.CreateBucketIfNotExists(subbkt)
				if err != nil {
					return fmt.Errorf("failed to create subbucket %s in sandbox %s (namespace %s): %w",
						subbkt, sb, ns, err)
				}

				return sbbkt.Bucket(subbkt).ForEach(func(key, value []byte) error {
					if value == nil {
						return fmt.Errorf("unexpected bucket %s", key)
					}
					return tsubbkt.Put(key, value)
				})
			})
		})
	}); err != nil {
		return fmt.Errorf("failed to copy sandboxes into v1 bucket: %w", err)
	}

	for _, ns := range deletingBuckets {
		err := tx.DeleteBucket(ns)
		if err != nil {
			return fmt.Errorf("failed to cleanup bucket %s in root: %w", ns, err)
		}
	}
	return nil
}

// noOpMigration was for a database change from boltdb/bolt which is no
// longer being supported, to go.etcd.io/bbolt which is the currently
// maintained repo for boltdb.
func noOpMigration(tx *bolt.Tx) error {
	return nil
}
Repository: containerd/containerd
File: pkg/sys/unshare_linux.go (Go)

Code before:
			GidMappingsEnableSetgroups: true,
			// NOTE: It's reexec but it's not heavy because subprocess
			// be in PTRACE_TRACEME mode before performing execve.
			Ptrace:    true,
			Pdeathsig: syscall.SIGKILL,
			PidFD:     &pidfd,
		},
	})
	if err != nil {
		return fmt.Errorf("failed to start noop process for unshare: %w", err)
	}

	if pidfd == -1 || !SupportsPidFD() {
		proc.Kill()
		proc.Wait()
		return fmt.Errorf("kernel doesn't support CLONE_PIDFD")
	}

	// Since go1.23.{0,1} has double close issue, we should dup it before using it.
	//
	// References:
	// - https://github.com/golang/go/issues/68984
	// - https://github.com/golang/go/milestone/371
	if goVer := runtime.Version(); goVer == "go1.23.0" || goVer == "go1.23.1" {
		dupPidfd, dupErr := unix.FcntlInt(uintptr(pidfd), syscall.F_DUPFD_CLOEXEC, 0)
		if dupErr != nil {
			proc.Kill()
			proc.Wait()
			return fmt.Errorf("failed to dupfd: %w", dupErr)
		}
		pidfd = dupPidfd
	}

	defer func() {
		derr := unix.PidfdSendSignal(pidfd, unix.SIGKILL, nil, 0)
		if derr != nil {
			if !errors.Is(derr, unix.ESRCH) {
				retErr = derr
			}
			return
		}
		pidfdWaitid(pidfd)
	}()

	if f != nil {
		if err := f(proc.Pid); err != nil {
			return err
		}
	}

	// Ensure the child process is still alive. If the err is ESRCH, we

Review comment (suggestion):
If you still need to push again (for the sign-off); could you update this one as well? (`dupPidfd, err := `). Sorry I thought I commented on this one, but looks like I forgot to submit before I had to jump into a meeting 🙈 


(changes LGTM otherwise ❤️)

Code after:
			GidMappingsEnableSetgroups: true,
			// NOTE: It's reexec but it's not heavy because subprocess
			// be in PTRACE_TRACEME mode before performing execve.
			Ptrace:    true,
			Pdeathsig: syscall.SIGKILL,
			PidFD:     &pidfd,
		},
	})
	if err != nil {
		return fmt.Errorf("failed to start noop process for unshare: %w", err)
	}

	if pidfd == -1 || !SupportsPidFD() {
		proc.Kill()
		proc.Wait()
		return fmt.Errorf("kernel doesn't support CLONE_PIDFD")
	}

	// Since go1.23.{0,1} has double close issue, we should dup it before using it.
	//
	// References:
	// - https://github.com/golang/go/issues/68984
	// - https://github.com/golang/go/milestone/371
	if goVer := runtime.Version(); goVer == "go1.23.0" || goVer == "go1.23.1" {
		dupPidfd, err := unix.FcntlInt(uintptr(pidfd), syscall.F_DUPFD_CLOEXEC, 0)
		if err != nil {
			proc.Kill()
			proc.Wait()
			return fmt.Errorf("failed to dupfd: %w", err)
		}
		pidfd = dupPidfd
	}

	defer func() {
		derr := unix.PidfdSendSignal(pidfd, unix.SIGKILL, nil, 0)
		if derr != nil {
			if !errors.Is(derr, unix.ESRCH) {
				retErr = derr
			}
			return
		}
		pidfdWaitid(pidfd)
	}()

	if f != nil {
		if err := f(proc.Pid); err != nil {
			return err
		}
	}

	// Ensure the child process is still alive. If the err is ESRCH, we
Repository: containerd/containerd
File: core/mount/losetup_linux_test.go (Go)

Code before:
       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.
*/

package mount

import (
	"os"
	"testing"
	"time"

	"github.com/containerd/continuity/testutil"
)

var randomData = []byte("randomdata")

func createTempFile(t *testing.T) string {
	t.Helper()

	f, err := os.CreateTemp(t.TempDir(), "losetup")
	if err != nil {
		t.Fatal(err)
	}
	defer f.Close()

	if err = f.Truncate(512); err != nil {
		t.Fatal(err)
	}

	return f.Name()
}

func TestNonExistingLoop(t *testing.T) {
	testutil.RequiresRoot(t)

	backingFile := "setup-loop-test-no-such-file"
	_, err := setupLoop(backingFile, LoopParams{})
	if err == nil {
		t.Fatalf("setupLoop with non-existing file should fail")
	}
}

func TestRoLoop(t *testing.T) {
	testutil.RequiresRoot(t)

	backingFile := createTempFile(t)

Review comment (suggestion):
```suggestion
	f, err := os.Create(t.TempDir(), "losetup")
```

If we're in a `t.TempDir` we don't need the randomized name from `os.CreateTemp`.

Please also remove `defer`s that delete these files.  `t.TempDir` implicitly adds cleanup functions to `t.Cleanup`.

Code after:
       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.
*/

package mount

import (
	"os"
	"path/filepath"
	"testing"
	"time"

	"github.com/containerd/continuity/testutil"
	"github.com/stretchr/testify/require"
)

var randomData = []byte("randomdata")

func createTempFile(t *testing.T) string {
	t.Helper()

	f, err := os.Create(filepath.Join(t.TempDir(), "losetup"))
	require.NoError(t, err)
	t.Cleanup(func() {
		f.Close()
	})

	err = f.Truncate(512)
	require.NoError(t, err)

	return f.Name()
}

func TestNonExistingLoop(t *testing.T) {
	testutil.RequiresRoot(t)

	backingFile := "setup-loop-test-no-such-file"
	_, err := setupLoop(backingFile, LoopParams{})
	if err == nil {
		t.Fatalf("setupLoop with non-existing file should fail")
	}
}

func TestRoLoop(t *testing.T) {
	testutil.RequiresRoot(t)
Repository: containerd/containerd
File: integration/client/export_test.go (Go)

Code before:

				// Check if archive contains only one manifest for the default platform
				if isImageInArchive(ctx, t, client, dstFile, defaultPlatformManifest) {
					t.Fatal("archive shouldn't contain manifest for the default platform")
				}
			},
		},
	} {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			ctx, cancel := testContext(t)
			defer cancel()

			namespace := uuid.New().String()
			client, err := newClient(t, address, WithDefaultNamespace(namespace))
			if err != nil {
				t.Fatal(err)
			}
			defer client.Close()

			ctx = namespaces.WithNamespace(ctx, namespace)

			img := tc.prepare(ctx, t, client)

			dstFile, err := os.CreateTemp(t.TempDir(), "export-test")
			if err != nil {
				t.Fatal(err)
			}
			defer func() {
				dstFile.Close()
				os.Remove(dstFile.Name())
			}()

			tc.check(ctx, t, client, dstFile, img)
		})
	}
}

func isImageInArchive(ctx context.Context, t *testing.T, client *Client, dstFile *os.File, mfst ocispec.Descriptor) bool {
	dstFile.Seek(0, 0)
	tr := tar.NewReader(dstFile)

	var blobs []string
	for {
		h, err := tr.Next()
		if err != nil {
			if err == io.EOF {
				break
			}
			t.Fatal(err)
		}

Review comment (suggestion):
```suggestion
			dstFile, err := os.Create(t.TempDir(), "export-test")
```

Code after:
					t.Fatal(err)
				}

				// Check if archive contains only one manifest for the default platform
				if isImageInArchive(ctx, t, client, dstFile, defaultPlatformManifest) {
					t.Fatal("archive shouldn't contain manifest for the default platform")
				}
			},
		},
	} {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			ctx, cancel := testContext(t)
			defer cancel()

			namespace := uuid.New().String()
			client, err := newClient(t, address, WithDefaultNamespace(namespace))
			require.NoError(t, err)
			t.Cleanup(func() {
				client.Close()
			})

			ctx = namespaces.WithNamespace(ctx, namespace)

			img := tc.prepare(ctx, t, client)

			dstFile, err := os.Create(filepath.Join(t.TempDir(), "export-test"))
			require.NoError(t, err)
			t.Cleanup(func() {
				dstFile.Close()
			})

			tc.check(ctx, t, client, dstFile, img)
		})
	}
}

func isImageInArchive(ctx context.Context, t *testing.T, client *Client, dstFile *os.File, mfst ocispec.Descriptor) bool {
	dstFile.Seek(0, 0)
	tr := tar.NewReader(dstFile)

	var blobs []string
	for {
		h, err := tr.Next()
		if err != nil {
			if err == io.EOF {
				break
			}
			t.Fatal(err)
		}