fix: use error instead of strings as error in blockstoreutil/remove

Author: Jorropo

blocks/blockstoreutil/remove.go

@@ -3,6 +3,7 @@ package blockstoreutil
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 
@@ -13,14 +14,14 @@ import (
 )
 
 // RemovedBlock is used to represent the result of removing a block.
-// If a block was removed successfully, then the Error string will be
-// empty.  If a block could not be removed, then Error will contain the
+// If a block was removed successfully, then the Error will be empty.
+// If a block could not be removed, then Error will contain the
 // reason the block could not be removed.  If the removal was aborted
 // due to a fatal error, Hash will be empty, Error will contain the
 // reason, and no more results will be sent.
 type RemovedBlock struct {
-	Hash  string `json:",omitempty"`
-	Error string `json:",omitempty"`
+	Hash  string
+	Error error
 }
 
 // RmBlocksOpts is used to wrap options for RmBlocks().
@@ -51,17 +52,17 @@ func RmBlocks(ctx context.Context, blocks bs.GCBlockstore, pins pin.Pinner, cids
 			// remove this sometime in the future.
 			has, err := blocks.Has(ctx, c)
 			if err != nil {
-				out <- &RemovedBlock{Hash: c.String(), Error: err.Error()}
+				out <- &RemovedBlock{Hash: c.String(), Error: err}
 				continue
 			}
 			if !has && !opts.Force {
-				out <- &RemovedBlock{Hash: c.String(), Error: format.ErrNotFound{Cid: c}.Error()}
+				out <- &RemovedBlock{Hash: c.String(), Error: format.ErrNotFound{Cid: c}}
 				continue
 			}
 
 			err = blocks.DeleteBlock(ctx, c)
 			if err != nil {
-				out <- &RemovedBlock{Hash: c.String(), Error: err.Error()}
+				out <- &RemovedBlock{Hash: c.String(), Error: err}
 			} else if !opts.Quiet {
 				out <- &RemovedBlock{Hash: c.String()}
 			}
@@ -79,7 +80,7 @@ func FilterPinned(ctx context.Context, pins pin.Pinner, out chan<- interface{},
 	stillOkay := make([]cid.Cid, 0, len(cids))
 	res, err := pins.CheckIfPinned(ctx, cids...)
 	if err != nil {
-		out <- &RemovedBlock{Error: fmt.Sprintf("pin check failed: %s", err)}
+		out <- &RemovedBlock{Error: fmt.Errorf("pin check failed: %w", err)}
 		return nil
 	}
 	for _, r := range res {
@@ -88,7 +89,7 @@ func FilterPinned(ctx context.Context, pins pin.Pinner, out chan<- interface{},
 		} else {
 			out <- &RemovedBlock{
 				Hash:  r.Key.String(),
-				Error: r.String(),
+				Error: errors.New(r.String()),
 			}
 		}
 	}
@@ -107,11 +108,11 @@ func ProcRmOutput(next func() (interface{}, error), sout io.Writer, serr io.Writ
 			return err
 		}
 		r := res.(*RemovedBlock)
-		if r.Hash == "" && r.Error != "" {
-			return fmt.Errorf("aborted: %s", r.Error)
-		} else if r.Error != "" {
+		if r.Hash == "" && r.Error != nil {
+			return fmt.Errorf("aborted: %w", r.Error)
+		} else if r.Error != nil {
 			someFailed = true
-			fmt.Fprintf(serr, "cannot remove %s: %s\n", r.Hash, r.Error)
+			fmt.Fprintf(serr, "cannot remove %s: %v\n", r.Hash, r.Error)
 		} else {
 			fmt.Fprintf(sout, "removed %s\n", r.Hash)
 		}

core/commands/block.go

@@ -8,7 +8,6 @@ import (
 
 	files "github.com/ipfs/go-ipfs-files"
 
-	util "github.com/ipfs/go-ipfs/blocks/blockstoreutil"
 	cmdenv "github.com/ipfs/go-ipfs/core/commands/cmdenv"
 	"github.com/ipfs/go-ipfs/core/commands/cmdutils"
 
@@ -213,6 +212,11 @@ const (
 	blockQuietOptionName = "quiet"
 )
 
+type removedBlock struct {
+	Hash  string `json:",omitempty"`
+	Error string `json:",omitempty"`
+}
+
 var blockRmCmd = &cmds.Command{
 	Helptext: cmds.HelpText{
 		Tagline: "Remove IPFS block(s).",
@@ -246,7 +250,7 @@ It takes a list of base58 encoded multihashes to remove.
 
 			err = api.Block().Rm(req.Context, rp, options.Block.Force(force))
 			if err != nil {
-				if err := res.Emit(&util.RemovedBlock{
+				if err := res.Emit(&removedBlock{
 					Hash:  rp.Cid().String(),
 					Error: err.Error(),
 				}); err != nil {
@@ -256,7 +260,7 @@ It takes a list of base58 encoded multihashes to remove.
 			}
 
 			if !quiet {
-				err := res.Emit(&util.RemovedBlock{
+				err := res.Emit(&removedBlock{
 					Hash: rp.Cid().String(),
 				})
 				if err != nil {
@@ -269,8 +273,29 @@ It takes a list of base58 encoded multihashes to remove.
 	},
 	PostRun: cmds.PostRunMap{
 		cmds.CLI: func(res cmds.Response, re cmds.ResponseEmitter) error {
-			return util.ProcRmOutput(res.Next, os.Stdout, os.Stderr)
+			someFailed := false
+			for {
+				res, err := res.Next()
+				if err == io.EOF {
+					break
+				} else if err != nil {
+					return err
+				}
+				r := res.(*removedBlock)
+				if r.Hash == "" && r.Error != "" {
+					return fmt.Errorf("aborted: %s", r.Error)
+				} else if r.Error != "" {
+					someFailed = true
+					fmt.Fprintf(os.Stderr, "cannot remove %s: %s\n", r.Hash, r.Error)
+				} else {
+					fmt.Fprintf(os.Stdout, "removed %s\n", r.Hash)
+				}
+			}
+			if someFailed {
+				return fmt.Errorf("some blocks not removed")
+			}
+			return nil
 		},
 	},
-	Type: util.RemovedBlock{},
+	Type: removedBlock{},
 }

core/coreapi/block.go

@@ -107,10 +107,8 @@ func (api *BlockAPI) Rm(ctx context.Context, p path.Path, opts ...caopts.BlockRm
 			return errors.New("got unexpected output from util.RmBlocks")
 		}
 
-		// Because errors come as strings we lose information about
-		// the error type.
-		if remBlock.Error != "" {
-			return errors.New(remBlock.Error)
+		if remBlock.Error != nil {
+			return remBlock.Error
 		}
 		return nil
 	case <-ctx.Done():