Bug 8998 - Notify code can miss a ChDir.
Notify code can miss a ChDir.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-14 18:09 UTC by Jeremy Allison
Modified: 2012-06-17 19:09 UTC (History)
0 users

See Also:
rsharpe: review+


Attachments
git am fix for 3.6.x. (3.40 KB, patch)
2012-06-14 18:28 UTC, Jeremy Allison
jra: review? (rsharpe)
vl: review+
Details
git-am fix for 3.5.next. (2.30 KB, patch)
2012-06-14 19:09 UTC, Jeremy Allison
vl: review-
Details
Updated git-am fix for 3.5.next. (2.35 KB, patch)
2012-06-15 15:43 UTC, Jeremy Allison
vl: review+
jra: review? (rsharpe)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2012-06-14 18:09:04 UTC
Richard Sharpe <realrichardsharpe@gmail.com>

>> We are seeing issues in one of our VFSes where relative path names are
>> causing confusion. We are capturing chdir requests in the VFS and
>> saving the path and if we see a relative path name we prepend the
>> captured path name.
>>
>> However, we also check that the path that has been constructed matches
>> the path coming down via the connection pointed to by the handle and
>> we are seeing intermittent exceptions.

Patch for 3.5.x and 3.6.x to follow.
Comment 1 Jeremy Allison 2012-06-14 18:28:12 UTC
Created attachment 7645 [details]
git am fix for 3.6.x.

Richard, please review for 3.6.next.
Jeremy.
Comment 2 Jeremy Allison 2012-06-14 18:46:16 UTC
Richard, I notice in your version of the patch you do the vfs_ChDir() over both the notify_onelevel() call and also the notify_trigger() call with the fullpath. Is that needed ? In Volker's original patch the vfs_ChDir() call is only done over the notify_onelevel(), and not the notify_trigger() call.

Can you explain ?

Thanks,

Jeremy.
Comment 3 Richard Sharpe 2012-06-14 19:01:49 UTC
(In reply to comment #2)
> Richard, I notice in your version of the patch you do the vfs_ChDir() over both
> the notify_onelevel() call and also the notify_trigger() call with the
> fullpath. Is that needed ? In Volker's original patch the vfs_ChDir() call is
> only done over the notify_onelevel(), and not the notify_trigger() call.
> 
> Can you explain ?

I didn't understand what I was doing :-)

Will it cause a problem? I guess I need to look at the underlying code more carefully.
Comment 4 Jeremy Allison 2012-06-14 19:09:32 UTC
Created attachment 7646 [details]
git-am fix for 3.5.next.

Richard, here are the same fixes (combined into one patch) as for 3.6.x (with a few extra log messages that were in your original back-port).

This only keeps the ChDir over the notify_onelevel() call, as does Volker's original patch.

Can you confirm this works please and if so we'll get it into 3.5.next.

Cheers,

Jeremy.
Comment 5 Richard Sharpe 2012-06-14 19:11:11 UTC
Comment on attachment 7645 [details]
git am fix for 3.6.x.

Looks good.
Comment 6 Richard Sharpe 2012-06-14 19:12:10 UTC
I think I have reviewed it ...
Comment 7 Jeremy Allison 2012-06-14 19:12:43 UTC
(In reply to comment #3)

> I didn't understand what I was doing :-)
> 
> Will it cause a problem? I guess I need to look at the underlying code more
> carefully.

No, I don't think it'll cause a problem, I just think it's unnecessary :-).

Can you test the 3.5.next patch I just uploaded and if you're happy we'll
re-assign to Karolin for inclusion.

Cheers,

Jeremy.
Comment 8 Jeremy Allison 2012-06-14 19:14:18 UTC
Ok, I saw the change you made changing the review flag from '?' to '+' in an email, but I don't see the change actually on the bug. I don't get it :-(.

Still, the comments inline are +1, so I'll re-assign to Karolin for inclusion in 3.6.next and 3.5.next.

Jeremy.
Comment 9 Richard Sharpe 2012-06-14 19:36:00 UTC
(In reply to comment #4)
> Created attachment 7646 [details]
> git-am fix for 3.5.next.
> 
> Richard, here are the same fixes (combined into one patch) as for 3.6.x (with a
> few extra log messages that were in your original back-port).
> 
> This only keeps the ChDir over the notify_onelevel() call, as does Volker's
> original patch.
> 
> Can you confirm this works please and if so we'll get it into 3.5.next.
 
I believe that the fix will work. It is functionally equivalent to what I am currently running and that no longer seems to drop the ChDir.

I will move to that patch, but it will probably be tomorrow before I actually have it running in code in our torture environment.
Comment 10 Volker Lendecke 2012-06-15 10:10:16 UTC
Comment on attachment 7646 [details]
git-am fix for 3.5.next.

Doesn't this create unused variables? oldwd and smb_fname_parent seem unused in notify_fname
Comment 11 Jeremy Allison 2012-06-15 15:43:18 UTC
Created attachment 7650 [details]
Updated git-am fix for 3.5.next.

Removes unused variables. Thanks Volker !
Comment 12 Jeremy Allison 2012-06-15 17:01:41 UTC
Karolin, please push the 3.6.x patch - let's wait on Volker or Richard for the 3.5.x version.

Cheers,

Jeremy.
Comment 13 Karolin Seeger 2012-06-15 19:06:18 UTC
(In reply to comment #12)
> Karolin, please push the 3.6.x patch - let's wait on Volker or Richard for the
> 3.5.x version.
> 
> Cheers,
> 
> Jeremy.

Pushed to v3-6-test.
Comment 14 Karolin Seeger 2012-06-17 19:09:51 UTC
Pushed to v3-5-test (Volker has granted review).
Closing out bug report.

Thanks!