[PATCH] Final removal of lp_posix_pathnames() from the smbd server main code paths.

[Jeremy Allison]

On Thu, Mar 24, 2016 at 02:35:00PM +0200, Uri Simchoni wrote:
> Done!
> 
> To summarize:
> - I have a couple of cosmetic comments on #5, but it's otherwise RB+ me.

OK, I'll fix those and push.

> - Cannot ack #13 - seems like a "product decision" because it changes
> behavior no to support old buggy clients, plus if we do change behavior
> it seems we can delete the "buggy client support code" rather than
> change it. (see separate message on thread)

I have a plan for the evil patch #13 that should keep
existing behavior for SMB1. New behavior for SMB2 is
of course allowed :-).

> I have some comments on this patch, none of them seem to actually point
> to a bug so I leave it to you whether to fix or not.
> 
> In general reviewing this one proved harder than originally anticipated,
> because if the flags are not copied, we should justify why they are 0. I
> used the heuristic that if the user of the smb_fname knows what it's
> doing with it at the posix level (i.e. it's doing a stat or an lstat or
> mkdir etc) then it's safe to pass 0, because "below the VFS" only
> readdir_fn cares whether it's a POSIX or NT name (to make the proper
> stat) - as can be seen in patch #6.

Yes, I had the same issues when deciding what flags to
use when not copied.

> This means that we may encounter bugs down the road if a future VFS
> module implementing unlink or opendir or mkdir cares whether the
> smb_fname is POSIX or NT, and relies on the flags.

> A couple of cosmetic comments:
> 
> > @@ -3816,7 +3825,8 @@ static void fruit_copy_chunk_done(struct tevent_req *subreq)
> >                         req,
> >                         state->dst_fsp->fsp_name->base_name,
> >                         streams[i].name,
> > -                       NULL);
> > +                       NULL,
> > +                       state->src_fsp->fsp_name->flags);
> >                 if (tevent_req_nomem(dst_fname_tmp, req)) {
> >                         TALLOC_FREE(src_fname_tmp);
> >                         return;
> 
> Should this be state->dst_fsp->fsp_name->flags? (yeah I know they are
> probably the same but it looks cleaner)

Yes, correct - fixed.

> > @@ -299,7 +299,8 @@ static bool recycle_create_dir(vfs_handle_struct *handle, const char *dname)
> >                         smb_fname = synthetic_smb_fname(talloc_tos(),
> >                                                 new_dir,
> >                                                 NULL,
> > -                                               NULL);
> > +                                               NULL,
> > +                                               0);
> >                         if (smb_fname == NULL) {
> >                                 goto done;
> >                         }
> Maybe the flags here better come from the unlink smb_fname?

I don't think so on this one. As we may be making an entire
directory tree here I'd argue we come under the "heuristic
that if the user of the smb_fname knows what it's doing with
it at the posix level".

Thanks for all the help !

Jeremy.