package/Dockerfile (2)

30-30: Add progress indicators or suppress output when using wget in Dockerfile.

To improve build log readability and avoid excessive output, consider adding --progress=dot:giga to wget commands or suppress output with -q or --no-verbose.

Apply this diff:

 RUN wget -O - https://s3-us-west-1.amazonaws.com/rancher-longhorn/libqcow-alpha-20181117.tar.gz | tar xvzf - -C /usr/src
+RUN wget --progress=dot:giga -O - https://s3-us-west-1.amazonaws.com/rancher-longhorn/libqcow-alpha-20181117.tar.gz | tar xvzf - -C /usr/src

 ...

 RUN cd /usr/src && \
-    wget -O - https://download.libguestfs.org/libnbd/1.13-development/libnbd-1.13.1.tar.gz | tar -xzf - && \
+    wget --progress=dot:giga -O - https://download.libguestfs.org/libnbd/1.13-development/libnbd-1.13.1.tar.gz | tar -xzf - && \
     cd libnbd-1.13.1 && \
     ./configure --disable-ocaml --disable-python --disable-golang; \
     make; \

Also applies to: 38-38

🧰 Tools

🪛 GitHub Check: CodeFactor

[notice] 30-30: package/Dockerfile#L30
Avoid use of wget without progress bar. Use wget --progress=dot:giga <url>. Or consider using -q or -nv (shorthands for --quiet or --no-verbose). (DL3047)

---

37-37: Specify package versions and clean up after zypper install.

To ensure consistent builds and reduce image size, specify exact package versions when installing with zypper and clean up the package cache after installation.

Apply this diff:

 RUN zypper -n install libxml2-devel nbd
