[linux-cifs-client] Re: jeff patches

[Jeff Layton]

On Tue, 23 Sep 2008 13:00:25 -0500
"Steve French" <smfrench at gmail.com> wrote:

> comments on patch series follow.  Remainder are merged now.
> 
> On Tue, Sep 23, 2008 at 12:50 PM, Steve French <smfrench at gmail.com> wrote:
> 1)        cifs: make cifs_rename handle -EACCES errors      looks fine but has
> dependencies

Yep. That'll have to go in after the others.

> 2)        cifs: update DOS attributes in cifsInode if we successf ...
>     looks fine but has dependencies

Ditto...

> 3)        cifs: clean up error handling in cifs_unlink
> 4)        cifs: undo changes in cifs_rename_pending_delete if ...
> I think this isn't right.   In any case we should set delete pending
> even if we can't rename here:
>         rc = CIFSSMBRenameOpenFile(xid, tcon, netfid, NULL, cifs_sb->local_nls,
>                                    cifs_sb->mnt_cifs_flags &
>                                             CIFS_MOUNT_MAP_SPECIAL_CHR);
> -       if (rc != 0)
> -               goto out_close;
> +       if (rc != 0) {
> +               rc = -ETXTBSY;
> +               goto undo_setattr;
> +       }
> 
>         /* set DELETE_ON_CLOSE */
>         rc = CIFSSMBSetFileDisposition(xid, tcon, true, netfid, current->tgid);
> +       if (rc != 0)
> +               goto undo_rename;
> 

Why would we set DELETE_ON_CLOSE if the rename fails? If it fails, then
we're going to return error on the unlink which means that the file
shouldn't be touched, correct?

> 5)        cifs: add function to set file disposition
>      merged
> 6)        cifs: move rename and delete-on-close logic into helper ...
>      merged
> 7)       cifs: fix busy-file renames and refactor cifs_rename ...
>      Question to resolve still on why forbid cross directory renames?

I suppose it doesn't hurt to try them. It's just that all windows
servers that I've tried fail. Samba servers on the other hand may work,
but they do path-based renames on open files so we don't need to try
this there.

> 8)        cifs: remove NULL termination from rename target in ...
>     looks fine but waiting for answer from ms

Ok. FWIW, wireshark seems to think that the trailing NULL shouldn't be
there.


> 9)        cifs: add constants for string lengths of keynames ...
>    fixed up and merged

Thanks.

> 10)        cifs: clean up upcall handling for dns_resolver keys
>                                   clarify or find similar usage of
> generic fields being overloaded by your patch x.[0] x.[1] (are they
>  described somewhere?)

Unfortunately, nowhere that I know of. Those fields are key-type
specific however, so the generic keys API should just treat them as
opaque.

> 11)        cifs: no need to use rcu_assign_pointer on immutable ...
>                                   seems like it doesn't matter

It's just a micro-efficiency patch. Since it's an immutable key, RCU is
a waste of time. We know that it's not going to be updated...

> 12)        cifs: explicitly revoke SPNEGO key after session setup     the revoke
>  vs. put - who cleans up question needs to be doublechecked
>                                   at first glance it does not look
> like revoke is a superset of put

revoke is not a superset of put. It simply marks the key so that it
cannot be found in a search. This makes certain we won't pick up an
earlier key instead of doing an upcall. We still have to do a key_put
after revoking it.

> 13)        cifs: have find_writeable_file prefer filehandles opene ...
>      merged
>

-- 
Jeff Layton <jlayton at redhat.com>