decomposedfs: refactor xattrs package errors (#2540)

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

changelog/unreleased/decomposedfs-xattr-errors.md

@@ -0,0 +1,5 @@
+Enhancement: Refactored the xattrs package in the decomposedfs
+
+The xattrs package now uses the xattr.ENOATTR instead of os.ENODATA or os.ENOATTR to check attribute existence.
+
+https://github.com/cs3org/reva/pull/2540

pkg/storage/utils/decomposedfs/node/node.go

@@ -181,9 +181,9 @@ func ReadNode(ctx context.Context, lu PathLookup, id string) (n *Node, err error
 	switch {
 	case err == nil:
 		n.ParentID = attr
-	case isAttrUnset(err):
+	case xattrs.IsAttrUnset(err):
 		return nil, errtypes.InternalError(err.Error())
-	case isNotFound(err):
+	case xattrs.IsNotExist(err):
 		return n, nil // swallow not found, the node defaults to exists = false
 	default:
 		return nil, errtypes.InternalError(err.Error())
@@ -216,7 +216,7 @@ func ReadNode(ctx context.Context, lu PathLookup, id string) (n *Node, err error
 	// Check if parent exists. Otherwise this node is part of a deleted subtree
 	_, err = os.Stat(lu.InternalPath(n.ParentID))
 	if err != nil {
-		if isNotFound(err) {
+		if os.IsNotExist(err) {
 			return nil, errtypes.NotFound(err.Error())
 		}
 		return nil, err
@@ -320,7 +320,7 @@ func (n *Node) Owner() (*userpb.UserId, error) {
 	switch {
 	case err == nil:
 		owner.OpaqueId = attr
-	case isAttrUnset(err), isNotFound(err):
+	case xattrs.IsAttrUnset(err), xattrs.IsNotExist(err):
 		fallthrough
 	default:
 		return nil, err
@@ -331,7 +331,7 @@ func (n *Node) Owner() (*userpb.UserId, error) {
 	switch {
 	case err == nil:
 		owner.Idp = attr
-	case isAttrUnset(err), isNotFound(err):
+	case xattrs.IsAttrUnset(err), xattrs.IsNotExist(err):
 		fallthrough
 	default:
 		return nil, err
@@ -342,7 +342,7 @@ func (n *Node) Owner() (*userpb.UserId, error) {
 	switch {
 	case err == nil:
 		owner.Type = utils.UserTypeMap(attr)
-	case isAttrUnset(err), isNotFound(err):
+	case xattrs.IsAttrUnset(err), xattrs.IsNotExist(err):
 		fallthrough
 	default:
 		// TODO the user type defaults to invalid, which is the case
@@ -681,9 +681,9 @@ func readChecksumIntoResourceChecksum(ctx context.Context, nodePath, algo string
 			Type: storageprovider.PKG2GRPCXS(algo),
 			Sum:  hex.EncodeToString([]byte(v)),
 		}
-	case isAttrUnset(err):
+	case xattrs.IsAttrUnset(err):
 		appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set")
-	case isNotFound(err):
+	case xattrs.IsNotExist(err):
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount")
 	default:
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum")
@@ -703,9 +703,9 @@ func readChecksumIntoOpaque(ctx context.Context, nodePath, algo string, ri *prov
 			Decoder: "plain",
 			Value:   []byte(hex.EncodeToString([]byte(v))),
 		}
-	case isAttrUnset(err):
+	case xattrs.IsAttrUnset(err):
 		appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set")
-	case isNotFound(err):
+	case xattrs.IsNotExist(err):
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount")
 	default:
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum")
@@ -735,9 +735,9 @@ func readQuotaIntoOpaque(ctx context.Context, nodePath string, ri *provider.Reso
 		} else {
 			appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("quota", v).Msg("malformed quota")
 		}
-	case isAttrUnset(err):
+	case xattrs.IsAttrUnset(err):
 		appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Msg("quota not set")
-	case isNotFound(err):
+	case xattrs.IsNotExist(err):
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Msg("file not found when reading quota")
 	default:
 		appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Msg("could not read quota")
@@ -862,7 +862,7 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap prov
 		switch {
 		case err == nil:
 			AddPermissions(&ap, g.GetPermissions())
-		case isAttrUnset(err):
+		case xattrs.IsAttrUnset(err):
 			err = nil
 			appctx.GetLogger(ctx).Error().Interface("node", n).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing")
 			// continue with next segment

pkg/storage/utils/decomposedfs/node/permissions.go

@@ -21,7 +21,6 @@ package node
 import (
 	"context"
 	"strings"
-	"syscall"
 
 	userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
@@ -30,7 +29,6 @@ import (
 	"github.com/cs3org/reva/pkg/storage/utils/decomposedfs/xattrs"
 	"github.com/cs3org/reva/pkg/utils"
 	"github.com/pkg/errors"
-	"github.com/pkg/xattr"
 )
 
 // NoPermissions represents an empty set of permissions
@@ -252,7 +250,7 @@ func nodeHasPermission(ctx context.Context, cn *Node, groupsMap map[string]bool,
 			if check(g.GetPermissions()) {
 				return true
 			}
-		case isAttrUnset(err):
+		case xattrs.IsAttrUnset(err):
 			appctx.GetLogger(ctx).Error().Interface("node", cn.ID).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing")
 		default:
 			appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn.ID).Str("grant", grantees[i]).Msg("error reading permissions")
@@ -290,14 +288,3 @@ func (p *Permissions) getUserAndPermissions(ctx context.Context, n *Node) (*user
 	}
 	return u, nil
 }
-
-// The os not exists error is buried inside the xattr error,
-// so we cannot just use os.IsNotExists().
-func isNotFound(err error) bool {
-	if xerr, ok := err.(*xattr.Error); ok {
-		if serr, ok2 := xerr.Err.(syscall.Errno); ok2 {
-			return serr == syscall.ENOENT
-		}
-	}
-	return false
-}

pkg/storage/utils/decomposedfs/node/permissions_unix.go → pkg/storage/utils/decomposedfs/xattrs/errors.go

@@ -19,18 +19,31 @@
 //go:build !darwin
 // +build !darwin
 
-package node
+package xattrs
 
 import (
 	"syscall"
 
 	"github.com/pkg/xattr"
 )
 
-func isAttrUnset(err error) bool {
+// IsNotExist checks if there is a os not exists error buried inside the xattr error,
+// as we cannot just use os.IsNotExist().
+func IsNotExist(err error) bool {
 	if xerr, ok := err.(*xattr.Error); ok {
 		if serr, ok2 := xerr.Err.(syscall.Errno); ok2 {
-			return serr == syscall.ENODATA
+			return serr == syscall.ENOENT
+		}
+	}
+	return false
+}
+
+// IsAttrUnset checks the xattr.ENOATTR from the xattr package which redifines it as ENODATA on platforms that do not natively support it (eg. linux)
+// see https://github.com/pkg/xattr/blob/8725d4ccc0fcef59c8d9f0eaf606b3c6f962467a/xattr_linux.go#L19-L22
+func IsAttrUnset(err error) bool {
+	if xerr, ok := err.(*xattr.Error); ok {
+		if serr, ok2 := xerr.Err.(syscall.Errno); ok2 {
+			return serr == xattr.ENOATTR
 		}
 	}
 	return false

pkg/storage/utils/decomposedfs/xattrs/xattrs.go

@@ -19,6 +19,7 @@
 package xattrs
 
 import (
+	"strconv"
 	"strings"
 
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
@@ -165,6 +166,19 @@ func Get(filePath, key string) (string, error) {
 	return val, nil
 }
 
+// GetInt64 reads a string as int64 from the xattrs
+func GetInt64(filePath, key string) (int64, error) {
+	attr, err := Get(filePath, key)
+	if err != nil {
+		return 0, err
+	}
+	v, err := strconv.ParseInt(attr, 10, 64)
+	if err != nil {
+		return 0, errors.Wrapf(err, "invalid xattr format")
+	}
+	return v, nil
+}
+
 // All reads all extended attributes for a node
 func All(filePath string) (map[string]string, error) {
 	attrNames, err := xattr.List(filePath)