Magic symlinks redux

[Ivan Voras]

Luigi Rizzo wrote:
> On Fri, Aug 22, 2008 at 01:54:29AM +0200, Ivan Voras wrote:
>> I was reading about new things in NetBSD, and one thing caught my 
>> attention: per-user /tmp. See 
>> http://www.feyrer.de/NetBSD/bx/blosxom.cgi/nb_20080714_0251.html for 
>> example.
>>
>> Google says that a discussion about magic symlinks happens every now and 
>> then in FreeBSD but nothing really gets done. I found this 
>> implementation which looks like it's for 7.0:
>>
>> http://butcher.heavennet.ru/patches/kernel/magiclinks/
> 
> interestingly simple.

Yes, that's a big part of the attractiveness of this patch :)

> Question - is the process' ENV easily available in this part
> of the kernel, so one could in principle use environment variables
> as replacement strings ?
> 
> Some comments on the code in the above patch:
> 
> + readability it might be improved a bit:
>   e.g. I don't see why uma_{zalloc|zfree} are hidden behind macros,
>   nor why symlynk_magic() isn't simply called as
> 
> 	if (vfs_magiclinks)
> 		symlink_magic(td, cp, &linklen);
> 
>   as it cannot fail as implemented;

Ok.

>   also, the whole MATCH/SUBSTITUTE macros are not particularly
>   readable -- i understand one needs macros to implement sizeof("somestring")
>   correctly, but apart from a wrapper I believe the core of these two
>   blocks should be implemented by functions (possibly inline) with
>   MATCH() returning the match length so one doesn't need to replicate
>   the string parameter in SUBSTITUTE();

Yes, I intended to remove the code macros into static inline functions, 
possibly with some macro glue for sizeof.

> + correctness --
>   1. termchar is not reset to '/' if a match is not found
>   2. what is the intended behaviour when the replacement string overflows
>      the buffer ?

I'll check those later, when I make an updated patch.

> + efficiency of symlink_magic() might be improved too:
>   e.g. the function could do a quick check for the presence of @ and return
>   without allocation/deallocation if not found;

I think it's because the author wanted a single pass over the string (in 
case of the "extended" @{...} syntax we can't just check if cp[0] == 
'@'). The first few lines of the symlink_magic loop ("if (cp[i] != 
'@')") effectively do what strchr() does.

>   getcredhostname() (and similar routines) could be called so that
>   they write directly to tmp, without the need for
>   allocating an in-stack buffer

I think this is for consistency in calling SUBSTITUTE. It could be 
broken into two variants of the code but is it worth it? I'd rather 
modify getcredhostname() in kern/kern_jail.c to return the length of the 
created string and avoid a strlen().