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

Code before:
package libnetwork

import (
	"context"
	"fmt"
	"sync"

	"github.com/containerd/log"
	"github.com/docker/docker/libnetwork/iptables"
	"github.com/docker/docker/libnetwork/netlabel"
	"github.com/docker/docker/libnetwork/options"
	"github.com/docker/docker/libnetwork/osl"
)

// FirewallBackend returns the name of the firewall backend for "docker info".
func (c *Controller) FirewallBackend() [][2]string {
	if iptables.FirewalldRunning() {
		return [][2]string{{"driver", "iptables+firewalld"}}
	}
	return [][2]string{{"driver", "iptables"}}
}

// enabledIptablesVersions returns the iptables versions that are enabled
// for the controller.
func (c *Controller) enabledIptablesVersions() []iptables.IPVersion {
	c.mu.Lock()
	defer c.mu.Unlock()
	if c.cfg == nil {
		return nil
	}
	// parse map cfg["bridge"]["generic"]["EnableIPTable"]
	cfgBridge := c.cfg.DriverConfig("bridge")
	cfgGeneric, ok := cfgBridge[netlabel.GenericData].(options.Generic)
	if !ok {
		return nil
	}

	var versions []iptables.IPVersion
	if enabled, ok := cfgGeneric["EnableIPTables"].(bool); enabled || !ok {
		// iptables is enabled unless user explicitly disabled it
		versions = append(versions, iptables.IPv4)
	}

Review comment (suggestion):
And pass the context here;
```suggestion
// FirewallBackend returns the name of the firewall backend for "docker info".
func (c *Controller) FirewallBackend(ctx context.Context) [][2]string {
	if iptables.FirewalldRunning(ctx) {
```

Code after:
package libnetwork

import (
	"context"
	"fmt"
	"sync"

	"github.com/containerd/log"
	"github.com/docker/docker/libnetwork/iptables"
	"github.com/docker/docker/libnetwork/netlabel"
	"github.com/docker/docker/libnetwork/options"
	"github.com/docker/docker/libnetwork/osl"
)

// enabledIptablesVersions returns the iptables versions that are enabled
// for the controller.
func (c *Controller) enabledIptablesVersions() []iptables.IPVersion {
	c.mu.Lock()
	defer c.mu.Unlock()
	if c.cfg == nil {
		return nil
	}
	// parse map cfg["bridge"]["generic"]["EnableIPTable"]
	cfgBridge := c.cfg.DriverConfig("bridge")
	cfgGeneric, ok := cfgBridge[netlabel.GenericData].(options.Generic)
	if !ok {
		return nil
	}

	var versions []iptables.IPVersion
	if enabled, ok := cfgGeneric["EnableIPTables"].(bool); enabled || !ok {
		// iptables is enabled unless user explicitly disabled it
		versions = append(versions, iptables.IPv4)
	}
	if enabled, _ := cfgGeneric["EnableIP6Tables"].(bool); enabled {
		versions = append(versions, iptables.IPv6)
	}
	return versions
}

// getDefaultOSLSandbox returns the controller's default [osl.Sandbox]. It
// creates the sandbox if it does not yet exist.
Repository: moby/moby
File: libnetwork/drivers/bridge/setup_ip_tables_linux.go (Go)

Code before:

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.cleanup()
		}
	}()

	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return nil, err
		}
	}
	if n.Enable6 {
		if err := n.configure(iptables.IPv6, n.Config6); err != nil {
			return nil, err
		}
	}
	return n, nil
}

func (n *iptablesNetwork) delNetworkLevelRules() error {
	return n.cleanup()
}

func (n *iptablesNetwork) configure(ipv iptables.IPVersion, conf networkConfigFam) error {
	if !conf.Prefix.IsValid() {
		// Delete INC rules, in case they were created by a 28.0.0 daemon that didn't check
		// whether the network had iptables/ip6tables enabled.
		// This preserves https://github.com/moby/moby/commit/8cc4d1d4a2b6408232041f9ba4dff966eba80cc0
		return setINC(ipv, n.IfName, conf.Routed, false)
	}
	if err := n.setupIPTables(ipv, conf); err != nil {
		return err
	}
	if !n.Internal {
		// Register the cleanup function first. Then, if setINC fails after creating
		// some rules, they will be deleted.
		n.registerCleanFunc(func() error {
			return setINC(ipv, n.IfName, conf.Routed, false)
		})
		if err := setINC(ipv, n.IfName, conf.Routed, true); err != nil {
			return err
		}
	}
	return nil
}


