Fwd: Busybox[PATCH] for mount.c - enable fsc as a mount option for NFS

[Roland Arnold]

On Wednesday 13 May  10:49, Denys Vlasenko wrote:
> On Tuesday 12 May 2009 09:40, Roland Arnold wrote:
>> Hi All,
>>
>> This is a patch to mount.c to allow the file-system caching 'fsc' flag
>> for NFS mounts. FS-Cache is new in the 2.6.30 kernel. Note that this
>> changes the behaviour of the mount sys-call to pass through a string
>> and not a struct anymore; it makes the code match what nfs-utils does
>> - at least for NFS3.
>>
> ...
>> @@ -1538,7 +1541,7 @@ static int nfsmount(struct mntent *mp, l
>>   do_mount: /* perform actual mount */
>>
>>       mp->mnt_type = (char*)"nfs";
>> -     retval = mount_it_now(mp, vfsflags, (char*)&data);
>> +     retval = mount_it_now(mp, vfsflags, (char*)mp->mnt_opts);
>>       goto ret;
>
> After this change, "data" variable becomes unused, right?

Yes, that is true. However changing the data type sent to the kernel
is quite a large step, and the initial patch aimed to keep the code
changes to a minimum.

> How kernel differentiates between string and data struct here?

AFAICS, looking at nfs_validate_mount_data() in super.c (see
http://lxr.linux.no/linux+v2.6.29/fs/nfs/super.c#L1542), the void *
data is cast to an nfs_mount_data struct, and a switch statement is
done on on the struct's version. In the case of being passed a string
(as is what happens now with the patch applied), this will never match
anything except for the switch's default-statement, which then parses
it as a char * and does the right things.

> Is it safe?

Yes, any normal ascii characters in the string are never going to have
a value from 1-6 and accidentally break things in the switch. Either
way, any attempt to pass in anything strange gets rejected (or it's a
kernel bug, and I didn't see anything potentially bad there).

On Thursday 14 May 2009 00:12 Denys Vlasenko wrote:
>> This is a patch to mount.c to allow the file-system caching 'fsc' flag
>> for NFS mounts. FS-Cache is new in the 2.6.30 kernel. Note that this
>> changes the behaviour of the mount sys-call to pass through a string
>> and not a struct anymore; it makes the code match what nfs-utils does
>> - at least for NFS3.
>
> AFAIKS this works only starting from 2.6.23. If you want to use that
> anyway (which is a good idea, this struct nfs_mount_data
> mustn't be invented in the first place), you need
> to read /proc/version and do it conditionally.
>
> Otherwise you'll make life of 2.4.x people miserable.

Fair enough - knowing that helps - I don't want to break this for anyone.

>> --- orig/util-linux/mount.c   2009-05-09 13:35:41.854799281 +0200
>> +++ mod/util-linux/mount.c    2009-05-09 13:33:23.147299934 +0200
>> @@ -991,8 +991,9 @@ static int nfsmount(struct mntent *mp, l
>>       mclient = NULL;
>>
>>       /* NB: hostname, mounthost, filteropts must be free()d prior to return */
>> -
>> -     filteropts = xstrdup(filteropts); /* going to trash it later... */
>> +
>> +     free(mp->mnt_opts);
>> +     mp->mnt_opts = xstrdup(filteropts); /* going to trash it later... */
>
> Why?

mp->mnt_opts holds the original options string from the user (so it's
close), except this includes things like 'noatime', 'rw', etc. that
must be filtered out in the options string we send through in the
mount() call. filteropts is exactly the string I want, but it gets
trashed by strtok later on, before I can use it. Based on the comment
at the top of this function '// NB: mp->xxx fields may be trashed on
exit', I figured that changing mp->mnt_opts was acceptable, rather
than introducing a new variable, but it is slightly obscure, and I can
see how you might not like it.

At this stage, what would be best? I can make a new patch that
implements the /proc/version test you mentioned, but it will probably
only be over the weekend.

Thanks for the feedback!
Roland Arnold