[PATCH] Fix failure to update dirpath

[Anoop C S]

On Tue, 2018-08-21 at 09:54 -0700, Jeremy Allison wrote:
> On Tue, Aug 21, 2018 at 11:42:40AM +0530, Anoop C S wrote:
> > On Mon, 2018-08-20 at 18:35 -0700, Jeremy Allison wrote:
> > > On Mon, Aug 20, 2018 at 06:02:31PM -0700, Jeremy Allison via samba-technical wrote:
> > > > 
> > > > Hang on a minute. I've been looking really closely
> > > > now at the *callers* of check_parent_exists() and
> > > > many of them depend on *dirpath == '\0' in the
> > > > case where smb_fname->base_name doesn't contain
> > > > a component containing '/' characters.
> > > > 
> > > > I think my fix, although it might work, will affect
> > > > other paths. Let me look at this more closely
> > > > tomorrow. I'm withdrawing this for now.
> > > 
> > > OK, my fix was way too invasive for the fix that
> > > you need to make Gluster work again for that bug.
> > > 
> > > The problem is that the code that constructs
> > > the complete pathname depends on *dirpath == '\0'
> > > to avoid adding a "./" prefix to every constructed
> > > pathname. My patch would cause all of those
> > > code paths to change to the full/component/path
> > > cases, which is not what we want. That's going
> > > to affect what goes into the stat-cache as well
> > > as many other things I'll have to go through
> > > carefully to find.
> > 
> > Ok.
> > 
> > > This is a set of code-paths that we might clean
> > > up in the future, but not for this specific bug
> > > I think.
> 
> FYI, the attached diff is the full clean-up
> that is required to make everything
> work correctly (otherwise it fails in the
> samba3.blackbox.smbclient_s3.SMB3.plain
> test).

Ok. We have few other checks on "dirpath" for '\0' inside unix_convert() which we  missed in
previous version which might have caused the test failures. And I see that "dirpath" is now being
allocated to "." by default.

Glad to hear that this comprehensive change also passed autobuild.

> I'm not planning to propose this as it's a
> more invasive change than we need, but I wanted
> to make sure I fully understood what the
> issue was :-).

Sure.

When it comes to proposing the change for review commit message(and subject) could be adapted to
reflect the changes included rather than mere mentioning of check_parent_exists().

> > > Can you test the following change instead ?
> > 
> > The attached diff also works.
> 
> Thanks for testing !
> 
> > > Much simpler and I think it will fix the immediate
> > > Gluster problem (it's the same change that already
> > > exists inside get_real_filename_full_scan() so
> > > I think this is something we already ran across
> > > there).
> > 
> > Considering the fact that we are not 100% sure about the side effects of previous fix which was
> > more
> > aggressive, the current solution is more clean and elegant. In addition to that the presence of
> > same
> > statements inside get_real_filename_full_scan(), which is a fallback, gives us more confidence
> > to
> > fix it here.
> > 
> > I hope you you will be attaching the diff as a patch to other thread for review.
> 
> Yep, I'll post the "simple" patch as the
> proposed fix for bug:
> 
> https://bugzilla.samba.org/show_bug.cgi?id=13585
> 
> in another thread.

Thanks for your time.

> Thanks for all your help !
> 
> Cheers,
> 
> 	Jeremy.