ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: client/container_exec.go (Go)

Code before:
// [stdcopy.StdCopy]: https://pkg.go.dev/github.com/moby/moby/api/pkg/stdcopy#StdCopy
func (cli *Client) ContainerExecAttach(ctx context.Context, execID string, config ExecAttachOptions) (HijackedResponse, error) {
	if versions.LessThan(cli.ClientVersion(), "1.42") {
		config.ConsoleSize = nil
	}
	req := container.ExecStartRequest{
		Detach:      config.Detach,
		Tty:         config.Tty,
		ConsoleSize: config.ConsoleSize,
	}
	return cli.postHijacked(ctx, "/exec/"+execID+"/start", nil, req, http.Header{
		"Content-Type": {"application/json"},
	})
}

// ContainerExecInspect returns information about a specific exec process on the docker host.
func (cli *Client) ContainerExecInspect(ctx context.Context, execID string) (container.ExecInspect, error) {
	resp, err := cli.get(ctx, "/exec/"+execID+"/json", nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return container.ExecInspect{}, err
	}

	var response container.ExecInspect
	err = json.NewDecoder(resp.Body).Decode(&response)
	return response, err
}


Review comment (suggestion):
I could probably consider taking the opportunity to rename `ExecID` to `ID` (matching the response), and `Pid` to `PID` (the field-name, not the `json` name if we want to keep marshalling the same when used) to have the  proper casing.

Code after:
// [stdcopy.StdCopy]: https://pkg.go.dev/github.com/moby/moby/api/pkg/stdcopy#StdCopy
func (cli *Client) ContainerExecAttach(ctx context.Context, execID string, config ExecAttachOptions) (HijackedResponse, error) {
	if versions.LessThan(cli.ClientVersion(), "1.42") {
		config.ConsoleSize = nil
	}
	req := container.ExecStartRequest{
		Detach:      config.Detach,
		Tty:         config.Tty,
		ConsoleSize: config.ConsoleSize,
	}
	return cli.postHijacked(ctx, "/exec/"+execID+"/start", nil, req, http.Header{
		"Content-Type": {"application/json"},
	})
}

// ExecInspect holds information returned by exec inspect.
//
// It provides a subset of the information included in [container.ExecInspectResponse].
//
// TODO(thaJeztah): include all fields of [container.ExecInspectResponse] ?
type ExecInspect struct {
	ExecID      string `json:"ID"`
	ContainerID string `json:"ContainerID"`
	Running     bool   `json:"Running"`
	ExitCode    int    `json:"ExitCode"`
	Pid         int    `json:"Pid"`
}

