Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> + params.create_flags = 0
> + | SHFL_CF_DIRECTORY
> + | SHFL_CF_ACT_OPEN_IF_EXISTS
> + | SHFL_CF_ACT_FAIL_IF_NEW | SHFL_CF_ACCESS_READ;
The 0 here would seem to be superfluous. Also, most common practice in the
kernel would put the binary operator at the end of the preceding line.
> +/**
> + * This is called when reference count of [file] goes to zero. Notify
> + * the host that it can free whatever is associated with this directory
> + * and deallocate our own internal buffers
> + * Return: 0 or negative errno value.
> + * @inode inode
> + * @file file
> + */
> +static int sf_dir_release(struct inode *inode, struct file *file)
I'm pretty certain most of your kdoc comments are invalid, but I could be
wrong. Shouldn't this be:
/**
* name_of_function - Brief description
* @arg1: Summary arg 1
* @arg2: Summary arg 2
* @arg3: Summary arg 3
*
* Description...
*/
type name_of_function(type arg1, type arg2, type arg3)
{
...
}
> +static int sf_get_d_type(u32 mode)
unsigned int would be preferable, I think.
> + * Return: 0 or negative errno value.
> ...
> +static int sf_getdent(struct file *dir, loff_t pos,
> + char d_name[NAME_MAX], int *d_type)
> ...
> + return 1;
The return value is not concordant with the function description.
> + * This is called when vfs wants to populate internal buffers with
> + * directory [dir]s contents.
I would say "the directory" rather than "directory [dir]s". It's fairly
obvious what the definite article refers to in this case.
> [opaque] is an argument to the
> + * [filldir]. [filldir] magically modifies it's argument - [opaque]
> + * and takes following additional arguments (which i in turn get from
> + * the host via sf_getdent):
opaque and filldir no longer exist.
> + * name : name of the entry (i must also supply it's length huh?)
> + * type : type of the entry (FILE | DIR | etc) (i ellect to use DT_UNKNOWN)
> + * pos : position/index of the entry
> + * ino : inode number of the entry (i fake those)
I would indent these more (use a tab after the '*' rather than a space).
> + /* d_name now contains a valid entry name */
> + sanity = ctx->pos + 0xbeef;
> + fake_ino = sanity;
> + /*
> + * On 32 bit systems pos is 64 signed, while ino is 32 bit
> + * unsigned so fake_ino may overflow, check for this.
> + */
> + if (sanity - fake_ino) {
Ugh. Why '0xbeef'? Why not '1'? I wonder if:
if ((ino_t)(ctx->pos + 1) != (unsigned long long)(ctx->pos + 1))
would work.
> +/* Query mappings changes. */
> +#define SHFL_FN_QUERY_MAPPINGS (1)
> +/* Query mappings changes. */
> +#define SHFL_FN_QUERY_MAP_NAME (2)
> ...
Enumify?
> +#define SHFL_ROOT_NIL ((u32)~0)
UINT_MAX?
> +#define SHFL_HANDLE_NIL ((u64)~0LL)
ULONGLONG_MAX?
> +/** Shared folder filesystem properties. */
> +struct shfl_fsproperties {
> ...
> +};
> +VMMDEV_ASSERT_SIZE(shfl_fsproperties, 12);
Should this be __packed given the size assertion?
> +static const match_table_t vboxsf_tokens = {
> + { opt_nls, "nls=%s" },
> + { opt_uid, "uid=%u" },
> + { opt_gid, "gid=%u" },
> + { opt_ttl, "ttl=%u" },
> + { opt_dmode, "dmode=%o" },
> + { opt_fmode, "fmode=%o" },
> + { opt_dmask, "dmask=%o" },
> + { opt_fmask, "fmask=%o" },
> + { opt_error, NULL },
> +};
This needs converting to the new mount API. See:
Documentation/filesystems/mount_api.txt
> + if (options[0] == VBSF_MOUNT_SIGNATURE_BYTE_0 &&
> + options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 &&
> + options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 &&
> + options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
> + vbg_err("vboxsf: Old binary mount data not supported, remove obsolete mount.vboxsf and/or update your VBoxService.\n");
> + return -EINVAL;
> + }
This bit should go in your ->parse_monolithic() method.
David
