ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: daemon/volume/mounts/linux_parser_test.go (Go)

Code before:
					Source: "/tmp",
					Target: "/tmp1",
					Type:   mount.TypeBind,
				},
			},
		},
		{
			mount: mount.Mount{
				Target:   "/data/anonymous-read-only-volume",
				ReadOnly: true,
				Type:     mount.TypeVolume,
			},
			expected: &MountPoint{
				Destination: "/data/anonymous-read-only-volume",
				Type:        mount.TypeVolume,
				CopyData:    true,
				Spec: mount.Mount{
					Target:   "/data/anonymous-read-only-volume",
					ReadOnly: true,
					Type:     mount.TypeVolume,
				},
			},
		},
		{
			mount: mount.Mount{
				Source: "/invalid/source/path",
				Target: "/data/anonymous-read-only-volume",
				Type:   mount.TypeBind,
			},
			expErr: `invalid mount config for type "bind": bind source path does not exist: /invalid/source/path`,
		},
		{
			mount: mount.Mount{
				Target: "/data/invalid-type",
				Type:   "invalid",
			},
			expErr: `invalid mount config for type "invalid": mount type unknown`,
		},
	}

	parser := NewLinuxParser()

	for _, tc := range tests {
		m, err := parser.ParseMountSpec(tc.mount)
		if tc.expErr != "" {
			assert.Check(t, is.Nil(m))
			assert.Check(t, is.Error(err, tc.expErr))
			continue
		}

		assert.NilError(t, err)

Review comment (suggestion):
Not _strictly_ needed, but to prevent any case where `"/data/anonymous-read-only-volume"` either exists (very unlikely 😅), or gets created implicitly (shouldn't happen), you could use the `tmpDir` here as well; for example;


```suggestion
				Source: filepath.Join(tmpDir, "no-such-path"),
```

Code after:
			},
			expected: &MountPoint{
				Source:      tmpDir,
				Destination: "/tmp1",
				RW:          true,
				Type:        mount.TypeBind,
				Propagation: "rprivate",
				Spec: mount.Mount{
					Source: tmpDir,
					Target: "/tmp1",
					Type:   mount.TypeBind,
				},
			},
		},
		{
			doc: "read-only",
			mount: mount.Mount{
				Target:   "/data/anonymous-read-only-volume",
				ReadOnly: true,
				Type:     mount.TypeVolume,
			},
			expected: &MountPoint{
				Destination: "/data/anonymous-read-only-volume",
				Type:        mount.TypeVolume,
				CopyData:    true,
				Spec: mount.Mount{
					Target:   "/data/anonymous-read-only-volume",
					ReadOnly: true,
					Type:     mount.TypeVolume,
				},
			},
		},
		{
			doc: "invalid source path",
			mount: mount.Mount{
				Source: "/tmp/definitely-no-such-path",
				Target: "/data/anonymous-read-only-volume",
				Type:   mount.TypeBind,
			},
			expErr: `invalid mount config for type "bind": bind source path does not exist: /tmp/definitely-no-such-path`,
		},
		{
			doc: "invalid mount type",
			mount: mount.Mount{
				Target: "/data/invalid-type",
				Type:   "invalid",
			},
			expErr: `invalid mount config for type "invalid": mount type unknown`,
		},
	}
Repository: moby/moby
File: hack/vendor.sh (Shell)

Code before:

	go mod edit -modfile client/go.mod -require="github.com/moby/moby/api@$ref"
	(cd client; go mod tidy)

	go mod edit \
		-require="github.com/moby/moby/api@$ref" \
		-require="github.com/moby/moby/client@$ref"
	go mod tidy
	go mod vendor
)

help() {
	printf "%s:\n" "$(basename "$0")"
	echo "  - tidy: run go mod tidy"
	echo "  - vendor: run go mod vendor"
	echo "  - replace: run go mod edit replace for local modules"
	echo "  - dropreplace: run go mod edit dropreplace for local modules"
	echo "  - all: run tidy && vendor"
	echo "  - help: show this help"
}

case "$1" in
	tidy) tidy ;;
	vendor) vendor ;;
	replace) replace ;;
	dropreplace) dropreplace "$2" ;;
	""|all) tidy && vendor ;;
	*) help ;;
