Re: [LSF/MM/BPF TOPIC] Allowing linkat() to replace the destination

[Omar Sandoval <osandov@osandov.com>]

On Sat, Jan 18, 2020 at 12:47:38AM +0000, Al Viro wrote:
> On Fri, Jan 17, 2020 at 03:54:44PM -0800, Omar Sandoval wrote:
>  
> > > 	3) permission checks need to be specified
> > 
> > I believe the only difference here vs standard linkat is that newpath
> > must not be immutable or append-only?
> 
> I would bloody hope not - at the very least you want sticky bit on parent
> to have effect, same as with rename()/rmdir()/unlink()...

Right, I should've reread may_delete(). I'll document that, too.

> > > references to pathconf, Cthulhu and other equally delightful entities
> > > are not really welcome.
> > 
> > EOPNOTSUPP is probably the most helpful.
> 
> Umm...  What would you feed it, though?  You need to get past your
> "links to the same file, do nothing" escape...

I think what you're getting at is that we can make this easier by
failing linkat AT_REPLACE very early if the filesystem doesn't have a
->link_replace(). Namely, if the filesystem doesn't support AT_REPLACE
but we still allow the "same file" or "newpath doesn't exist" cases to
succeed, then feature detection gets annoying.

As long as that's right, then applications can do the usual "try the new
feature or fall back" pattern that they do for fallocate modes and such.

> > Based on my previous attempt at it [1], it's not too bad.
> 
> +                       error = may_delete(dir, new_dentry, d_is_dir(old_dentry));                                       
> 
> Why bother with d_is_dir(), when you are going to reject directories
> anyway?
> 
> +       if (dir->i_op->link)                                                                                             
> +               error = dir->i_op->link(old_dentry, dir, new_dentry);                                                    
> +       else                                                                                                             
> +               error = dir->i_op->link2(old_dentry, dir, new_dentry, flags);                                            
> +       if (error)                                                                                                       
> +               goto out;                                                                                                
> +                                                                                                                        
> 
> No.  This is completely wrong; just make it ->link_replace() and be done
> with that; no extra arguments and *always* the same conditions wrt
> positive/negative.  One of the reasons why ->rename() tends to be
> ugly (and a source of quite a few bugs over years) are those "if
> target is positive/if target is negative" scattered over the instances.
> 
> Make the choice conditional upon the positivity of target.

Yup, you already convinced me that ->link_replace() is better in your
last email.

> And you don't need to reproduce every quirk of rename() error values.
> Really.  Unless you really intend to have userland do a loop of
> linkat(2) attempts (a-la mkstemp(3)), followed by rename(2) for
> fallback...

Understood, thanks. I'll get this all cleaned up and resent next week.