Review comment (suggestion):
I think you can merge `cleanup` with this function and call it from all the defers that are currently calling `cleanup`.

Code after:

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.delNetworkLevelRules()
		}
	}()

	if err := n.reapplyNetworkLevelRules(); err != nil {
		return nil, err
	}
	return n, nil
}

func (n *iptablesNetwork) reapplyNetworkLevelRules() error {
	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return err
		}
	}
	if n.Enable6 {
		if err := n.configure(iptables.IPv6, n.Config6); err != nil {
			return err
		}
	}
	return nil
}

func (n *iptablesNetwork) delNetworkLevelRules() error {
	var errs []error
	for _, cleanFunc := range n.cleanFuncs {
		if err := cleanFunc(); err != nil {
			errs = append(errs, err)
		}
	}
	n.cleanFuncs = nil
	return errors.Join(errs...)
}

func (n *iptablesNetwork) configure(ipv iptables.IPVersion, conf networkConfigFam) error {
	if !conf.Prefix.IsValid() {
		// Delete INC rules, in case they were created by a 28.0.0 daemon that didn't check
		// whether the network had iptables/ip6tables enabled.
		// This preserves https://github.com/moby/moby/commit/8cc4d1d4a2b6408232041f9ba4dff966eba80cc0
		return setINC(ipv, n.IfName, conf.Routed, false)
	}
	if err := n.setupIPTables(ipv, conf); err != nil {
		return err
Repository: moby/moby
File: libnetwork/drivers/bridge/setup_ip_tables_linux.go (Go)

Code before:

	return nil
}

type networkConfigFam struct {
	HostIP      netip.Addr
	Prefix      netip.Prefix
	Routed      bool
	Unprotected bool
}

type networkConfig struct {
	IfName     string
	Internal   bool
	ICC        bool
	Masquerade bool
	Config4    networkConfigFam
	Config6    networkConfigFam
	Hairpin    bool
	Enable4    bool
	Enable6    bool
}

type iptablesNetwork struct {
	networkConfig
	cleanFuncs iptablesCleanFuncs
}

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.cleanup()
		}
	}()

	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return nil, err
		}
	}
	if n.Enable6 {
		if err := n.configure(iptables.IPv6, n.Config6); err != nil {
			return nil, err
		}
	}
	return n, nil
}


Review comment (bug):
I think clean up functions aren't even needed if all delete functions are made idempotent. That is, if `configure` fails, it's caller's responsibility to delete the network altogether. This would also solve the zombie rules issue described here: https://github.com/moby/moby/pull/49668/files#diff-93cf2916a163f32710fd2d4cc0845eea796989ba93c0dc2c9418f3eaeab4adccR10-R11

Maybe something to keep in mind for a follow-up?

Code after:

	return nil
}

type networkConfigFam struct {
	HostIP      netip.Addr
	Prefix      netip.Prefix
	Routed      bool
	Unprotected bool
}

type networkConfig struct {
	IfName     string
	Internal   bool
	ICC        bool
	Masquerade bool
	Config4    networkConfigFam
	Config6    networkConfigFam
	Hairpin    bool
	Enable4    bool
	Enable6    bool
}

type iptablesNetwork struct {
	networkConfig
	cleanFuncs iptablesCleanFuncs
}

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.delNetworkLevelRules()
		}
	}()

	if err := n.reapplyNetworkLevelRules(); err != nil {
		return nil, err
	}
	return n, nil
}

func (n *iptablesNetwork) reapplyNetworkLevelRules() error {
	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return err
		}
	}
	if n.Enable6 {
Repository: moby/moby
File: libnetwork/drivers/bridge/setup_ip_tables_linux.go (Go)

Code before:

type networkConfigFam struct {
	HostIP      netip.Addr
	Prefix      netip.Prefix
	Routed      bool
	Unprotected bool
}

type networkConfig struct {
	IfName     string
	Internal   bool
	ICC        bool
	Masquerade bool
	Config4    networkConfigFam
	Config6    networkConfigFam
	Hairpin    bool
	Enable4    bool
	Enable6    bool
}

type iptablesNetwork struct {
	networkConfig
	cleanFuncs iptablesCleanFuncs
}

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.cleanup()
		}
	}()

	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return nil, err
		}
	}
	if n.Enable6 {
		if err := n.configure(iptables.IPv6, n.Config6); err != nil {
			return nil, err
		}
	}
	return n, nil
}