esac


Review comment (suggestion):
Maybe worth updating the help text just above? Something like ...

```
echo "  - dropreplace [<gitref>]: run go mod edit dropreplace for local modules"
```

Code after:

	go mod edit -modfile client/go.mod -require="github.com/moby/moby/api@$ref"
	(cd client; go mod tidy)

	go mod edit \
		-require="github.com/moby/moby/api@$ref" \
		-require="github.com/moby/moby/client@$ref"
	go mod tidy
	go mod vendor
)

help() {
	printf "%s:\n" "$(basename "$0")"
	echo "  - tidy: run go mod tidy"
	echo "  - vendor: run go mod vendor"
	echo "  - replace: run go mod edit replace for local modules"
	echo "  - dropreplace [<gitref>]: remove replace rules and update the api and client modules to the provided ref (defaults to upstream/master)"
	echo "  - all: run tidy && vendor"
	echo "  - help: show this help"
}

case "$1" in
	tidy) tidy ;;
	vendor) vendor ;;
	replace) replace ;;
	dropreplace) dropreplace "$2" ;;
	""|all) tidy && vendor ;;
	*) help ;;
esac
Repository: moby/moby
File: vendor/github.com/theupdateframework/go-tuf/v2/metadata/keys.go (Go)

Code before:
		pemKey, err := cryptoutils.MarshalPublicKeyToPEM(k)
		if err != nil {
			return nil, err
		}
		key.Value.PublicKey = string(pemKey)
	case *ecdsa.PublicKey:
		key.Type = KeyTypeECDSA_SHA2_P256
		key.Scheme = KeySchemeECDSA_SHA2_P256
		pemKey, err := cryptoutils.MarshalPublicKeyToPEM(k)
		if err != nil {
			return nil, err
		}
		key.Value.PublicKey = string(pemKey)
	case ed25519.PublicKey:
		key.Type = KeyTypeEd25519
		key.Scheme = KeySchemeEd25519
		key.Value.PublicKey = hex.EncodeToString(k)
	default:
		return nil, fmt.Errorf("unsupported public key type")
	}
	return key, nil
}

// ID returns the keyID value for the given Key
func (k *Key) ID() string {
	// the identifier is a hexdigest of the SHA-256 hash of the canonical form of the key
	if k.id == "" {
		data, err := cjson.EncodeCanonical(k)
		if err != nil {
			panic(fmt.Errorf("error creating key ID: %w", err))
		}
		digest := sha256.Sum256(data)
		k.id = hex.EncodeToString(digest[:])
	}
	return k.id
}


Review comment (suggestion):
Interesting ... looks like this is a breaking change (doesn't appear to impact us though); probably would've required a major version update.

Looks like they called it out as breaking change, but ... would've been better to make that a major version update; https://github.com/theupdateframework/go-tuf/releases/tag/v2.4.0

Code after:
		pemKey, err := cryptoutils.MarshalPublicKeyToPEM(k)
		if err != nil {
			return nil, err
		}
		key.Value.PublicKey = string(pemKey)
	case *ecdsa.PublicKey:
		key.Type = KeyTypeECDSA_SHA2_P256
		key.Scheme = KeySchemeECDSA_SHA2_P256
		pemKey, err := cryptoutils.MarshalPublicKeyToPEM(k)
		if err != nil {
			return nil, err
		}
		key.Value.PublicKey = string(pemKey)
	case ed25519.PublicKey:
		key.Type = KeyTypeEd25519
		key.Scheme = KeySchemeEd25519
		key.Value.PublicKey = hex.EncodeToString(k)
	default:
		return nil, fmt.Errorf("unsupported public key type")
	}
	return key, nil
}

// ID returns the keyID value for the given Key, or an error if the key
// cannot be canonically encoded.
func (k *Key) ID() (string, error) {
	// the identifier is a hexdigest of the SHA-256 hash of the canonical form of the key
	if k.id == "" {
		data, err := cjson.EncodeCanonical(k)
		if err != nil {
			return "", fmt.Errorf("error creating key ID: %w", err)
		}
		digest := sha256.Sum256(data)
		k.id = hex.EncodeToString(digest[:])
	}
	return k.id, nil
}
Repository: moby/moby
File: daemon/libnetwork/osl/interface_linux.go (Go)

