From f1cec4f5d439589ab5588fe3a8dcdc6c806774e3 Mon Sep 17 00:00:00 2001 From: Lei Xue Date: Sat, 14 Mar 2026 18:27:56 +0800 Subject: [PATCH 1/3] fix regex, remove unsafe pointer usage, and add graceful HTTP shutdown - Fix versionMatcher regex: gorilla/mux does not support (?:) syntax, and -dirty suffix was required instead of optional - Replace unsafe.Pointer LUN casts with binary.LittleEndian.Uint64 in sbc.go, scsi.go, and target.go - Implement graceful HTTP server shutdown with 5s timeout using srv.Shutdown() instead of raw listener close - Replace golang.org/x/net/context with standard library context - Respect existing req.Cancel value in canceler to avoid overwriting - Add early context cancellation check in Do() to fail fast Based on review of PR #120 by @orzhang, with fixes applied. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/api/client/transport/cancellable/canceler.go | 5 ++++- pkg/api/client/transport/cancellable/cancellable.go | 7 +++++++ pkg/apiserver/apiserver.go | 9 ++++++--- pkg/scsi/sbc.go | 3 +-- pkg/scsi/scsi.go | 3 +-- pkg/scsi/target.go | 4 ++-- 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/pkg/api/client/transport/cancellable/canceler.go b/pkg/api/client/transport/cancellable/canceler.go index a7e499a..dee9a91 100644 --- a/pkg/api/client/transport/cancellable/canceler.go +++ b/pkg/api/client/transport/cancellable/canceler.go @@ -14,7 +14,10 @@ import ( ) func canceler(client transport.Sender, req *http.Request) func() { - // TODO(djd): Respect any existing value of req.Cancel. + // Respect any existing value of req.Cancel. + if req.Cancel != nil { + return nop + } ch := make(chan struct{}) req.Cancel = ch diff --git a/pkg/api/client/transport/cancellable/cancellable.go b/pkg/api/client/transport/cancellable/cancellable.go index 60f93bc..26e4f83 100644 --- a/pkg/api/client/transport/cancellable/cancellable.go +++ b/pkg/api/client/transport/cancellable/cancellable.go @@ -35,6 +35,13 @@ func Do(ctx context.Context, client transport.Sender, req *http.Request) (*http. client = http.DefaultClient } + // If the context is already done, return early. + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + // Request cancelation changed in Go 1.5, see canceler.go and canceler_go14.go. cancel := canceler(client, req) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 6803fd6..db20a17 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -19,11 +19,13 @@ package apiserver import ( "crypto/tls" + "context" "fmt" "net" "net/http" "strconv" "strings" + "time" systemdActivation "github.com/coreos/go-systemd/activation" "github.com/docker/go-connections/sockets" @@ -34,12 +36,11 @@ import ( "github.com/gostor/gotgt/pkg/apiserver/router/lu" "github.com/gostor/gotgt/pkg/apiserver/router/target" log "github.com/sirupsen/logrus" - "golang.org/x/net/context" ) // versionMatcher defines a variable matcher to be parsed by the router // when a request is about to be served. -const versionMatcher = "/v{version:[0-9.]+(?:-dirty)}" +const versionMatcher = "/v{version:[0-9.]+(-dirty)?}" // Config provides the configuration for the API server type Config struct { @@ -137,7 +138,9 @@ func (s *HTTPServer) Serve() error { // Close closes the HTTPServer from listening for the inbound requests. func (s *HTTPServer) Close() error { - return s.l.Close() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return s.srv.Shutdown(ctx) } func (s *Server) initTCPSocket(addr string) (l net.Listener, err error) { diff --git a/pkg/scsi/sbc.go b/pkg/scsi/sbc.go index 4ce559a..d7c32a3 100644 --- a/pkg/scsi/sbc.go +++ b/pkg/scsi/sbc.go @@ -21,7 +21,6 @@ import ( "bytes" "encoding/binary" "fmt" - "unsafe" "github.com/gostor/gotgt/pkg/api" "github.com/gostor/gotgt/pkg/util" @@ -498,7 +497,7 @@ func SBCReserve(host int, cmd *api.SCSICommand) api.SAMStat { } func SBCRelease(host int, cmd *api.SCSICommand) api.SAMStat { - lun := *(*uint64)(unsafe.Pointer(&cmd.Lun)) + lun := binary.LittleEndian.Uint64(cmd.Lun[:]) if err := deviceRelease(cmd.Target.TID, cmd.ITNexusID, lun, false); err != nil { return api.SAMStatReservationConflict } diff --git a/pkg/scsi/scsi.go b/pkg/scsi/scsi.go index c22ff8c..d36515f 100644 --- a/pkg/scsi/scsi.go +++ b/pkg/scsi/scsi.go @@ -21,7 +21,6 @@ import ( "encoding/binary" "fmt" "sync" - "unsafe" "github.com/gostor/gotgt/pkg/api" log "github.com/sirupsen/logrus" @@ -96,7 +95,7 @@ func (s *SCSITargetService) AddCommandQueue(tid int, scmd *api.SCSICommand) erro } } scmd.ITNexus = itn - lun := *(*uint64)(unsafe.Pointer(&scmd.Lun)) + lun := binary.LittleEndian.Uint64(scmd.Lun[:]) scmd.Device = target.Devices[lun] log.Debugf("scsi opcode: 0x%x, LUN: %d", int(scmd.SCB[0]), binary.LittleEndian.Uint64(scmd.Lun[:])) diff --git a/pkg/scsi/target.go b/pkg/scsi/target.go index dc19e5f..e96419d 100644 --- a/pkg/scsi/target.go +++ b/pkg/scsi/target.go @@ -17,8 +17,8 @@ limitations under the License. package scsi import ( + "encoding/binary" "fmt" - "unsafe" "github.com/google/uuid" "github.com/gostor/gotgt/pkg/api" @@ -97,7 +97,7 @@ func RemoveITNexus(target *api.SCSITarget, itnexus *api.ITNexus) { func deviceReserve(cmd *api.SCSICommand) error { var lu *api.SCSILu - lun := *(*uint64)(unsafe.Pointer(&cmd.Lun)) + lun := binary.LittleEndian.Uint64(cmd.Lun[:]) for tgtLUN, lunDev := range cmd.Target.Devices { if tgtLUN == lun { From 549f0cb46019578e3b0c8d75dc8088f8e14c1118 Mon Sep 17 00:00:00 2001 From: Lei Xue Date: Sat, 14 Mar 2026 18:30:49 +0800 Subject: [PATCH 2/3] fix gofmt import ordering Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/apiserver/apiserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index db20a17..eb40477 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -18,8 +18,8 @@ limitations under the License. package apiserver import ( - "crypto/tls" "context" + "crypto/tls" "fmt" "net" "net/http" From 633b84009c4d02459e2f68da56a83e06ecfbc408 Mon Sep 17 00:00:00 2001 From: Lei Xue Date: Sat, 14 Mar 2026 18:51:01 +0800 Subject: [PATCH 3/3] fix versionMatcher: use non-capturing group (?:-dirty)? for gorilla/mux gorilla/mux explicitly rejects capturing groups () in route regexps, only non-capturing groups (?:) are allowed. The original regex was missing the ? to make -dirty optional. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/apiserver/apiserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index eb40477..9a94cfd 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -40,7 +40,7 @@ import ( // versionMatcher defines a variable matcher to be parsed by the router // when a request is about to be served. -const versionMatcher = "/v{version:[0-9.]+(-dirty)?}" +const versionMatcher = "/v{version:[0-9.]+(?:-dirty)?}" // Config provides the configuration for the API server type Config struct {