Messages in this thread | | | Date | Mon, 27 Oct 2008 13:40:50 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 4/6] autofs4 - make autofs type usage explicit |
| |
On Thu, 23 Oct 2008 10:35:32 +0800 Ian Kent <raven@themaw.net> wrote:
> This patch further improves autofs mount type usage and provides > supplementry explanation of the changes made in the previous patch > "autofs4 - cleanup autofs mount type usage". > > Changes introduced in "autofs4 - cleanup autofs mount type usage": > > - the type assigned at mount when no type is given is changed > from 0 to AUTOFS_TYPE_INDIRECT. This was done because 0 and > AUTOFS_TYPE_INDIRECT were being treated implicitly as the same > type. > > - previously, an offset mount had it's type set to > AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET but the mount control > re-implementation needs to be able distinguish all three types. > So this was changed to make the type setting explicit. > > - a type AUTOFS_TYPE_ANY was added for use by the re-implementation > when checking if a given path is a mountpoint. It's not really a > type as we use this to ask if a given path is a mountpoint in the > autofs_dev_ioctl_ismountpoint() function. > > Changes introduced in this patch: > > - macros to set and test the autofs mount types have been added to > improve readability and make the type usage explicit.
^^^^^^^^^^^^^^^^^^^ <<-- ??
> - the mount type is used from user space for the mount control > re-implementtion so, for consistency, all the definitions have > been moved to the user space include file include/linux/auto_fs4.h. > > ... > > - if (sbi->type == AUTOFS_TYPE_INDIRECT) > + if (autofs_type_indirect(sbi->type))
spose so.
> - *type = AUTOFS_TYPE_INDIRECT; > + set_autofs_type_indirect(*type);
That's pretty nasty. One doesn't expect a "function" to modify a variable which was passed by value.
This interface _requires_ that set_autofs_type_indirect() be implemented as a macro.
This didn't improve readability.
> > ... > > +#define set_autofs_type_indirect(type) (type = AUTOFS_TYPE_INDIRECT)
You'll find very few places in the kernel pull tricks like this, for good reasons. The obnoxious exceptions include local_irq_save() and friends.
> +#define autofs_type_indirect(type) (type == AUTOFS_TYPE_INDIRECT)
I guess that's OK.
But why was it implemented as a macro? It didn't _need_ to be implemented in cpp - it could have been implemented in C.
> + > +#define set_autofs_type_direct(type) (type = AUTOFS_TYPE_DIRECT) > +#define autofs_type_direct(type) (type == AUTOFS_TYPE_DIRECT) > + > +#define set_autofs_type_offset(type) (type = AUTOFS_TYPE_OFFSET) > +#define autofs_type_offset(type) (type == AUTOFS_TYPE_OFFSET) > + > +#define autofs_type_trigger(type) \ > + (type == AUTOFS_TYPE_DIRECT || type == AUTOFS_TYPE_OFFSET)
And this one is dangerous. If passed an expression-with-side-effects it will evaluate that expression either once or twice. Bad. Should be implemented in C.
| |