// ContainerExecInspect returns information about a specific exec process on the docker host.
func (cli *Client) ContainerExecInspect(ctx context.Context, execID string) (ExecInspect, error) {
	resp, err := cli.get(ctx, "/exec/"+execID+"/json", nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ExecInspect{}, err
	}

	var response container.ExecInspectResponse
	err = json.NewDecoder(resp.Body).Decode(&response)
	if err != nil {
		return ExecInspect{}, err
	}

	var ec int
	if response.ExitCode != nil {
		ec = *response.ExitCode
	}

	return ExecInspect{
		ExecID:      response.ID,
		ContainerID: response.ContainerID,
		Running:     response.Running,
Repository: moby/moby
File: daemon/libnetwork/ns/init_linux.go (Go)

Code before:
	// NetlinkSocketsTimeout represents the default timeout duration for the sockets
	NetlinkSocketsTimeout = 3 * time.Second
)

// initHandles initializes a new network namespace
func initHandles() {
	var err error
	initNs, err = netns.Get()
	if err != nil {
		log.G(context.TODO()).Errorf("could not get initial namespace: %v", err)
	}
	initNl, err = nlwrap.NewHandle(getSupportedNlFamilies()...)
	if err != nil {
		log.G(context.TODO()).Errorf("could not create netlink handle on initial namespace: %v", err)
	}
	err = initNl.SetSocketTimeout(NetlinkSocketsTimeout)
	if err != nil {
		log.G(context.TODO()).Warnf("Failed to set the timeout on the default netlink handle sockets: %v", err)
	}
}

// ResetHandles resets the initial namespace and netlink handles.
// This is useful for testing to ensure a clean state, and should not be
// used in production code.
func ResetHandles() {
	if initNs.IsOpen() {
		initNs.Close()
		initNs = netns.None()
	}
	if initNl.Handle != nil {
		initNl.Close()
		initNl = nlwrap.Handle{}
	}
	initOnce = sync.Once{}
}

// ParseHandlerInt transforms the namespace handler into an integer
func ParseHandlerInt() int {
	return int(getHandler())
}

// GetHandler returns the namespace handler
func getHandler() netns.NsHandle {
	initOnce.Do(initHandles)
	return initNs
}

// NlHandle returns the netlink handler
func NlHandle() nlwrap.Handle {
	initOnce.Do(initHandles)
	return initNl

Review comment (suggestion):
We could even protect this by panicking (e.g.) when called if `testing.Testing()` is `false` (i.e. when used in a non-test binary)

Code after:
	initOnce sync.Once
	// NetlinkSocketsTimeout represents the default timeout duration for the sockets
	NetlinkSocketsTimeout = 3 * time.Second
)

// initHandles initializes a new network namespace
func initHandles() {
	var err error
	initNs, err = netns.Get()
	if err != nil {
		log.G(context.TODO()).Errorf("could not get initial namespace: %v", err)
	}
	initNl, err = nlwrap.NewHandle(getSupportedNlFamilies()...)
	if err != nil {
		log.G(context.TODO()).Errorf("could not create netlink handle on initial namespace: %v", err)
	}
	err = initNl.SetSocketTimeout(NetlinkSocketsTimeout)
	if err != nil {
		log.G(context.TODO()).Warnf("Failed to set the timeout on the default netlink handle sockets: %v", err)
	}
}

// ResetHandles resets the initial namespace and netlink handles.
// This is useful for testing to ensure a clean state. It will
// panic if called outside a test.
func ResetHandles() {
	if !testing.Testing() {
		panic("ResetHandles should only be called from tests")
	}
	if initNs.IsOpen() {
		initNs.Close()
		initNs = netns.None()
	}
	if initNl.Handle != nil {
		initNl.Close()
		initNl = nlwrap.Handle{}
	}
	initOnce = sync.Once{}
}

// ParseHandlerInt transforms the namespace handler into an integer
func ParseHandlerInt() int {
	return int(getHandler())
}

// GetHandler returns the namespace handler
func getHandler() netns.NsHandle {
	initOnce.Do(initHandles)
	return initNs
}
Repository: moby/moby
File: daemon/command/daemon_linux.go (Go)

Code before:
}

// notifyStopping sends a message to the host when the server is shutting down
func notifyStopping() {
	go systemdDaemon.SdNotify(false, systemdDaemon.SdNotifyStopping)
}

func validateCPURealtimeOptions(cfg *config.Config) error {
	if cfg.CPURealtimePeriod == 0 && cfg.CPURealtimeRuntime == 0 {
		return nil
	}
	if cdcgroups.Mode() == cdcgroups.Unified {
		return errors.New("daemon-scoped cpu-rt-period and cpu-rt-runtime are not implemented for cgroup v2")
	}
	if !sysinfo.New().CPURealtime {
		return errors.New("daemon-scoped cpu-rt-period and cpu-rt-runtime are not supported by the kernel")
	}
	return nil
}

// Buildkit breaks when userns remapping is enabled, and containerd snapshotter is used. As a workaround, disable
// containerd snapshotter if userns remapping is enabled. See https://github.com/moby/moby/issues/47377.
func disableC8dSnapshotterOnUsernsRemap(cfg *config.Config) {
	if cfg.RemappedRoot != "" {
		if enabled, ok := cfg.Features["containerd-snapshotter"]; !ok || enabled {
			log.G(context.TODO()).Warn("userns remapping enabled, disabling containerd snapshotter")
		}
		if cfg.Features == nil {
			cfg.Features = make(map[string]bool)
		}
		cfg.Features["containerd-snapshotter"] = false
	}
}


Review comment (bug):
Can we instead make this a hard failure and return an error (validate the combination of userns-remapping and snapshotter, then invalidate the config and error)?
https://github.com/moby/moby/blob/dcf5db2464b20961acecfcee71dc2f808e4f70ee/daemon/command/docker.go#L35-L42

