Hi,
Thank you for the quick review.
On 06-06-19 13:51, David Howells wrote:
Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
+enum {
+ SHFL_FN_QUERY_MAPPINGS = 1, /* Query mappings changes. */
+ SHFL_FN_QUERY_MAP_NAME, /* Query map name. */
+ SHFL_FN_CREATE, /* Open/create object. */
+ SHFL_FN_CLOSE, /* Close object handle. */
+ SHFL_FN_READ, /* Read object content. */
+ SHFL_FN_WRITE, /* Write new object content. */
+ SHFL_FN_LOCK, /* Lock/unlock a range in the object. */
+ SHFL_FN_LIST, /* List object content. */
+ SHFL_FN_INFORMATION, /* Query/set object information. */
+ /* Note function number 10 is not used! */
+ SHFL_FN_REMOVE = 11, /* Remove object */
+ SHFL_FN_MAP_FOLDER_OLD, /* Map folder (legacy) */
+ SHFL_FN_UNMAP_FOLDER, /* Unmap folder */
+ SHFL_FN_RENAME, /* Rename object */
+ SHFL_FN_FLUSH, /* Flush file */
+ SHFL_FN_SET_UTF8, /* Select UTF8 filename encoding */
+ SHFL_FN_MAP_FOLDER, /* Map folder */
+ SHFL_FN_READLINK, /* Read symlink dest (as of VBox 4.0) */
+ SHFL_FN_SYMLINK, /* Create symlink (as of VBox 4.0) */
+ SHFL_FN_SET_SYMLINKS, /* Ask host to show symlinks (as of 4.0) */
+};
If these are protocol numbers that can't be changed, I would assign the value
on all of them.
Ok, will do for the next version.
If they're used by userspace, should they be moved into a
uapi header (and the same for the other stuff in this file)?
This is the protocol between the guest and the hypervisor, so it is not uapi.
+static const struct fs_parameter_spec vboxsf_param_specs[] = {
+ fsparam_string("nls", opt_nls),
+ fsparam_u32("uid", opt_uid),
+ fsparam_u32("gid", opt_gid),
+ fsparam_u32("ttl", opt_ttl),
+ fsparam_u32oct("dmode", opt_dmode),
+ fsparam_u32oct("fmode", opt_fmode),
+ fsparam_u32oct("dmask", opt_dmask),
+ fsparam_u32oct("fmask", opt_fmask),
+ {}
+};
I would format this with tabs so that everything nicely lines up:
static const struct fs_parameter_spec vboxsf_param_specs[] = {
fsparam_string ("nls", opt_nls),
fsparam_u32 ("uid", opt_uid),
fsparam_u32 ("gid", opt_gid),
fsparam_u32 ("ttl", opt_ttl),
fsparam_u32oct ("dmode", opt_dmode),
fsparam_u32oct ("fmode", opt_fmode),
fsparam_u32oct ("dmask", opt_dmask),
fsparam_u32oct ("fmask", opt_fmask),
{}
};
but, otherwise, good!
Ok, will fix.
+ case opt_uid:
+ ctx->o.uid = result.uint_32;
+ break;
+ case opt_gid:
+ ctx->o.gid = result.uint_32;
+ break;
Should you be using kuid/kgid transforms for the appropriate namespace?
That is probably a good idea, will do for the next version.
opts->fs_uid = make_kuid(current_user_ns(), option);
if (!uid_valid(opts->fs_uid))
return -EINVAL;
sort of thing (excerpt from fs/fat/inode.c).
Shouldn't this use the user-namespace from the filesystem-context?
+ case opt_ttl:
+ ctx->o.ttl = msecs_to_jiffies(result.uint_32);
+ break;
Is 0 valid?
Yes, 0 means to always pass any stat() calls through to the host
instead of relying on cached values.
+ case opt_dmode:
+ ctx->o.dmode = result.uint_32;
+ break;
+ case opt_fmode:
+ ctx->o.fmode = result.uint_32;
+ break;
+ case opt_dmask:
+ ctx->o.dmask = result.uint_32;
+ break;
+ case opt_fmask:
+ ctx->o.fmask = result.uint_32;
+ break;
Do these need vetting? I guess you kind of do:
Well I could refuse values where (result.uint_32 & 0777)
is true here I guess; and then remove the & 0777 below:
inode->i_mode =
sf_g->o.dmode != ~0 ? (sf_g->o.dmode & 0777) : mode;
inode->i_mode &= ~sf_g->o.dmask;
I'm guessing you're stuck with the mount options?
More or less, yes. Changing them is going to be quite painful
from a userspace compat pov.
+struct vboxsf_options {
+ int ttl;
jiffies are unsigned long.
Ok.
+ int uid;
+ int gid;
kuid_t & kgid_t.
Ok.
+ int dmode;
+ int fmode;
+ int dmask;
+ int fmask;
+};
umode_t.
Ok.
Regards,
Hans
