Re: [GIT PULL] smb3 client fixes

[Linus Torvalds]

On Mon, 13 Apr 2026 at 20:13, Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
>
> Do the below changes look any better?

Let me suggest:

> -void d_mark_tmpfile_name(struct file *file, const struct qstr *name)
> +int d_mark_tmpfile_name(struct file *file, const struct qstr *name)
>  {
>         struct dentry *dentry = file->f_path.dentry;
>         char *dname = dentry->d_shortname.string;
>
> -       BUG_ON(dname_external(dentry));
> -       BUG_ON(d_really_is_positive(dentry));
> -       BUG_ON(!d_unlinked(dentry));
> -       BUG_ON(name->len > DNAME_INLINE_LEN - 1);
> +       if (WARN_ON_ONCE(dname_external(dentry)))
> +               return -EINVAL;
> +       if (WARN_ON_ONCE(d_really_is_positive(dentry)))
> +               return -EINVAL;
> +       if (WARN_ON_ONCE(!d_unlinked(dentry)))
> +               return -EINVAL;
> +       if (WARN_ON_ONCE(name->len > DNAME_INLINE_LEN - 1))
> +               return -ENAMETOOLONG;

Let's just return errors from this, and put the WARN_ON() in the caller.

It's not like the WARN_ON() is ever going to trigger anyway, unless
there's somethingwrong in the caller, so let's not make this helper
infrastructure any uglier than it needs to be. Adding the warnings
there only makes it less obvious.

You want the warnings to be where the *problem* is. Not in the helper
that did everything right, but was passed the wrong values.

See what I'm saying?

Also, this is a bit of a mixed bag:

> -               scnprintf(name, size,
> -                         CIFS_TMPNAME_PREFIX "%0*x",
> -                         CIFS_TMPNAME_COUNTER_LEN,
> -                         atomic_inc_return(&cifs_tmpcounter));
> +               /* Append tmpfile name to @path */
> +               namelen = scnprintf(name, namesize, ".__smbfile_tmp%0*x",
> +                                   (int)sizeof(cifs_tmpcounter) * 2,
> +                                   atomic_inc_return(&cifs_tmpcounter));

the 'namelen = ' part is better because it makes it much more obvious
where the numbers come from,. but I certainly didn't mind having a
CIFS_TMPNAME_PREFIX define to help abstract things out a bit.

What I minded was seeing *three* different #defines where one was a
magic nmumber that was very tighly tied to the others, but they didn't
help make that obvious.

So it's not that I minded the CIFS_TMPNAME_PREFIX thing,

I minded ther *magic* of it all that made it so obvious that somebody
knew a-priori what they were going to get, and then made the code
match the result they wanted, but without making the code show how
they got there.

That whole "namelen = scnprintf()" is a really cheap way to show "this
is how I got the name length" - because that's what the function
returns anyway.

And the other place you had that scnprintf() you still had that "I
know what the end result is, so I'm just hardcoding it as a random
value".

                            Linus