Code after:
	conf.Features["containerd-snapshotter"] = false

	return nil
}

// preNotifyReady sends a message to the host when the API is active, but before the daemon is
func preNotifyReady() error {
	return nil
}

// notifyReady sends a message to the host when the server is ready to be used
func notifyReady() {
	// Tell the init daemon we are accepting requests
	go systemdDaemon.SdNotify(false, systemdDaemon.SdNotifyReady)
}

// notifyStopping sends a message to the host when the server is shutting down
func notifyStopping() {
	go systemdDaemon.SdNotify(false, systemdDaemon.SdNotifyStopping)
}

func validateCPURealtimeOptions(cfg *config.Config) error {
	if cfg.CPURealtimePeriod == 0 && cfg.CPURealtimeRuntime == 0 {
		return nil
	}
	if cdcgroups.Mode() == cdcgroups.Unified {
		return errors.New("daemon-scoped cpu-rt-period and cpu-rt-runtime are not implemented for cgroup v2")
	}
	if !sysinfo.New().CPURealtime {
		return errors.New("daemon-scoped cpu-rt-period and cpu-rt-runtime are not supported by the kernel")
	}
	return nil
}
Repository: moby/moby
File: api/types/system/info.go (Go)

Code before:
package system

import (
	"github.com/moby/moby/api/types/container"
	"github.com/moby/moby/api/types/registry"
	"github.com/moby/moby/api/types/swarm"
)

// Info contains response of Engine API:
// GET "/info"
type Info struct {
	ID                string
	Containers        int
	ContainersRunning int
	ContainersPaused  int
	ContainersStopped int
	Images            int
	Driver            string
	DriverStatus      [][2]string
	SystemStatus      [][2]string `json:",omitempty"` // SystemStatus is only propagated by the Swarm standalone API
	Plugins           PluginsInfo
	MemoryLimit       bool
	SwapLimit         bool

	// KernelMemoryLimit is not supported on cgroups v2.
	//
	// Deprecated: This field is deprecated and will be removed in the next release.
	// Starting with kernel 6.12, the kernel has deprecated kernel memory tcp accounting
	KernelMemoryTCP bool `json:",omitempty"`

	CPUCfsPeriod       bool `json:"CpuCfsPeriod"`
	CPUCfsQuota        bool `json:"CpuCfsQuota"`
	CPUShares          bool
	CPUSet             bool
	PidsLimit          bool
	IPv4Forwarding     bool
	Debug              bool
	NFd                int
	OomKillDisable     bool
	NGoroutines        int
	SystemTime         string
	LoggingDriver      string
	CgroupDriver       string
	CgroupVersion      string `json:",omitempty"`
	NEventsListener    int

Review comment (suggestion):
Forgot we still had this one;  this field was never set by the docker daemon, but was something that was augmented by the swarm v1 API, which. It's possible that Mirantis is still using this, but not sure if they need this to be in the API types from this repository, or if they use their own types to (un)marshal the response.

cc @corhere 

Code after:
package system

import (
	"github.com/moby/moby/api/types/container"
	"github.com/moby/moby/api/types/registry"
	"github.com/moby/moby/api/types/swarm"
)

// Info contains response of Engine API:
// GET "/info"
type Info struct {
	ID                 string
	Containers         int
	ContainersRunning  int
	ContainersPaused   int
	ContainersStopped  int
	Images             int
	Driver             string
	DriverStatus       [][2]string
	SystemStatus       [][2]string `json:",omitempty"` // SystemStatus is only propagated by the Swarm standalone API
	Plugins            PluginsInfo
	MemoryLimit        bool
	SwapLimit          bool
	CPUCfsPeriod       bool `json:"CpuCfsPeriod"`
	CPUCfsQuota        bool `json:"CpuCfsQuota"`
	CPUShares          bool
	CPUSet             bool
	PidsLimit          bool
	IPv4Forwarding     bool
	Debug              bool
	NFd                int
	OomKillDisable     bool
	NGoroutines        int
	SystemTime         string
	LoggingDriver      string
	CgroupDriver       string
	CgroupVersion      string `json:",omitempty"`
	NEventsListener    int
	KernelVersion      string
	OperatingSystem    string
	OSVersion          string
	OSType             string
	Architecture       string
	IndexServerAddress string
	RegistryConfig     *registry.ServiceConfig
Repository: moby/moby
File: client/config_create.go (Go)

Code before:
package client

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Config swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	Response swarm.ConfigCreateResponse
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	var out ConfigCreateResult

	resp, err := cli.post(ctx, "/configs/create", nil, options.Config, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return out, err
	}

	err = json.NewDecoder(resp.Body).Decode(&out.Response)
	return out, err
}


Review comment (suggestion):
Could use your thoughts on this; given that the API can be updated independent from the client module, we could have a situation where the API gained a new field, but the client may not handle it; in this case, likely not a huge issue as it directly unmarshals whatever it receives, but in other cases it _could_ mean that the response is not handled, but the client returning a struct with fields not set and/or not handled correctly.

One option we have for that is to make the client define the counterpart to the response as well for the API version it was created for, e.g. similar to https://github.com/moby/moby/pull/50991/files 

Code after:
package client

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Spec swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	ID string
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	resp, err := cli.post(ctx, "/configs/create", nil, options.Spec, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ConfigCreateResult{}, err
	}

	var out swarm.ConfigCreateResponse
	err = json.NewDecoder(resp.Body).Decode(&out)
	if err != nil {
		return ConfigCreateResult{}, err
	}
	return ConfigCreateResult{ID: out.ID}, nil
}
Repository: moby/moby
File: client/config_create.go (Go)