func (n *iptablesNetwork) delNetworkLevelRules() error {
	return n.cleanup()
}

Review comment (suggestion):
Looking at your `Firewaller` interface in https://github.com/moby/moby/issues/49635, I guess this function will be called by iptabler's implementation of`NewNetwork(nc NetworkConfig) (Network, error)`.

Would it make sense to separate `NewNetwork` from `configure`, such that there's a `Setup` function (on `Firewaller` or maybe `Network`) that does set up the network ?

I think it's surprising to have a `new` function that actually has side-effects that go beyond the initialization of the immediate struct.

Code after:

type networkConfigFam struct {
	HostIP      netip.Addr
	Prefix      netip.Prefix
	Routed      bool
	Unprotected bool
}

type networkConfig struct {
	IfName     string
	Internal   bool
	ICC        bool
	Masquerade bool
	Config4    networkConfigFam
	Config6    networkConfigFam
	Hairpin    bool
	Enable4    bool
	Enable6    bool
}

type iptablesNetwork struct {
	networkConfig
	cleanFuncs iptablesCleanFuncs
}

func newIptablesNetwork(nc networkConfig) (_ *iptablesNetwork, retErr error) {
	n := &iptablesNetwork{
		networkConfig: nc,
	}
	defer func() {
		if retErr != nil {
			n.delNetworkLevelRules()
		}
	}()

	if err := n.reapplyNetworkLevelRules(); err != nil {
		return nil, err
	}
	return n, nil
}