Code before:
	address6 := i.AddressIPv6()
	ctx = log.WithLogger(ctx, log.G(ctx).WithFields(log.Fields{
		"iface":        i.dstName,
		"ifi":          ifIndex,
		"mac":          mac.String(),
		"ip4":          address4,
		"ip6":          address6,
		"mcastRouteOk": mcastRouteOk,
	}))

	if address4 == nil && address6 == nil {
		// Nothing to do - for example, a bridge with no configured addresses.
		log.G(ctx).Debug("No IP addresses to advertise")
		return nil
	}
	if mac == nil || len(mac) == 0 {
		// Nothing to do - for example, a layer-3 ipvlan.
		log.G(ctx).Debug("No MAC address to advertise")
		return nil
	}
	if i.advertiseAddrNMsgs == 0 {
		log.G(ctx).Debug("Unsolicited ARP/NA is disabled")
		return nil
	}

	log.G(ctx).Info("Sending neighbor advertisements")

	arpSender, naSender := n.prepAdvertiseAddrs(ctx, i, ifIndex)
	if arpSender == nil && naSender == nil {
		return nil
	}
	cleanup := func() {
		if arpSender != nil {
			arpSender.Close()
		}
		if naSender != nil {
			naSender.Close()
		}
	}
	stillSending := false
	defer func() {
		if !stillSending {
			cleanup()
		}
	}()

	send := func(ctx context.Context) error {
		link, err := nlh.LinkByIndex(ifIndex)
		if err != nil {
			return fmt.Errorf("failed to refresh link attributes: %w", err)
		}

Review comment (bug):
I don't think we should log this at Info level, and it's logged later at debug level anyway. So it's probably be best to remove this.

Code after:
	address4 := i.Address()
	address6 := i.AddressIPv6()
	ctx = log.WithLogger(ctx, log.G(ctx).WithFields(log.Fields{
		"iface":        i.dstName,
		"ifi":          ifIndex,
		"mac":          mac.String(),
		"ip4":          address4,
		"ip6":          address6,
		"mcastRouteOk": mcastRouteOk,
	}))

	if address4 == nil && address6 == nil {
		// Nothing to do - for example, a bridge with no configured addresses.
		log.G(ctx).Debug("No IP addresses to advertise")
		return nil
	}
	if len(mac) == 0 {
		// Nothing to do - for example, a layer-3 ipvlan.
		log.G(ctx).Debug("No MAC address to advertise")
		return nil
	}
	if i.advertiseAddrNMsgs == 0 {
		log.G(ctx).Debug("Unsolicited ARP/NA is disabled")
		return nil
	}

	arpSender, naSender := n.prepAdvertiseAddrs(ctx, i, ifIndex, mac)
	if arpSender == nil && naSender == nil {
		return nil
	}
	cleanup := func() {
		if arpSender != nil {
			arpSender.Close()
		}
		if naSender != nil {
			naSender.Close()
		}
	}
	stillSending := false
	defer func() {
		if !stillSending {
			cleanup()
		}
	}()

	send := func(ctx context.Context) error {
		link, err := nlh.LinkByIndex(ifIndex)
		if err != nil {
			return fmt.Errorf("failed to refresh link attributes: %w", err)
		}
		if curMAC := link.Attrs().HardwareAddr; !bytes.Equal(curMAC, mac) {
Repository: moby/moby
File: daemon/command/service_windows.go (Go)

Code before:
	err = s.Delete()
	if err != nil {
		return err
	}
	return nil
}

// initService is the entry point for running the daemon as a Windows
// service. It returns an indication to stop (if registering/un-registering);
// an indication of whether it is running as a service; and an error.
func initService(cli *daemonCLI) (bool, bool, error) {
	if *flUnregisterService {
		if *flRegisterService {
			return true, false, errors.New("--register-service and --unregister-service cannot be used together")
		}
		return true, false, unregisterService()
	}

	if *flRegisterService {
		return true, false, registerService()
	}

	if !*flRunService {
		return false, false, nil
	}

	// Check if we're running as a Windows service or interactively.
	isService, err := svc.IsWindowsService()
	if err != nil {
		return false, false, err
	}

	h := &handler{
		tosvc:     make(chan bool),
		fromsvc:   make(chan error),
		daemonCLI: cli,
	}

	var eventLog *eventlog.Log
	if isService {
		eventLog, err = eventlog.Open(*flServiceName)
		if err != nil {
			return false, false, err
		}
	}

	log.L.Logger.AddHook(&etwHook{eventLog})
	log.L.Logger.SetOutput(io.Discard)

	service = h
	go func() {

Review comment (suggestion):
I chose to produce the warning here, instead of the other location, otherwise we'd end up with;

- a warning
- followed by an error

With the `sync.OnceValue`, calling it multiple times should no longer have a cost, so calling it in multiple places won't harm (other than a bit more code sprinkled throughout 😓 😅 

Code after:
	eventlog.Remove(*flServiceName)
	err = s.Delete()
	if err != nil {
		return err
	}
	return nil
}

// initService is the entry point for running the daemon as a Windows
// service. It returns an indication to stop (if registering/un-registering);
// an indication of whether it is running as a service; and an error.
func initService(ctx context.Context, cli *daemonCLI) (bool, bool, error) {
	if *flUnregisterService {
		if *flRegisterService {
			return true, false, errors.New("--register-service and --unregister-service cannot be used together")
		}
		return true, false, unregisterService()
	}

	if *flRegisterService {
		if err := registerService(); err != nil {
			return false, false, err
		}
		if err := daemon.CheckSystem(); err != nil {
			log.G(ctx).Warnf("Windows service registration was successful but system requirements were not met: %v", err)
		}
		return true, false, nil
	}

	if !*flRunService {
		return false, false, nil
	}

	// Check if we're running as a Windows service or interactively.
	isService, err := svc.IsWindowsService()
	if err != nil {
		return false, false, err
	}

	h := &handler{
		tosvc:     make(chan bool),
		fromsvc:   make(chan error),
		daemonCLI: cli,
	}

	var eventLog *eventlog.Log
	if isService {
		eventLog, err = eventlog.Open(*flServiceName)
		if err != nil {
			return false, false, err
		}
Repository: moby/moby
File: daemon/volume/service/service.go (Go)

Code before:
	}

	rep := &volumetypes.PruneReport{VolumesDeleted: make([]string, 0, len(ls))}
	for _, v := range ls {
		select {
		case <-ctx.Done():
			err := ctx.Err()
			if err == context.Canceled {
				err = nil
			}
			return rep, err
		default:
		}

		vSize, err := directory.Size(ctx, v.Path())
		if err != nil {
			log.G(ctx).WithFields(log.Fields{
				"error":  err,
				"volume": v.Name(),
			}).Warn("could not determine size of volume")
		}
		if err := s.vs.Remove(ctx, v); err != nil {
			log.G(ctx).WithFields(log.Fields{
				"error":  err,
				"volume": v.Name(),
			}).Warn("Could not determine size of volume")
			continue
		}
		rep.SpaceReclaimed += uint64(vSize)
		rep.VolumesDeleted = append(rep.VolumesDeleted, v.Name())
	}
	s.eventLogger.LogVolumeEvent("", events.ActionPrune, map[string]string{
		"reclaimed": strconv.FormatInt(int64(rep.SpaceReclaimed), 10),
	})
	return rep, nil
}

// List gets the list of volumes which match the past in filters
// If filters is nil or empty all volumes are returned.
func (s *VolumesService) List(ctx context.Context, filter filters.Args) (volumes []volumetypes.Volume, warnings []string, _ error) {
	by, err := filtersToBy(filter, acceptedListFilters)
	if err != nil {
		return nil, nil, err
	}

	vols, warns, err := s.vs.Find(ctx, by)
	if err != nil {
		return nil, nil, err
	}

	return s.volumesToAPI(ctx, vols, useCachedPath(true)), warns, nil

Review comment (suggestion):
```suggestion
			}).Warn("Could not remove volume")
```

Code after:
	}

	rep := &volumetypes.PruneReport{VolumesDeleted: make([]string, 0, len(ls))}
	for _, v := range ls {
		select {
		case <-ctx.Done():
			err := ctx.Err()
			if err == context.Canceled {
				err = nil
			}
			return rep, err
		default:
		}

		vSize, err := directory.Size(ctx, v.Path())
		if err != nil {
			log.G(ctx).WithFields(log.Fields{
				"error":  err,
				"volume": v.Name(),
			}).Warn("could not determine size of volume")
		}
		if err := s.vs.Remove(ctx, v); err != nil {
			log.G(ctx).WithFields(log.Fields{
				"error":  err,
				"volume": v.Name(),
			}).Warn("Could not remove volume")
			continue
		}
		rep.SpaceReclaimed += uint64(vSize)
		rep.VolumesDeleted = append(rep.VolumesDeleted, v.Name())
	}
	s.eventLogger.LogVolumeEvent("", events.ActionPrune, map[string]string{
		"reclaimed": strconv.FormatInt(int64(rep.SpaceReclaimed), 10),
	})
	return rep, nil
}

// List gets the list of volumes which match the past in filters
// If filters is nil or empty all volumes are returned.
func (s *VolumesService) List(ctx context.Context, filter filters.Args) (volumes []volumetypes.Volume, warnings []string, _ error) {
	by, err := filtersToBy(filter, acceptedListFilters)
	if err != nil {
		return nil, nil, err
	}

	vols, warns, err := s.vs.Find(ctx, by)
	if err != nil {
		return nil, nil, err
	}

	return s.volumesToAPI(ctx, vols, useCachedPath(true)), warns, nil
Repository: moby/moby
File: daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go (Go)

Code before:
	"github.com/containerd/log"
	"github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge/internal/firewaller"
	"github.com/moby/moby/v2/daemon/libnetwork/internal/nftables"
)

// Cleanup deletes all rules created by Nftabler; it's intended to be used
// during startup, to clean up rules created by an old incarnation of the daemon
// after switching to a different Firewaller implementation.
func Cleanup(ctx context.Context, config firewaller.Config) {
	if config.IPv4 {
		if err := exec.Command("nft", "delete", "table", string(nftables.IPv4), dockerTable).Run(); err != nil {
			log.G(ctx).WithError(err).Info("Deleting nftables IPv4 rules")
		} else {
			log.G(ctx).Info("Deleted nftables IPv4 rules")
		}
	}
	if config.IPv6 {
		if err := exec.Command("nft", "delete", "table", string(nftables.IPv6), dockerTable).Run(); err != nil {
			log.G(ctx).WithError(err).Info("Deleting nftables IPv6 rules")
		} else {
			log.G(ctx).Info("Deleted nftables IPv6 rules")
		}
	}
}

func (nft *Nftabler) SetFirewallCleaner(fc firewaller.FirewallCleaner) {
	nft.cleaner = fc
}


Review comment (suggestion):
Slight cleanup of the log, because `nft` produces a "pretty" log with hints;

```console
Error: Could not process rule: No such file or directory
delete table ip docker-bridges
 ^^^^^^^^^^^^^^
```


Which made the logs rather ugly;

```console
INFO[2026-02-21T13:38:21.546483803Z] Deleting nftables IPv4 rules error="exit status 1" output="Error: Could not process rule: No such file or directory\ndelete table ip docker-bridges\n ^^^^^^^^^^^^^^\n"
INFO[2026-02-21T13:38:21.570983053Z] Deleting nftables IPv6 rules error="exit status 1" output="Error: Could not process rule: No such file or directory\ndelete table ip6 docker-bridges\n ^^^^^^^^^^^^^^\n"
```


Code after:

	"github.com/containerd/log"
	"github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge/internal/firewaller"
	"github.com/moby/moby/v2/daemon/libnetwork/internal/nftables"
)

// Cleanup deletes all rules created by Nftabler; it's intended to be used
// during startup, to clean up rules created by an old incarnation of the daemon
// after switching to a different Firewaller implementation.
func Cleanup(ctx context.Context, config firewaller.Config) {
	if config.IPv4 {
		tryCleanup(ctx, nftables.IPv4, "IPv4")
	}
	if config.IPv6 {
		tryCleanup(ctx, nftables.IPv6, "IPv6")
	}
}

func tryCleanup(ctx context.Context, family nftables.Family, label string) {
	cmd := exec.CommandContext(ctx, "nft", "delete", "table", string(family), dockerTable)
	out, err := cmd.CombinedOutput()
	if err != nil {
		// May not exist ("Error: Could not process rule: No such file or directory")
		log.G(ctx).WithFields(log.Fields{
			"error":  err,
			"output": string(bytes.TrimRight(out, "\n ^")), // remove "^^^^^" added in nft's error message.
		}).Info("Deleting nftables " + label + " rules")
		return
	}

	log.G(ctx).Info("Deleted nftables " + label + " rules")
}

func (nft *Nftabler) SetFirewallCleaner(fc firewaller.FirewallCleaner) {
	nft.cleaner = fc
}
Repository: moby/moby
File: daemon/command/daemon.go (Go)

Code before:
		builder:  b,
	})
	gs := newGRPCServer(ctx)
	b.backend.RegisterGRPC(gs)
	httpServer.Protocols = &p
	httpServer.Handler = newHTTPHandler(ctx, gs, apiServer.CreateMux(ctx, routers...))

	go d.ProcessClusterNotifications(ctx, c.GetWatchStream())

	cli.setupConfigReloadTrap()

	log.G(ctx).Info("Daemon has completed initialization")

	// Daemon is fully initialized. Start handling API traffic
	// and wait for serve API to complete.
	var (
		apiWG      sync.WaitGroup
		errAPI     = make(chan error, 1)
		apiStartWG sync.WaitGroup
	)

	apiStartWG.Add(len(lss))
	go func() {
		apiStartWG.Wait()
		// All API listeners started handling requests; notify systemd that we're ready.
		notifyReady()
	}()
	for _, ls := range lss {
		apiWG.Go(func() {
			log.G(ctx).Infof("API listen on %s", ls.Addr())
			apiStartWG.Done()
			if err := httpServer.Serve(ls); err != nil && !errors.Is(err, http.ErrServerClosed) {
				log.G(ctx).WithFields(log.Fields{
					"error":    err,
					"listener": ls.Addr(),
				}).Error("ServeAPI error")

				select {
				case errAPI <- err:
				default:
				}
			}
		})
	}
	apiWG.Wait()
	close(errAPI)

	c.Cleanup()

	// notify systemd that we're shutting down
	notifyStopping()