Code before:

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Config swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	Response swarm.ConfigCreateResponse
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	var out ConfigCreateResult

	resp, err := cli.post(ctx, "/configs/create", nil, options.Config, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return out, err
	}

	err = json.NewDecoder(resp.Body).Decode(&out.Response)
	return out, err
}


Review comment (suggestion):
I think I commented something similar on another PR; I generally prefer the explicit `return SomeStruct{}, err` - it makes it crystal clear "nothing is returned", and it prevents some cases where we returned a _partially_ propagated structure.






Code after:

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Spec swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	ID string
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	resp, err := cli.post(ctx, "/configs/create", nil, options.Spec, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ConfigCreateResult{}, err
	}

	var out swarm.ConfigCreateResponse
	err = json.NewDecoder(resp.Body).Decode(&out)
	if err != nil {
		return ConfigCreateResult{}, err
	}
	return ConfigCreateResult{ID: out.ID}, nil
}
Repository: moby/moby
File: client/config_create.go (Go)

Code before:

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Config swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	Response swarm.ConfigCreateResponse
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	var out ConfigCreateResult

	resp, err := cli.post(ctx, "/configs/create", nil, options.Config, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return out, err
	}

	err = json.NewDecoder(resp.Body).Decode(&out.Response)
	return out, err
}


Review comment (suggestion):
If you make those changes, also move the `var out ConfigCreateResult` back to before the `json.NewDecoder` to keep it close to where used.

Code after:

	"github.com/moby/moby/api/types/swarm"
)

// ConfigCreateOptions holds options for creating a config.
type ConfigCreateOptions struct {
	Spec swarm.ConfigSpec
}

// ConfigCreateResult holds the result from the ConfigCreate method.
type ConfigCreateResult struct {
	ID string
}

// ConfigCreate creates a new config.
func (cli *Client) ConfigCreate(ctx context.Context, options ConfigCreateOptions) (ConfigCreateResult, error) {
	resp, err := cli.post(ctx, "/configs/create", nil, options.Spec, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ConfigCreateResult{}, err
	}

	var out swarm.ConfigCreateResponse
	err = json.NewDecoder(resp.Body).Decode(&out)
	if err != nil {
		return ConfigCreateResult{}, err
	}
	return ConfigCreateResult{ID: out.ID}, nil
}
Repository: moby/moby
File: client/config_update.go (Go)

Code before:
package client