func (n *iptablesNetwork) reapplyNetworkLevelRules() error {
	if n.Enable4 {
		if err := n.configure(iptables.IPv4, n.Config4); err != nil {
			return err
		}
	}
	if n.Enable6 {
		if err := n.configure(iptables.IPv6, n.Config6); err != nil {
			return err
		}
Repository: moby/moby
File: daemon/build.go (Go)

Code before:
package daemon

import (
	"context"
	"strings"

	cerrdefs "github.com/containerd/errdefs"
	"github.com/containerd/log"
	"github.com/distribution/reference"
	"github.com/docker/docker/api/types/events"
	c8dexporter "github.com/docker/docker/builder/builder-next/exporter"
	"github.com/moby/buildkit/exporter"
	"github.com/moby/buildkit/exporter/containerimage/exptypes"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

// ImageExportedByBuildkit is a callback that is called when an image is exported by buildkit.
// This is used to log the image creation event for untagged images.
// When no tag is given, buildkit doesn't call the image service so it has no
// way of knowing the image was created.
func (daemon *Daemon) ImageExportedByBuildkit(ctx context.Context, src *exporter.Source, attrs map[string]string) c8dexporter.ExportedCallback {
	if danglingPrefix, ok := attrs[string(exptypes.OptKeyDanglingPrefix)]; ok && danglingPrefix != "" {
		if imgSvc, ok := daemon.imageService.(interface {
			EnsureDanglingImage(ctx context.Context, refOrID string) error
		}); ok {
			imageName := attrs[string(exptypes.OptKeyName)]

			// The frontend can provide a name if requested.
			if imageName == "*" {
				imageName = string(src.Metadata["image.name"])
			}

			if imageName != "" {
				targetNames := strings.Split(imageName, ",")
				for _, targetName := range targetNames {
					if err := imgSvc.EnsureDanglingImage(ctx, targetName); err != nil && !cerrdefs.IsNotFound(err) {
						log.G(ctx).WithError(err).Warn("failed to keep the previous image as dangling")
					}

Review comment (suggestion):
I think we should try to keep as much buildkit stuff inside the `builder` package and avoid any dependencies on the `buildkit` package inside `daemon` package.



Code after:
package daemon

import (
	"context"

	"github.com/docker/docker/api/types/events"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

// ImageExportedByBuildkit is a callback that is called when an image is exported by buildkit.
// This is used to log the image creation event for untagged images.
// When no tag is given, buildkit doesn't call the image service so it has no
// way of knowing the image was created.
func (daemon *Daemon) ImageExportedByBuildkit(ctx context.Context, id string, desc ocispec.Descriptor) {
	daemon.imageService.LogImageEvent(ctx, id, id, events.ActionCreate)
}
Repository: moby/moby
File: api/types/types.go (Go)

Code before:
	BuildCache *BuildCacheDiskUsage
}

// ImageDiskUsage contains the image disk usage information.
type ImageDiskUsage struct {
	TotalSize   int64
	Reclaimable int64
	Items       []*image.Summary
}

// ContainerDiskUsage contains the container disk usage information.
type ContainerDiskUsage struct {
	TotalSize   int64
	Reclaimable int64
	Items       []*container.Summary
}

// VolumeDiskUsage contains the container disk usage information.
type VolumeDiskUsage struct {
	TotalSize   int64
	Reclaimable int64
	Items       []*volume.Volume
}

// BuildCacheDiskUsage contains the build cache disk usage information.
type BuildCacheDiskUsage struct {
	TotalSize   int64
	Reclaimable int64
	Items       []*BuildCache
}

// BuildCachePruneReport contains the response for Engine API:
// POST "/build/prune"
type BuildCachePruneReport struct {
	CachesDeleted  []string
	SpaceReclaimed uint64
}

// SecretCreateResponse contains the information returned to a client
// on the creation of a new secret.
type SecretCreateResponse struct {
	// ID is the id of the created secret.
	ID string
}

// SecretListOptions holds parameters to list secrets
type SecretListOptions struct {
	Filters filters.Args
}

// ConfigCreateResponse contains the information returned to a client

Review comment (suggestion):
I think "build" was one of the remaining parts that didn't have a scoped package yet; initially that was planned to be part of `images` (as; `docker image build`) but now that build does more than just "images", possibly we should have a package for this under `api/types/` (and corresponding parts for backend etc).

Code after:
	BuildCache  []*BuildCache
	BuilderSize int64 `json:",omitempty"` // Deprecated: deprecated in API 1.38, and no longer used since API 1.40.
}

// BuildCachePruneReport contains the response for Engine API:
// POST "/build/prune"
type BuildCachePruneReport struct {
	CachesDeleted  []string
	SpaceReclaimed uint64
}

// SecretCreateResponse contains the information returned to a client
// on the creation of a new secret.
type SecretCreateResponse struct {
	// ID is the id of the created secret.
	ID string
}

// SecretListOptions holds parameters to list secrets
type SecretListOptions struct {
	Filters filters.Args
}

// ConfigCreateResponse contains the information returned to a client
// on the creation of a new config.
type ConfigCreateResponse struct {
	// ID is the id of the created config.
	ID string
}

// ConfigListOptions holds parameters to list configs
type ConfigListOptions struct {
	Filters filters.Args
}

// PushResult contains the tag, manifest digest, and manifest size from the
// push. It's used to signal this information to the trust code in the client
// so it can sign the manifest if necessary.
type PushResult struct {
	Tag    string
	Digest string
	Size   int
}

// BuildResult contains the image id of a successful build
type BuildResult struct {
	ID string
}

// BuildCache contains information about a build cache record.
//
Repository: moby/moby
File: daemon/disk_usage.go (Go)

Code before:
				}
				du.TotalSize += v.UsageData.Size
			}
		}
		return du, nil
	})
	return volumes, err
}

type layerDiskUsage struct {
	LayersSize  int64
	ContentSize int64
}

// layerDiskUsage obtains information about layer disk usage from image service
// and makes sure that only one size calculation is performed at the same time.
func (daemon *Daemon) layerDiskUsage(ctx context.Context) (int64, error) {
	usage, _, err := daemon.usageLayer.Do(ctx, struct{}{}, func(ctx context.Context) (usage int64, err error) {
		return daemon.imageService.ImageDiskUsage(ctx)
	})
	return usage, err
}

// SystemDiskUsage returns information about the daemon data disk usage.
// Callers must not mutate contents of the returned fields.
func (daemon *Daemon) SystemDiskUsage(ctx context.Context, opts system.DiskUsageOptions) (du *types.DiskUsage, err error) {
	eg, ctx := errgroup.WithContext(ctx)

	du = &types.DiskUsage{}
	if opts.Containers {
		eg.Go(func() (err error) {
			du.Containers, err = daemon.containerDiskUsage(ctx)
			return err
		})
	}

	var (
		layersSize int64
		images     []*image.Summary
	)
	if opts.Images {
		eg.Go(func() (err error) {
			images, err = daemon.imageDiskUsage(ctx)
			return err
		})
		eg.Go(func() (err error) {
			layersSize, err = daemon.layerDiskUsage(ctx)
			return err
		})
	}


Review comment (suggestion):
Try to avoid named output variables; I don't think we need them in this case. We try to avoid them and only use them in exceptional cases (usually if there's an error to be handled in a `defer`, and in that case we make sure it's not named `err`, but (e.g.) `retErr` to make it less err-prone (we've had various cases where `err` was accidentally shadowed).

Code after:
				}
				du.TotalSize += v.UsageData.Size
			}
		}
		return du, nil
	})
	return volumes, err
}

