Let symlinks be backed by path.Parser

.github/workflows/main.yaml

@@ -953,7 +953,7 @@
             {
                "if": "matrix.host.platform_name == 'windows_amd64'",
                "name": "Execute WinFSP Integration Tests",
-               "run": "bazel test //pkg/filesystem/virtual/winfsp:file_system_integration_test"
+               "run": "bazel test --test_output=errors //pkg/filesystem/virtual/winfsp:file_system_integration_test"
             },
             {
                "env": {

.github/workflows/pull-requests.yaml

@@ -71,7 +71,7 @@
             {
                "if": "matrix.host.platform_name == 'windows_amd64'",
                "name": "Execute WinFSP Integration Tests",
-               "run": "bazel test //pkg/filesystem/virtual/winfsp:file_system_integration_test"
+               "run": "bazel test --test_output=errors //pkg/filesystem/virtual/winfsp:file_system_integration_test"
             }
          ],
          "strategy": {

cmd/bb_virtual_tmp/main.go

@@ -51,7 +51,7 @@ func main() {
 		if err := path.Resolve(path.UNIXFormat.NewParser(configuration.BuildDirectoryPath), scopeWalker); err != nil {
 			return util.StatusWrap(err, "Failed to resolve build directory path")
 		}
-		userSettableSymlink := virtual.NewUserSettableSymlink(buildDirectory)
+		userSettableSymlink := virtual.NewUserSettableSymlink(buildDirectory, path.UNIXFormat.NewParser("/invalid"))
 
 		// Expose the symbolic link through a virtual file system.
 		mount, handleAllocator, err := virtual_configuration.NewMountFromConfiguration(

cmd/bb_worker/main.go

@@ -225,7 +225,9 @@ func main() {
 
 				symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory(
 					virtual.BaseSymlinkFactory,
-					handleAllocator.New())
+					handleAllocator.New(),
+					path.LocalFormat,
+				)
 				characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory(
 					virtual.BaseCharacterDeviceFactory,
 					handleAllocator.New())

pkg/builder/virtual_build_directory.go

@@ -194,12 +194,17 @@ func (d *virtualBuildDirectory) Readlink(name path.Component) (path.Parser, erro
 	if err != nil {
 		return nil, err
 	}
-	if _, leaf := child.GetPair(); leaf != nil {
-		p := virtual.ApplyReadlink{}
-		if !child.GetNode().VirtualApply(&p) {
-			panic("build directory contains leaves that don't handle ApplyReadlink")
-		}
-		return p.Target, p.Err
+
+	_, leaf := child.GetPair()
+	if leaf == nil {
+		return nil, syscall.EISDIR
+	}
+
+	var attributes virtual.Attributes
+	leaf.VirtualGetAttributes(context.TODO(), virtual.AttributesMaskSymlinkTarget, &attributes)
+	symlinkTarget, ok := attributes.GetSymlinkTarget()
+	if !ok {
+		return nil, syscall.EINVAL
 	}
-	return nil, syscall.EISDIR
+	return symlinkTarget, nil
 }

pkg/filesystem/virtual/attributes.go

@@ -4,6 +4,7 @@ import (
 	"time"
 
 	"github.com/buildbarn/bb-storage/pkg/filesystem"
+	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
 )
 
 // AttributesMask is a bitmask of status attributes that need to be
@@ -51,7 +52,12 @@ const (
 	// bits of set_mode).
 	AttributesMaskPermissions
 	// AttributesMaskSizeBytes requests the file size (st_size).
+	// When requested, this field should be set for all types of
+	// files, except for symbolic links.
 	AttributesMaskSizeBytes
+	// AttributesMaskSymlinkTarget requests the target of a symbolic
+	// link.
+	AttributesMaskSymlinkTarget
 )
 
 // Attributes of a file, normally requested through stat() or readdir().
@@ -72,6 +78,7 @@ type Attributes struct {
 	ownerUserID                     uint32
 	permissions                     Permissions
 	sizeBytes                       uint64
+	symlinkTarget                   path.Parser
 }
 
 // GetChangeID returns the change ID, which clients can use to determine
@@ -264,3 +271,15 @@ func (a *Attributes) SetSizeBytes(sizeBytes uint64) *Attributes {
 	a.fieldsPresent |= AttributesMaskSizeBytes
 	return a
 }
+
+// GetSymlinkTarget returns the target of a symbolic link.
+func (a *Attributes) GetSymlinkTarget() (path.Parser, bool) {
+	return a.symlinkTarget, a.fieldsPresent&AttributesMaskSymlinkTarget != 0
+}
+
+// SetSymlinkTarget sets the target of a symbolic link.
+func (a *Attributes) SetSymlinkTarget(symlinkTarget path.Parser) *Attributes {
+	a.symlinkTarget = symlinkTarget
+	a.fieldsPresent |= AttributesMaskSymlinkTarget
+	return a
+}

pkg/filesystem/virtual/base_symlink_factory.go

@@ -2,21 +2,17 @@ package virtual
 
 import (
 	"context"
-	"unicode/utf8"
 
 	remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
 	"github.com/buildbarn/bb-storage/pkg/filesystem"
 	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
-
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 type symlinkFactory struct{}
 
-func (symlinkFactory) LookupSymlink(target []byte) LinkableLeaf {
-	return symlink{target: target}
+func (symlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) {
+	return symlink{target: target}, nil
 }
 
 // BaseSymlinkFactory can be used to create simple immutable symlink nodes.
@@ -25,23 +21,12 @@ var BaseSymlinkFactory SymlinkFactory = symlinkFactory{}
 type symlink struct {
 	placeholderFile
 
-	target []byte
-}
-
-func (f symlink) readlinkParser() (path.Parser, error) {
-	if !utf8.Valid(f.target) {
-		return nil, status.Error(codes.InvalidArgument, "Symbolic link contents are not valid UTF-8")
-	}
-	return path.UNIXFormat.NewParser(string(f.target)), nil
+	target path.Parser
 }
 
 func (f symlink) readlinkString() (string, error) {
-	targetParser, err := f.readlinkParser()
-	if err != nil {
-		return "", err
-	}
 	targetPath, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker)
-	if err := path.Resolve(targetParser, scopeWalker); err != nil {
+	if err := path.Resolve(f.target, scopeWalker); err != nil {
 		return "", err
 	}
 	return targetPath.GetUNIXString(), nil
@@ -52,11 +37,7 @@ func (f symlink) VirtualGetAttributes(ctx context.Context, requested AttributesM
 	attributes.SetFileType(filesystem.FileTypeSymlink)
 	attributes.SetHasNamedAttributes(false)
 	attributes.SetPermissions(PermissionsRead | PermissionsWrite | PermissionsExecute)
-	attributes.SetSizeBytes(uint64(len(f.target)))
-}
-
-func (f symlink) VirtualReadlink(ctx context.Context) ([]byte, Status) {
-	return f.target, StatusOK
+	attributes.SetSymlinkTarget(f.target)
 }
 
 func (f symlink) VirtualSetAttributes(ctx context.Context, in *Attributes, requested AttributesMask, out *Attributes) Status {
@@ -69,8 +50,6 @@ func (f symlink) VirtualSetAttributes(ctx context.Context, in *Attributes, reque
 
 func (f symlink) VirtualApply(data any) bool {
 	switch p := data.(type) {
-	case *ApplyReadlink:
-		p.Target, p.Err = f.readlinkParser()
 	case *ApplyGetBazelOutputServiceStat:
 		if target, err := f.readlinkString(); err == nil {
 			p.Stat = &bazeloutputservice.BatchStatResponse_Stat{

pkg/filesystem/virtual/blob_access_cas_file_factory.go

@@ -2,7 +2,6 @@ package virtual
 
 import (
 	"context"
-	"syscall"
 
 	remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice"
@@ -73,8 +72,6 @@ func (blobAccessCASFile) VirtualAllocate(off, size uint64) Status {
 
 func (f *blobAccessCASFile) virtualApplyCommon(data any) bool {
 	switch p := data.(type) {
-	case *ApplyReadlink:
-		p.Err = syscall.EINVAL
 	case *ApplyUploadFile:
 		// This file is already backed by the Content Addressable
 		// Storage. There is thus no need to upload it once again.
@@ -147,10 +144,6 @@ func (f *blobAccessCASFile) VirtualRead(buf []byte, off uint64) (int, bool, Stat
 	return len(buf), eof, StatusOK
 }
 
-func (blobAccessCASFile) VirtualReadlink(ctx context.Context) ([]byte, Status) {
-	return nil, StatusErrInval
-}
-
 func (blobAccessCASFile) VirtualClose(shareAccess ShareMask) {}
 
 func (blobAccessCASFile) virtualSetAttributesCommon(in *Attributes) Status {

pkg/filesystem/virtual/cas_initial_contents_fetcher.go

@@ -110,7 +110,10 @@ func (icf *casInitialContentsFetcher) fetchContentsUnwrapped(fileReadMonitorFact
 			return nil, status.Errorf(codes.InvalidArgument, "Directory contains multiple children named %#v", entry.Name)
 		}
 
-		leaf := icf.options.symlinkFactory.LookupSymlink([]byte(entry.Target))
+		leaf, err := icf.options.symlinkFactory.LookupSymlink(path.UNIXFormat.NewParser(entry.Target))
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "Failed to look up symlink named %#v", entry.Name)
+		}
 		children[component] = InitialChild{}.FromLeaf(leaf)
 		leavesToUnlink = append(leavesToUnlink, leaf)
 	}

pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go

@@ -212,7 +212,13 @@ func TestCASInitialContentsFetcherFetchContents(t *testing.T) {
 			gomock.Any(),
 		).Return(fileLeaf)
 		symlinkLeaf := mock.NewMockLinkableLeaf(ctrl)
-		symlinkFactory.EXPECT().LookupSymlink([]byte("target")).Return(symlinkLeaf)
+		symlinkFactory.EXPECT().LookupSymlink(gomock.Any()).
+			DoAndReturn(func(targetParser path.Parser) (virtual.LinkableLeaf, error) {
+				targetBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker)
+				require.NoError(t, path.Resolve(targetParser, scopeWalker))
+				require.Equal(t, "target", targetBuilder.GetUNIXString())
+				return symlinkLeaf, nil
+			})
 
 		children, err := initialContentsFetcher.FetchContents(fileReadMonitorFactory.Call)
 		require.NoError(t, err)

pkg/filesystem/virtual/configuration/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
         "//pkg/proto/configuration/filesystem/virtual",
         "@com_github_buildbarn_bb_storage//pkg/clock",
         "@com_github_buildbarn_bb_storage//pkg/eviction",
+        "@com_github_buildbarn_bb_storage//pkg/filesystem/path",
         "@com_github_buildbarn_bb_storage//pkg/jmespath",
         "@com_github_buildbarn_bb_storage//pkg/program",
         "@com_github_buildbarn_bb_storage//pkg/random",

pkg/filesystem/virtual/configuration/configuration.go

@@ -6,6 +6,7 @@ import (
 	pb "github.com/buildbarn/bb-remote-execution/pkg/proto/configuration/filesystem/virtual"
 	"github.com/buildbarn/bb-storage/pkg/clock"
 	"github.com/buildbarn/bb-storage/pkg/eviction"
+	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
 	"github.com/buildbarn/bb-storage/pkg/jmespath"
 	"github.com/buildbarn/bb-storage/pkg/program"
 	"github.com/buildbarn/bb-storage/pkg/random"
@@ -111,6 +112,7 @@ func (m *nfsv4Mount) Expose(terminationGroup program.Group, rootDirectory virtua
 			clock.SystemClock,
 			enforcedLeaseTime.AsDuration(),
 			announcedLeaseTime.AsDuration(),
+			path.LocalFormat,
 		),
 		nfsv4.NewNFS40Program(
 			rootDirectory,
@@ -121,6 +123,7 @@ func (m *nfsv4Mount) Expose(terminationGroup program.Group, rootDirectory virtua
 			clock.SystemClock,
 			enforcedLeaseTime.AsDuration(),
 			announcedLeaseTime.AsDuration(),
+			path.LocalFormat,
 		),
 	})
 

pkg/filesystem/virtual/directory.go

@@ -72,7 +72,7 @@ type Directory interface {
 	VirtualRemove(name path.Component, removeDirectory, removeLeaf bool) (ChangeInfo, Status)
 	// VirtualSymlink creates a symbolic link within the current
 	// directory.
-	VirtualSymlink(ctx context.Context, pointedTo []byte, linkName path.Component, requested AttributesMask, attributes *Attributes) (Leaf, ChangeInfo, Status)
+	VirtualSymlink(ctx context.Context, pointedTo path.Parser, linkName path.Component, requested AttributesMask, attributes *Attributes) (Leaf, ChangeInfo, Status)
 }
 
 const (

pkg/filesystem/virtual/fuse/simple_raw_file_system.go

@@ -31,7 +31,8 @@ const (
 		virtual.AttributesMaskOwnerGroupID |
 		virtual.AttributesMaskOwnerUserID |
 		virtual.AttributesMaskPermissions |
-		virtual.AttributesMaskSizeBytes
+		virtual.AttributesMaskSizeBytes |
+		virtual.AttributesMaskSymlinkTarget
 	// AttributesMaskForFUSEDirEntry is the attributes mask to use
 	// for VirtualReadDir() to populate all relevant fields of
 	// fuse.DirEntry.
@@ -183,11 +184,18 @@ func populateAttr(attributes *virtual.Attributes, out *fuse.Attr) {
 	}
 	out.Mode |= uint32(permissions.ToMode())
 
-	sizeBytes, ok := attributes.GetSizeBytes()
-	if !ok {
+	if sizeBytes, ok := attributes.GetSizeBytes(); ok {
+		out.Size = sizeBytes
+	} else if symlinkTarget, ok := attributes.GetSymlinkTarget(); ok {
+		pathBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker)
+		if err := path.Resolve(symlinkTarget, scopeWalker); err == nil {
+			out.Size = uint64(len(pathBuilder.GetUNIXString()))
+		} else {
+			out.Size = 1
+		}
+	} else {
 		panic("Attributes do not contain mandatory size attribute")
 	}
-	out.Size = sizeBytes
 }
 
 func populateEntryOut(attributes *virtual.Attributes, out *fuse.EntryOut) {
@@ -499,7 +507,7 @@ func (rfs *simpleRawFileSystem) Symlink(cancel <-chan struct{}, header *fuse.InH
 	rfs.nodeLock.RUnlock()
 
 	var attributes virtual.Attributes
-	child, _, vs := i.VirtualSymlink(ctx, []byte(pointedTo), path.MustNewComponent(linkName), AttributesMaskForFUSEAttr, &attributes)
+	child, _, vs := i.VirtualSymlink(ctx, path.UNIXFormat.NewParser(pointedTo), path.MustNewComponent(linkName), AttributesMaskForFUSEAttr, &attributes)
 	if vs != virtual.StatusOK {
 		return toFUSEStatus(vs)
 	}
@@ -517,8 +525,17 @@ func (rfs *simpleRawFileSystem) Readlink(cancel <-chan struct{}, header *fuse.In
 	i := rfs.getLeafLocked(header.NodeId)
 	rfs.nodeLock.RUnlock()
 
-	target, vs := i.VirtualReadlink(ctx)
-	return target, toFUSEStatus(vs)
+	var attributes virtual.Attributes
+	i.VirtualGetAttributes(ctx, virtual.AttributesMaskSymlinkTarget, &attributes)
+	symlinkTarget, ok := attributes.GetSymlinkTarget()
+	if !ok {
+		return nil, fuse.EINVAL
+	}
+	pathBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker)
+	if err := path.Resolve(symlinkTarget, scopeWalker); err != nil {
+		return nil, fuse.EIO
+	}
+	return []byte(pathBuilder.GetUNIXString()), fuse.OK
 }
 
 func (rfs *simpleRawFileSystem) Access(cancel <-chan struct{}, input *fuse.AccessIn) fuse.Status {