import (
	"context"
	"net/url"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigUpdateOptions holds options for updating a config.
type ConfigUpdateOptions struct {
	Config swarm.ConfigSpec
}

type ConfigUpdateResult struct{}

// ConfigUpdate attempts to update a config
func (cli *Client) ConfigUpdate(ctx context.Context, vID SwarmVersionedID, options ConfigUpdateOptions) (ConfigUpdateResult, error) {
	id, err := trimID("config", vID.ID)
	if err != nil {
		return ConfigUpdateResult{}, err
	}
	query := url.Values{}
	query.Set("version", vID.Version.String())
	resp, err := cli.post(ctx, "/configs/"+id+"/update", query, options.Config, nil)
	defer ensureReaderClosed(resp)
	return ConfigUpdateResult{}, err
}


Review comment (suggestion):
🚲 🏡  What if we named these fields `Spec` for all the swarm-related ones? Basically all swarm ones come down to;

- Create (Spec)
- To update a spec;
   - Fetch current spec (get revision and properties)
   - Patch current spec
   - Patch (= Update) spec (passing revision to update from and updated spec)


Code after:
package client

import (
	"context"
	"net/url"

	"github.com/moby/moby/api/types/swarm"
)

// ConfigUpdateOptions holds options for updating a config.
type ConfigUpdateOptions struct {
	Version swarm.Version
	Spec    swarm.ConfigSpec
}

type ConfigUpdateResult struct{}

// ConfigUpdate attempts to update a config
func (cli *Client) ConfigUpdate(ctx context.Context, id string, options ConfigUpdateOptions) (ConfigUpdateResult, error) {
	id, err := trimID("config", id)
	if err != nil {
		return ConfigUpdateResult{}, err
	}
	query := url.Values{}
	query.Set("version", options.Version.String())
	resp, err := cli.post(ctx, "/configs/"+id+"/update", query, options.Spec, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ConfigUpdateResult{}, err
	}
	return ConfigUpdateResult{}, nil
}
Repository: moby/moby
File: client/config_inspect.go (Go)

Code before:
	"github.com/moby/moby/api/types/swarm"
)

// ConfigInspectOptions holds options for inspecting a config.
type ConfigInspectOptions struct {
	// Add future optional parameters here
}

// ConfigInspectResult holds the result from the ConfigInspect method.
type ConfigInspectResult struct {
	Config swarm.Config
	Raw    []byte
}

// ConfigInspect returns the config information with raw data
func (cli *Client) ConfigInspect(ctx context.Context, id string, options ConfigInspectOptions) (ConfigInspectResult, error) {
	var out ConfigInspectResult

	id, err := trimID("config", id)
	if err != nil {
		return out, err
	}
	resp, err := cli.get(ctx, "/configs/"+id, nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return out, err
	}

	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return out, err
	}

	out.Raw = body
	rdr := bytes.NewReader(body)
	err = json.NewDecoder(rdr).Decode(&out.Config)

	return out, err
}


Review comment (refactor):
How about something like this?

```go
	var rawResponse bytes.Buffer
	rdr := io.TeeReader(resp.Body, &rawResponse)

	var out ConfigInspectResult
	err = json.NewDecoder(rdr).Decode(&out.Config)
	if err != nil {
		return ConfigInspectResult{}, err
	}
	out.Raw = rawResponse.Bytes()
	return out, nil
```

Potentially that could even allow to be a small utility;

```go
func decodeWithRaw[T any](rdr io.Reader, out *T) (raw []byte, _ error) {
	var buf bytes.Buffer
	tr := io.TeeReader(rdr, &buf)
	err := json.NewDecoder(tr).Decode(out)
	return buf.Bytes(), err
}
```

Which would keep things really minimal;

```go
var out ConfigInspectResult
out.Raw, err = decodeWithRaw(resp.Body, &out.Config)
if err != nil {
    return ConfigInspectResult{}, err
}
```


Code after:
// ConfigInspectOptions holds options for inspecting a config.
type ConfigInspectOptions struct {
	// Add future optional parameters here
}

// ConfigInspectResult holds the result from the ConfigInspect method.
type ConfigInspectResult struct {
	Config swarm.Config
	Raw    []byte
}

// ConfigInspect returns the config information with raw data
func (cli *Client) ConfigInspect(ctx context.Context, id string, options ConfigInspectOptions) (ConfigInspectResult, error) {
	id, err := trimID("config", id)
	if err != nil {
		return ConfigInspectResult{}, err
	}
	resp, err := cli.get(ctx, "/configs/"+id, nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ConfigInspectResult{}, err
	}

	var out ConfigInspectResult
	out.Raw, err = decodeWithRaw(resp, &out.Config)
	return out, err
}