Re: [PATCH v2] fs: Add VirtualBox guest shared folder (vboxsf) support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 15-01-18 20:32, Amir Goldstein wrote:
> On Mon, Jan 15, 2018 at 7:51 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> VirtualBox hosts can share folders with guests, this commit adds a
>> VFS driver implementing the Linux-guest side of this, allowing folders
>> exported by the host to be mounted under Linux.
>>
>> This driver depends on the guest <-> host IPC functions exported by
>> the vboxguest driver.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   fs/Kconfig                     |   1 +
>>   fs/Makefile                    |   1 +
>>   fs/vboxsf/Kconfig              |   9 +
>>   fs/vboxsf/Makefile             |   3 +
>>   fs/vboxsf/dir.c                | 648 +++++++++++++++++++++++++++++
>>   fs/vboxsf/file.c               | 416 +++++++++++++++++++
>>   fs/vboxsf/shfl_hostintf.h      | 919 +++++++++++++++++++++++++++++++++++++++++
>>   fs/vboxsf/super.c              | 430 +++++++++++++++++++
>>   fs/vboxsf/utils.c              | 589 ++++++++++++++++++++++++++
>>   fs/vboxsf/vboxsf_wrappers.c    | 365 ++++++++++++++++
>>   fs/vboxsf/vboxsf_wrappers.h    |  46 +++
>>   fs/vboxsf/vfsmod.h             | 104 +++++
>>   include/uapi/linux/vbsfmount.h |  62 +++
>>   13 files changed, 3593 insertions(+)
>>   create mode 100644 fs/vboxsf/Kconfig
>>   create mode 100644 fs/vboxsf/Makefile
>>   create mode 100644 fs/vboxsf/dir.c
>>   create mode 100644 fs/vboxsf/file.c
>>   create mode 100644 fs/vboxsf/shfl_hostintf.h
>>   create mode 100644 fs/vboxsf/super.c
>>   create mode 100644 fs/vboxsf/utils.c
>>   create mode 100644 fs/vboxsf/vboxsf_wrappers.c
>>   create mode 100644 fs/vboxsf/vboxsf_wrappers.h
>>   create mode 100644 fs/vboxsf/vfsmod.h
>>   create mode 100644 include/uapi/linux/vbsfmount.h
> 
> A MAINTAINERS entry seems in order.

Ack and thank you for the feedback.

>> diff --git a/fs/Kconfig b/fs/Kconfig
>> index 7aee6d699fd6..7f80ad1cf591 100644
>> --- a/fs/Kconfig
>> +++ b/fs/Kconfig
>> @@ -248,6 +248,7 @@ source "fs/pstore/Kconfig"
>>   source "fs/sysv/Kconfig"
>>   source "fs/ufs/Kconfig"
>>   source "fs/exofs/Kconfig"
>> +source "fs/vboxsf/Kconfig"
>>
>>   endif # MISC_FILESYSTEMS
>>
>> diff --git a/fs/Makefile b/fs/Makefile
>> index ef772f1eaff8..3057830f112a 100644
>> --- a/fs/Makefile
>> +++ b/fs/Makefile
>> @@ -129,3 +129,4 @@ obj-y                               += exofs/ # Multiple modules
>>   obj-$(CONFIG_CEPH_FS)          += ceph/
>>   obj-$(CONFIG_PSTORE)           += pstore/
>>   obj-$(CONFIG_EFIVAR_FS)                += efivarfs/
>> +obj-$(CONFIG_VBOXSF_FS)                += vboxsf/
>> diff --git a/fs/vboxsf/Kconfig b/fs/vboxsf/Kconfig
>> new file mode 100644
>> index 000000000000..620e2232969c
>> --- /dev/null
>> +++ b/fs/vboxsf/Kconfig
>> @@ -0,0 +1,9 @@
>> +config VBOXSF_FS
>> +       tristate "VirtualBox guest shared folder (vboxsf) support"
> 
> 
> Don't know if you noticed, but calling your filesystem vboxsf
> is quite odd name among other XXXfs beasts.

Yes I noticed, note I'm only the guy pushing this upstream this code
has a long out-of-tree history. FWIW the sf stand for "shared folder"

> Will it be an option to re-brand this as vboxfs?
> Even if it is too late or too much of a hustle to change the user visible
> file_system_type name, I think changing the internal name is worth it.

We can quite definitely not change the user-visible name, the mount
arg changes Christoph Hellwig has requested are tricky enough wrt
compatibility with the out-of-tree version most users use atm.

The users will need updated userspace tools to deal with the mount arg
changes, but that is as easy as checking for -EINVAL and trying again
with the new style string args. But figuring out the right fstype name
is rather more tricky and the mount binary name has been mount.vboxsf
for ages...  So I would really like to keep the file_system_type name
as vboxsf, at which point it seems counter-productive to me to rename
the files / kernel-mode to vboxfs.

> The other thing is if you can help it to avoid the short 'sf_' prefix and
> use a longer prefix even for static functions, something like vbsf_ or
> vbfs_ that would be better.

Agreed, I did not take the buildin use-case where everything gets linked
into a single binary into account, will fix for v3.

Regards,

Hans