This is an automated email from the ASF dual-hosted git repository. sjanc pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mynewt-newtmgr.git
commit 7770b41f3f077850e01ab5ff5cd2ff5d28ed9b33 Author: Szymon Janc <[email protected]> AuthorDate: Thu Oct 22 00:59:43 2026 +0200 nmxact/xact: Stop image upload on device error ImageUploadCmd ignored the rc of upload responses. When the device rejected a chunk (e.g. flash write error, image too large), the response carries no useful offset, so MaxRxOff never reached the image size and Run() kept looping forever. "newtmgr image upload" hung instead of reporting the error. Stop sending further chunks once a response with a non-zero rc arrives and return the result, like the other multi-request commands (fs upload/download, core load) do. Responses to requests still in flight are ignored after the error so that the error response stays the last one in the result and Status() reports the device's rc. The CLI then prints "Error: <rc>" as it does for other failed commands. Assisted-by: Claude:claude-opus-5-5 --- nmxact/xact/image.go | 42 +++++++++++++++++------ nmxact/xact/image_upload_test.go | 73 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/nmxact/xact/image.go b/nmxact/xact/image.go index 7de5745..7548074 100644 --- a/nmxact/xact/image.go +++ b/nmxact/xact/image.go @@ -65,6 +65,9 @@ type ImageUploadIntTracker struct { WCap int Off int MaxRxOff int32 + + // Set when the device answered with a non-zero rc. + rspFailed bool } type ImageUploadResult struct { @@ -267,14 +270,23 @@ func (t *ImageUploadIntTracker) HandleResponse(c *ImageUploadCmd, rsp nmp.NmpRsp defer t.Mutex.Unlock() wFull := false - if rsp != nil { + // Once the device reported an error, responses to requests which + // were still in flight are ignored. This keeps the error response + // last in the result, so that Status() reports it. + if rsp != nil && !t.rspFailed { irsp := rsp.(*nmp.ImageUploadRsp) res.Rsps = append(res.Rsps, irsp) - t.UpdateTracker(int(irsp.Off), IMAGE_UPLOAD_STATUS_RQ) - // MaxRxOff is also read without the mutex by Run(). - if t.MaxRxOff < int32(irsp.Off) { - atomic.StoreInt32(&t.MaxRxOff, int32(irsp.Off)) + if irsp.Rc != 0 { + // The device rejected the request; stop the upload. + t.rspFailed = true + } else { + t.UpdateTracker(int(irsp.Off), IMAGE_UPLOAD_STATUS_RQ) + + // MaxRxOff is also read without the mutex by Run(). + if t.MaxRxOff < int32(irsp.Off) { + atomic.StoreInt32(&t.MaxRxOff, int32(irsp.Off)) + } } if c.ProgressCb != nil { c.ProgressCb(c, irsp) @@ -351,6 +363,14 @@ func (c *ImageUploadCmd) Run(s sesn.Sesn) (Result, error) { <-ch } + // Stop if the device reported an error. + t.Mutex.Lock() + failed := t.rspFailed + t.Mutex.Unlock() + if failed { + break + } + t.ProcessMissedChunks() if t.Off == len(c.Data) { @@ -394,19 +414,21 @@ func (c *ImageUploadCmd) Run(s sesn.Sesn) (Result, error) { }(int(r.Off)) } - maxRxOff := int(atomic.LoadInt32(&t.MaxRxOff)) - if maxRxOff == len(c.Data) { + t.Mutex.Lock() + defer t.Mutex.Unlock() + + // On a non-zero rc from the device, the result is returned as well; + // its Status() reports the device's rc. + if t.rspFailed || int(t.MaxRxOff) == len(c.Data) { // Responses to requests still in flight may be processed after // we return; hand out a snapshot of the result so that the // caller does not race with them. - t.Mutex.Lock() - defer t.Mutex.Unlock() snap := newImageUploadResult() snap.Rsps = append(snap.Rsps, res.Rsps...) return snap, nil } else { return nil, fmt.Errorf("ImageUpload unexpected error after %d/%d bytes", - maxRxOff, len(c.Data)) + t.MaxRxOff, len(c.Data)) } } diff --git a/nmxact/xact/image_upload_test.go b/nmxact/xact/image_upload_test.go index f3c928c..39e76fc 100644 --- a/nmxact/xact/image_upload_test.go +++ b/nmxact/xact/image_upload_test.go @@ -40,8 +40,10 @@ type fakeImgDev struct { upgrade bool img int - // If non-zero, all upload requests are answered with this rc. - failRc int + // If non-zero, upload requests with an offset of at least failOff + // are answered with this rc. + failRc int + failOff uint32 // Offsets of requests answered with a timeout error, once each. dropOffs map[uint32]bool @@ -61,7 +63,7 @@ func (d *fakeImgDev) handle(req fakeReq) (nmp.NmpRsp, error) { return nil, nmxutil.NewRspTimeoutError("NMP timeout") } - if d.failRc != 0 { + if d.failRc != 0 && ur.Off >= d.failOff { return &nmp.ImageUploadRsp{Rc: d.failRc}, nil } @@ -531,3 +533,68 @@ func TestImageUpgradeRun(t *testing.T) { mtx.Unlock() } } + +// A non-zero rc from the device must end the upload and be reported via +// the result's status instead of making Run() loop forever. +func TestImageUploadRunDeviceError(t *testing.T) { + data := testImage(5000) + + for _, winSz := range []int{1, 5} { + for _, failOff := range []uint32{0, 1} { + dev := &fakeImgDev{ + failRc: nmp.NMP_ERR_ENOMEM, + failOff: failOff, + } + s := newFakeSesn(dev.handle) + s.mtu = 200 + + var mtx sync.Mutex + var cbRcs []int + c := NewImageUploadCmd() + c.Data = data + c.MaxWinSz = winSz + c.ProgressCb = func(_ *ImageUploadCmd, r *nmp.ImageUploadRsp) { + mtx.Lock() + defer mtx.Unlock() + cbRcs = append(cbRcs, r.Rc) + } + + res, err := runCmd(t, c, s) + if err != nil { + t.Fatalf("win %d fail at %d: %v", winSz, failOff, err) + } + if st := res.Status(); st != nmp.NMP_ERR_ENOMEM { + t.Errorf("win %d fail at %d: status %d", winSz, failOff, + st) + } + + mtx.Lock() + if len(cbRcs) == 0 || cbRcs[len(cbRcs)-1] != nmp.NMP_ERR_ENOMEM { + t.Errorf("win %d fail at %d: progress rcs %v", winSz, + failOff, cbRcs) + } + mtx.Unlock() + } + } +} + +func TestImageUpgradeRunDeviceError(t *testing.T) { + dev := &imgUpgradeDev{} + dev.failRc = nmp.NMP_ERR_ENOMEM + dev.failOff = 1 + s := newFakeSesn(dev.handle) + dev.s = s + + c := NewImageUpgradeCmd() + c.Data = testImage(3000) + c.MaxWinSz = 1 + c.ProgressCb = func(*ImageUploadCmd, *nmp.ImageUploadRsp) {} + + res, err := runCmd(t, c, s) + if err != nil { + t.Fatal(err) + } + if st := res.Status(); st != nmp.NMP_ERR_ENOMEM { + t.Errorf("status %d", st) + } +}