Review comment (bug):
If servers fail to start, is there a race between the two wait groups / could we end up calling `notifyStopping` before `notifyReady`?

Maybe there should be another `apiStartWG.Wait()` before the `apiWG.Wait()`, or   `notifyReady()` shouldn't be called if there's an error?

Code after:
		builder:  b,
	})
	gs := newGRPCServer(ctx)
	b.backend.RegisterGRPC(gs)
	httpServer.Protocols = &p
	httpServer.Handler = newHTTPHandler(ctx, gs, apiServer.CreateMux(ctx, routers...))

	go d.ProcessClusterNotifications(ctx, c.GetWatchStream())

	cli.setupConfigReloadTrap()

	log.G(ctx).Info("Daemon has completed initialization")

	// Daemon is fully initialized. Start handling API traffic
	// and wait for serve API to complete.
	var (
		apiWG      sync.WaitGroup
		errAPI     = make(chan error, 1)
		apiStartWG sync.WaitGroup
	)

	apiStartWG.Add(len(lss))
	for _, ls := range lss {
		apiWG.Go(func() {
			log.G(ctx).Infof("API listen on %s", ls.Addr())
			apiStartWG.Done()
			if err := httpServer.Serve(ls); err != nil && !errors.Is(err, http.ErrServerClosed) {
				log.G(ctx).WithFields(log.Fields{
					"error":    err,
					"listener": ls.Addr(),
				}).Error("ServeAPI error")

				select {
				case errAPI <- err:
				default:
				}
			}
		})
	}

	// Wait for all API listeners to start handling requests.
	apiStartWG.Wait()

	select {
	case <-errAPI:
		// An API listener failed to start; skip notifying systemd that we're ready.
	default:
		// All API listeners started handling requests; notify systemd that we're ready.
		notifyReady()
	}