Get rid of Directory.VirtualSymlink()

[EdSchouten]

In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks.

Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.

Cc @visualphoenix

[visualphoenix]

Are you sure you want to do this? Imho it feels unexpected for mknod to make symlinks…

[visualphoenix]

Like, put another way - if VirtualMknod can subsume VirtualSymlink because Attributes can carry SymlinkTarget, why not VirtualMkdir too? I feel like the name should match what they do... Keeping the interface aligned with POSIX makes it easier to reason about...

[EdSchouten]

Our VFS is slightly different from some other implementations, in that symlink targets are also modeled as an attribute. This wasn't always the case. We used to have a VirtualReadlink() method, however we got rid of it in #216 to improve robustness of symlink resolution on Windows.

In #226 you changed VirtualMknod() to allow passing in arbitrary file creation attributes. This means that the signature of VirtualMknod() can already be used to create symlinks. It's just that we don't use it that way. However, supporting this would simplify the VFS API, and would in fact make it more consistent with protocols like NFSv4. NFSv4 also doesn't have a separate SYMLINK operation. Instead, symlinks are created using the same operation as for FIFOs, sockets, devices, etc.

[EdSchouten]

Like, put another way - if VirtualMknod can subsume VirtualSymlink because Attributes can carry SymlinkTarget, why not VirtualMkdir too?

That is a very good question. Right now VirtualMkdir and VirtualMknod have two different signatures. One returns a Directory and the other one a Leaf. Sure, we could return a DirectoryChild, but that would require the caller to do additional type conversions in case it needs one of the more specific types.

[visualphoenix]

You're obviously right in so far as NFS is concerned ❤️ NFS ops vs POSIX syscalls arent 1-1 in their design - that's for sure! So would you ever consider mirroring what you did in go-xdr for the virtual interface? Right now by the time VirtualMknod runs the type safe union has become a FileType and bag of fields by convention - I just wondered what you thought... would you ever consider something like:

 case *nfsv4.Createtype4_NF4LNK:
  if !utf8.Valid(objectType.Linkdata) {
    return &nfsv4.Create4res_default{Status: nfsv4.NFS4ERR_BADCHAR}
  }
  leaf, changeInfo, vs = currentDirectory.VirtualMknod(
    ctx, name,
    &virtual.CreateSymlink{Target: path.UNIXFormat.NewParser(string(objectType.Linkdata))},
    virtual.AttributesMaskFileHandle, &actualAttributes,
  )

[EdSchouten]

Woah, looks like I forgot to respond to this question.

What you proposed would also be possible, but it might lead to more duplication of code. I think that reusing the Attributes structure like we do now is not necessarily bad. It's similar to what the VFS inside the BSD kernels have. They also have a 'struct stat' equivalent that has a bit mask of the fields that are actually set.