+RUN zypper -n install libxml2-devel=<version> nbd=<version> && \
+    zypper clean --all && \
+    rm -rf /var/cache/zypp/*

 ...

 RUN zypper -n install nfs-client nfs4-acl-tools cifs-utils libaio1 sg3_utils nbd \
     iputils iproute2 qemu-tools e2fsprogs && \
-    rm -rf /var/cache/zypp/*
+    zypper clean --all && \
+    rm -rf /var/cache/zypp/*

Replace <version> with the specific versions required.

Also applies to: 59-59

🧰 Tools

🪛 GitHub Check: CodeFactor

[warning] 37-37: package/Dockerfile#L37
Specify version with zypper install -y <package>=<version>. (DL3037)

---

[warning] 37-37: package/Dockerfile#L37
zypper clean missing after zypper use. (DL3036)

pkg/frontend/nbd/frontend.go (4)

55-57: Enhance Error Context in Startup Method

In the Startup method, the error returned by startNbdServer lacks contextual information, which can make debugging difficult. Consider wrapping the error with additional context.

Apply this diff:

func (n *Nbd) Startup(rwu types.ReaderWriterUnmapperAt) error {
    if err := n.startNbdServer(rwu); err != nil {
-       return err
+       return fmt.Errorf("failed to start NBD server: %w", err)
    }
    n.isUp = true
    // ...
}

---

83-91: Check for Errors When Removing Existing Symlink

When removing an existing symlink at devicePath, errors from os.Remove are ignored. This could lead to hidden issues if the file cannot be removed.

Apply this diff:

if _, err := os.Lstat(devicePath); err == nil {
-   os.Remove(devicePath)
+   if err := os.Remove(devicePath); err != nil {
+       return fmt.Errorf("failed to remove existing device symlink: %w", err)
+   }
}

---

96-107: Limit Hardcoded NBD Device Iteration

In getNextFreeNbdDevice, the loop checks only 16 devices. If more NBD devices are available, they won't be utilized. Consider retrieving the maximum number of NBD devices dynamically or making the limit configurable.

Apply this diff:

-func (n *Nbd) getNextFreeNbdDevice() (string, error) {
-   for i := 0; i < 16; i++ {
+func (n *Nbd) getNextFreeNbdDevice(maxDevices int) (string, error) {
+   for i := 0; i < maxDevices; i++ {
        nbdDevicePath := fmt.Sprintf("/dev/nbd%d", i)
        // ...
    }
    return "", fmt.Errorf("no more nbd devices")
}

And update the call site in Startup:

 n.nbdDevicePath, err = n.getNextFreeNbdDevice()
+// Potentially pass the desired maximum devices

---

209-211: Recover Panics in Goroutines

In handleServerConnection, the deferred function captures panics but only logs them. This may hide critical issues. Consider logging at an appropriate level and possibly propagating the error.

vendor/libguestfs.org/libnbd/bindings.go (1)

1429-1434: Unused or Incomplete Struct Definition

The PreadOptargs struct is defined but may not be fully implemented or utilized in the code.

Review the usage of PreadOptargs to ensure it's fully integrated or remove it if unnecessary.

vendor/github.com/pojntfx/go-nbd/pkg/backend/backend.go (1)

5-11: LGTM! Consider adding interface documentation.

The Backend interface is well-designed, combining standard io interfaces with essential block device operations. It's thread-safe by design through the use of ReaderAt/WriterAt.

Consider adding GoDoc comments to document:

• The interface's purpose in NBD context
• Expected behavior of Size() and Sync()
• Thread-safety guarantees

+// Backend defines the interface for NBD backend implementations.
+// It provides thread-safe read/write operations and block device management.
 type Backend interface {
 	io.ReaderAt
 	io.WriterAt
 
+	// Size returns the total size of the backend in bytes
 	Size() (int64, error)
+	// Sync ensures all pending writes are persisted
 	Sync() error
 }

vendor/golang.org/x/sync/errgroup/pre_go120.go (2)

12-15: Consider logging dropped error cause in pre-Go 1.20 implementation

While the implementation provides compatible behavior, it silently drops the error cause which could impact debugging capabilities in older Go versions.

Consider adding debug logging when error cause is dropped:

 func withCancelCause(parent context.Context) (context.Context, func(error)) {
 	ctx, cancel := context.WithCancel(parent)
-	return ctx, func(error) { cancel() }
+	return ctx, func(err error) {
+		if err != nil {
+			// Log dropped error cause for debugging
+			log.Printf("Context cancelled with cause (dropped in pre-Go 1.20): %v", err)
+		}
+		cancel()
+	}
 }

---

1-15: Consider documenting NBD error handling strategy

These foundational pieces handle the low-level NBD operations and error propagation. To ensure reliable NBD integration:

1. Document how NBD-specific errors are propagated through the error cause chain
2. Consider adding error codes/types specific to NBD operations
3. Establish guidelines for handling NBD connection failures and recovery

Would you like help creating this documentation?

vendor/github.com/pojntfx/go-nbd/pkg/protocol/transmission.go (1)

4-14: Consider documenting NBD protocol version compatibility

The magic numbers and constants align with the NBD protocol specification. However, it would be helpful to document which NBD protocol version this implementation targets.

Consider adding a comment block at the top of the constants section:

+// NBD Protocol v2.9.8 constants
+// Reference: https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md

vendor/github.com/pojntfx/go-nbd/pkg/protocol/negotiation.go (1)

3-4: Add comprehensive documentation for protocol constants and structures

While the link to the protocol documentation is helpful, consider adding detailed comments explaining the purpose and usage of each constant and structure for better maintainability.

vendor/github.com/pmorjan/kmod/README.md (3)

10-13: Improve clarity in documentation

The sentence structure could be improved for better readability:

-Compressed files are not supported directly but users can provide a custom function to uncompress and load a module file into the kernel. (**SetInitFunc**).
+Compressed files are not supported directly, but users can provide a custom function to uncompress and load a module file into the kernel (**SetInitFunc**).

🧰 Tools

🪛 LanguageTool

[uncategorized] ~11-~11: Possible missing comma found.
Context: ...pressed files are not supported directly but users can provide a custom function to ...

(AI_HYDRA_LEO_MISSING_COMMA)

---

15-16: Fix grammatical error in example reference

-for an complete example.
+for a complete example.

🧰 Tools

🪛 LanguageTool

[misspelling] ~16-~16: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...probe.go](cmd/modprobe/modprobe.go) for an complete example. ```go // Load uncomp...

(EN_A_VS_AN)

---

63-77: Consider enhancing error handling in example code

The example for loading compressed modules could benefit from improved error handling:

 func modInit(path, params string, flags int) error {
     f, err := os.Open(path)
     if err != nil {
         return err
     }
+    defer f.Close()
     rd, err := xz.NewReader(f)
     if err != nil {
         return err
     }
     buf, err := ioutil.ReadAll(rd)
     if err != nil {
         return err
     }
     return unix.InitModule(buf, params)
 }

pkg/replica/rpc/dataserver.go (1)

15-18: Consider using boolean for nbdEnabled flag

The nbdEnabled field is used as a boolean check but defined as an int:

 type DataServer struct {
     protocol   types.DataServerProtocol
     address    string
     s          *replica.Server
-    nbdEnabled int
+    nbdEnabled bool
 }

pkg/types/types.go (1)

109-109: Consider using boolean for NBD flag

The nbdEnabled parameter would be more idiomatic as a boolean since it appears to be used as a flag to enable/disable NBD functionality.

-Create(volumeName, address string, dataServerProtocol DataServerProtocol, engineReplicaTimeout time.Duration, nbdEnabled int) (Backend, error)
+Create(volumeName, address string, dataServerProtocol DataServerProtocol, engineReplicaTimeout time.Duration, enableNBD bool) (Backend, error)

go.mod (1)

77-77: Document the reason for using a forked go-nbd package

The replacement of github.com/pojntfx/go-nbd with a fork should be documented to explain why this specific fork is necessary.

Consider adding a comment above the replace directive:

+# Using chazapis fork for enhanced concurrency support needed for Longhorn's NBD implementation
 replace github.com/pojntfx/go-nbd => github.com/chazapis/go-nbd v0.0.0-20231031233644-40daa63e22c3

pkg/backend/file/file.go (1)

127-127: Document why nbdEnabled parameter is unused

The nbdEnabled parameter is added to match the interface but isn't used in the implementation. Consider adding a comment to explain why this parameter is ignored for the file backend.

+// Create implements BackendFactory interface. The nbdEnabled parameter is ignored
+// as the file backend doesn't require NBD-specific handling.
 func (ff *Factory) Create(volumeName, address string, dataServerProtocol types.DataServerProtocol, engineToReplicaTimeout time.Duration, nbdEnabled int) (types.Backend, error) {

vendor/libguestfs.org/libnbd/callbacks.go (1)

69-72: Consider using atomic operations for nextGoCallbackId

The current implementation uses a mutex for protecting both the map and the counter. Consider using atomic operations for nextGoCallbackId to reduce lock contention.

-var nextGoCallbackId int = firstGoCallbackId
+var nextGoCallbackId atomic.Int32
+
+func init() {
+    nextGoCallbackId.Store(firstGoCallbackId)
+}

app/cmd/replica.go (1)

76-81: Consider using bool flag instead of int for nbd-enabled

The current implementation uses an int flag for enabling/disabling NBD. A boolean flag would be more appropriate for this binary choice.

-			cli.IntFlag{
+			cli.BoolFlag{
 				Name:     "nbd-enabled",
 				Required: false,
-				Value:    0,
 				Usage:    "Flag to enable NBD data server",
 			},

app/cmd/controller.go (1)

92-97: Consider adding validation for nbd-enabled flag.

While the implementation looks good, consider adding validation to ensure the number of connections is within reasonable limits to prevent resource exhaustion.

 cli.IntFlag{
   Name:     "nbd-enabled",
   Required: false,
   Value:    0,
+  Usage:    "Flag to enable NBD data server. Option 0 to disable, options 1-32 for number of connections",
-  Usage:    "Flag to enable NBD data server. Option 0 to disable, options >0 for number of connections",
 }

pkg/backend/remote/remote.go (1)

363-370: Consider extracting client creation logic.

The client creation logic could be extracted into a separate method for better maintainability.

+func createDataConnClient(conn net.Conn, engineToReplicaTimeout time.Duration, nbdEnabled int) types.ReaderWriterUnmapperAt {
+    if nbdEnabled > 0 {
+        return dataconn.NewNbdClientWrapper(conn, engineToReplicaTimeout, nbdEnabled)
+    }
+    return dataconn.NewClient(conn, engineToReplicaTimeout)
+}

 func (rf *Factory) Create(...) (types.Backend, error) {
     ...
-    if nbdEnabled > 0 {
-        dataConnClientNBD := dataconn.NewNbdClientWrapper(conn, engineToReplicaTimeout, nbdEnabled)
-        r.ReaderWriterUnmapperAt = dataConnClientNBD
-    } else {
-        dataConnClient = dataconn.NewClient(conn, engineToReplicaTimeout)
-        r.ReaderWriterUnmapperAt = dataConnClient
-    }
+    r.ReaderWriterUnmapperAt = createDataConnClient(conn, engineToReplicaTimeout, nbdEnabled)

vendor/libguestfs.org/libnbd/README.md (1)

6-10: Improve URL formatting in markdown

The URLs should be properly formatted using markdown link syntax for better readability and to follow markdown best practices.

Apply this diff:

-Please see the manual to learn how to use this module:
-https://libguestfs.org/libnbd-golang.3.html
+Please see the [manual](https://libguestfs.org/libnbd-golang.3.html) to learn how to use this module.
 
-This is part of libnbd, please check the project at:
-https://gitlab.com/nbdkit/libnbd
+This is part of libnbd, please check the [project repository](https://gitlab.com/nbdkit/libnbd).

🧰 Tools

🪛 Markdownlint (0.35.0)

7-7: null
Bare URL used

(MD034, no-bare-urls)

---

10-10: null
Bare URL used

(MD034, no-bare-urls)

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Verify that libnbd is installed and pkg-config can find it.

# Test: Check if pkg-config can find libnbd
pkg-config --exists --print-errors libnbd

# Expected result: No output and exit code 0 indicate success. Errors suggest that libnbd is not found.

#!/bin/bash
# Let's check the build system and dependencies setup

# Check if there's a Dockerfile or any build instructions
fd -t f "Dockerfile|Makefile|go.mod"

# Check for any build or dependency documentation
fd -t f "README|INSTALL|BUILDING" -e md

# Look for any build-related environment variables or libnbd references
rg -i "libnbd|PKG_CONFIG" -A 3 -B 3

#!/bin/bash
# Description: Check for dynamic library dependencies that might affect container portability

# Find all binary files in the repository
fd -t x -x ldd {} 2>/dev/null || true

# Check if NBD-related libraries are available in the base container
grep -r "FROM.*alpine\|debian\|ubuntu" .

#!/bin/bash
# Check build configurations and container setup
# Look for Dockerfile and build-related files
fd -t f "Dockerfile|\.dockerfile" -X cat {}

# Check build script for any other linking or dependency related flags
cat scripts/build

# Look for any dependency documentation
fd -t f "README|INSTALL|DEPENDENCIES" -X cat {}

// Add encoding/binary methods to ensure proper network byte order
func (h *TransmissionRequestHeader) MarshalBinary() ([]byte, error) {
    buf := make([]byte, 32) // Size of TransmissionRequestHeader
    binary.BigEndian.PutUint32(buf[0:], h.RequestMagic)
    // ... handle other fields
    return buf, nil
}

#!/bin/bash
# Description: Check for consistent interface implementation across codebase

# Look for Create method implementations
rg "func.*Create.*Backend.*error"

# Check for factory interface definitions
rg "type.*Factory.*interface"

#!/bin/bash
# Check the BackendFactory interface definition
ast-grep --pattern 'interface {
  $$$
}'  pkg/types/types.go

#!/bin/bash
# Description: Verify NBD backend implementations and thread safety

# Check for other NBD backend implementations
echo "Checking for NBD backend implementations..."
rg -l "type.*Backend struct" --type go

# Check for potential thread safety issues
echo "Checking for mutex usage patterns..."
ast-grep --pattern 'type $_ struct {
  $$$
  lock sync.$_
  $$$
}'

# Check for proper mutex unlock patterns
echo "Checking for unsafe mutex unlock patterns..."
rg -l "lock\.(Lock|RLock)[^)].*[^(Unlock|RUnlock)]" --type go

# Check for NBD-related TODOs
echo "Checking for NBD-related TODOs..."
rg -i "todo.*nbd" --type go

#!/bin/bash
# Let's verify the NBD frontend implementation and its thread safety

# Check NBD frontend implementation details
echo "Checking NBD frontend implementation..."
cat pkg/frontend/nbd/frontend.go

# Check for mutex usage in NBD-related files
echo "Checking mutex patterns in NBD files..."
rg -p "sync.Mutex|sync.RWMutex" pkg/frontend/nbd/

# Check for potential race conditions in NBD implementation
echo "Checking critical sections in NBD implementation..."
rg -p "Lock\(\)|RLock\(\)|Unlock\(\)|RUnlock\(\)" pkg/frontend/nbd/

# Check interface implementation
echo "Checking Backend interface definition..."
rg -p "type.*Backend.*interface" vendor/github.com/pojntfx/go-nbd/pkg/backend/

#!/bin/bash
# Let's check the Backend interface definition and thread safety in the NBD package

# Check Backend interface requirements
echo "Checking Backend interface definition..."
cat vendor/github.com/pojntfx/go-nbd/pkg/backend/backend.go

# Check NBD server implementation for concurrency handling
echo "Checking NBD server implementation..."
cat vendor/github.com/pojntfx/go-nbd/pkg/server/server.go

# Check memory and file backend implementations
echo "Checking memory backend implementation..."
cat vendor/github.com/pojntfx/go-nbd/pkg/backend/memory.go

echo "Checking file backend implementation..."
cat vendor/github.com/pojntfx/go-nbd/pkg/backend/file.go

#!/bin/bash
# Check NBD frontend implementation for concurrent connection handling
ast-grep --pattern 'type $name struct {
  $$$
  streams int
  $$$
}'

# Check for proper cleanup in frontend implementation
rg -A 5 'func.*Close\(\).*error'

#!/bin/bash
# Check NBD frontend implementation details
rg -A 10 "type Server struct" pkg/frontend/nbd/
rg -A 10 "func \(s \*Server\) Start\(" pkg/frontend/nbd/
rg -A 10 "func \(s \*Server\) Shutdown\(" pkg/frontend/nbd/

# Check for concurrent connection handling
rg -A 5 "MaximumConnection" pkg/frontend/nbd/
rg -A 5 "concurrent" pkg/frontend/nbd/

# Check for proper cleanup in NBD frontend
ast-grep --pattern 'func $name($args) {
  $$$
  defer $cleanup
  $$$
}'  pkg/frontend/nbd/

#!/bin/bash
# Search for NBD server implementation files
fd nbd.go pkg/frontend/

# Check NBD implementation details with broader context
rg -l "nbd" pkg/frontend/nbd/
rg -A 15 "package nbd" pkg/frontend/nbd/

# Look for connection handling patterns
rg -A 10 "Listen|Accept|connection" pkg/frontend/nbd/

#!/bin/bash
# Check the activity and stability of the kmod repository
gh api repos/pmorjan/kmod/commits?per_page=1
gh api repos/pmorjan/kmod/issues?state=open

# Check libnbd version compatibility
rg -A 5 "libnbd.*version" README.md

#!/bin/bash
# Let's check for libnbd version requirements in the codebase
rg -A 3 "libnbd" .

# Check if there are any specific version requirements or documentation
fd README -t f -X cat {}

# Check for any build or installation instructions that might mention libnbd
fd INSTALL -t f -X cat {}
fd BUILD -t f -X cat {}

# Look for any direct usage of libnbd in the code
ast-grep --pattern 'import "libguestfs.org/libnbd"'