// layerDiskUsage obtains information about layer disk usage from image service
// and makes sure that only one size calculation is performed at the same time.
func (daemon *Daemon) layerDiskUsage(ctx context.Context) (int64, error) {
	usage, _, err := daemon.usageLayer.Do(ctx, struct{}{}, func(ctx context.Context) (usage int64, err error) {
		return daemon.imageService.ImageDiskUsage(ctx)
	})
	return usage, err
}

// SystemDiskUsage returns information about the daemon data disk usage.
// Callers must not mutate contents of the returned fields.
func (daemon *Daemon) SystemDiskUsage(ctx context.Context, opts system.DiskUsageOptions) (*systemtypes.DiskUsage, error) {
	eg, ctx := errgroup.WithContext(ctx)

	du := &systemtypes.DiskUsage{}
	if opts.Containers {
		eg.Go(func() (err error) {
			du.Containers, err = daemon.containerDiskUsage(ctx)
			return err
		})
	}

	var (
		layersSize int64
		images     []*image.Summary
	)
	if opts.Images {
		eg.Go(func() (err error) {
			images, err = daemon.imageDiskUsage(ctx)
			return err
		})
		eg.Go(func() (err error) {
			layersSize, err = daemon.layerDiskUsage(ctx)
			return err
		})
	}

	if opts.Volumes {
		eg.Go(func() (err error) {
			du.Volumes, err = daemon.localVolumesSize(ctx)
			return err
		})
Repository: moby/moby
File: contrib/dockerd-rootless-setuptool.sh (Shell)

Code before:
		elif command -v yum > /dev/null 2>&1; then
			instructions=$(
				cat <<- EOI
					${instructions}
					# Install newuidmap & newgidmap binaries
					yum install -y shadow-utils
				EOI
			)
		else
			ERROR "newuidmap binary not found. Please install with a package manager."
			exit 1
		fi
	fi

	# instruction: iptables dependency check
	faced_iptables_error=""
	if ! command -v iptables > /dev/null 2>&1 && [ ! -f /sbin/iptables ] && [ ! -f /usr/sbin/iptables ]; then
		faced_iptables_error=1
		if [ -z "$OPT_SKIP_IPTABLES" ]; then
			if command -v apt-get > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						apt-get install -y iptables
					EOI
				)
			elif command -v dnf > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						dnf install -y iptables
					EOI
				)
			elif command -v yum > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						yum install -y iptables
					EOI
				)
			else
				ERROR "iptables binary not found. Please install with a package manager."
				exit 1
			fi
		fi
	fi

	# instruction: ip_tables module dependency check

Review comment (suggestion):
This sets `faced_iptables_error=1` even when iptables are available

Code after:
		elif command -v yum > /dev/null 2>&1; then
			instructions=$(
				cat <<- EOI
					${instructions}
					# Install newuidmap & newgidmap binaries
					yum install -y shadow-utils
				EOI
			)
		else
			ERROR "newuidmap binary not found. Please install with a package manager."
			exit 1
		fi
	fi

	# instruction: iptables dependency check
	faced_iptables_error=""
	# Many OSs now use iptables-nft by default so, check for module nf_tables by default. But,
	# if "iptables --version" worked and reported "legacy", check for module ip_tables instead.
	iptables_module="nf_tables"
	iptables_command=$(PATH=$PATH:/sbin:/usr/sbin command -v iptables 2> /dev/null) || :
	if [ -n "$iptables_command" ]; then
		iptables_version=$($iptables_command --version 2> /dev/null) || :
		case $iptables_version in
			*legacy*) iptables_module="ip_tables" ;;
		esac
		faced_iptables_error=1
		if [ -z "$OPT_SKIP_IPTABLES" ]; then
			if command -v apt-get > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						apt-get install -y iptables
					EOI
				)
			elif command -v dnf > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						dnf install -y iptables
					EOI
				)
			elif command -v yum > /dev/null 2>&1; then
				instructions=$(
					cat <<- EOI
						${instructions}
						# Install iptables
						yum install -y iptables
					EOI
				)