status-im
/
consul
mirror of https://github.com/status-im/consul.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

consul/state: refactor tnxService to avoid missed cases 

Handling errors at the end of a log switch/case block is somewhat
brittle. This block included a couple cases where errors were ignored,
but it was not obvious the way it was written.

This change moves all error handling into each case block. There is
still potentially one case where err is ignored, which will be handled
in a follow up.
This commit is contained in:
Daniel Nephin 2020-05-19 16:48:17 -04:00
parent 9f27d61bee
commit 1bbea2751f
1 changed files with 32 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
agent/consul/state/txn.go
View File

@ -210,64 +210,62 @@ func (s *Store) txnNode(tx *memdb.Txn, idx uint64, op *structs.TxnNodeOp) (struc
// txnService handles all Service-related operations. // txnService handles all Service-related operations.
func (s *Store) txnService(tx *memdb.Txn, idx uint64, op *structs.TxnServiceOp) (structs.TxnResults, error) { func (s *Store) txnService(tx *memdb.Txn, idx uint64, op *structs.TxnServiceOp) (structs.TxnResults, error) {
var entry *structs.NodeService
var err error
switch op.Verb { switch op.Verb {
case api.ServiceGet: case api.ServiceGet:
entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
if entry == nil && err == nil { switch {
case err != nil:
return nil, err
case entry == nil:
return nil, fmt.Errorf("service %q on node %q doesn't exist", op.Service.ID, op.Node) return nil, fmt.Errorf("service %q on node %q doesn't exist", op.Service.ID, op.Node)
default:
return structs.TxnResults{&structs.TxnResult{Service: entry}}, nil
} }
case api.ServiceSet: case api.ServiceSet:
err = s.ensureServiceTxn(tx, idx, op.Node, &op.Service) if err := s.ensureServiceTxn(tx, idx, op.Node, &op.Service); err != nil {
if err != nil {
return nil, err return nil, err
} }
entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
return newTxnResultFromNodeServiceEntry(entry), err
case api.ServiceCAS: case api.ServiceCAS:
var ok bool ok, err := s.ensureServiceCASTxn(tx, idx, op.Node, &op.Service)
ok, err = s.ensureServiceCASTxn(tx, idx, op.Node, &op.Service) // TODO: err != nil case is ignored
if !ok && err == nil { if !ok && err == nil {
err = fmt.Errorf("failed to set service %q on node %q, index is stale", op.Service.ID, op.Node) err := fmt.Errorf("failed to set service %q on node %q, index is stale", op.Service.ID, op.Node)
break return nil, err
} }
entry, err = s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
entry, err := s.getNodeServiceTxn(tx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
return newTxnResultFromNodeServiceEntry(entry), err
case api.ServiceDelete: case api.ServiceDelete:
err = s.deleteServiceTxn(tx, idx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta) err := s.deleteServiceTxn(tx, idx, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
return nil, err
case api.ServiceDeleteCAS: case api.ServiceDeleteCAS:
var ok bool ok, err := s.deleteServiceCASTxn(tx, idx, op.Service.ModifyIndex, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
ok, err = s.deleteServiceCASTxn(tx, idx, op.Service.ModifyIndex, op.Node, op.Service.ID, &op.Service.EnterpriseMeta)
if !ok && err == nil { if !ok && err == nil {
return nil, fmt.Errorf("failed to delete service %q on node %q, index is stale", op.Service.ID, op.Node) return nil, fmt.Errorf("failed to delete service %q on node %q, index is stale", op.Service.ID, op.Node)
} }
return nil, err
default: default:
return nil, fmt.Errorf("unknown Service verb %q", op.Verb) return nil, fmt.Errorf("unknown Service verb %q", op.Verb)
} }
if err != nil { }
return nil, err
// newTxnResultFromNodeServiceEntry returns a TxnResults with a single result,
// a copy of entry. The entry is copied to prevent modification of the state
// store.
func newTxnResultFromNodeServiceEntry(entry *structs.NodeService) structs.TxnResults {
if entry == nil {
return nil
} }
clone := *entry
// For a GET we keep the value, otherwise we clone and blank out the result := structs.TxnResult{Service: &clone}
// value (we have to clone so we don't modify the entry being used by return structs.TxnResults{&result}
// the state store).
if entry != nil {
if op.Verb == api.ServiceGet {
result := structs.TxnResult{Service: entry}
return structs.TxnResults{&result}, nil
}
clone := *entry
result := structs.TxnResult{Service: &clone}
return structs.TxnResults{&result}, nil
}
return nil, nil
} }
// txnCheck handles all Check-related operations. // txnCheck handles all Check-related operations.