a few cleanup patches

[Volker Lendecke]

On Thu, Oct 24, 2013 at 09:59:19AM -0700, Jeremy Allison wrote:
> On Thu, Oct 24, 2013 at 05:16:02PM +0200, Volker Lendecke wrote:
> > On Thu, Oct 24, 2013 at 02:17:30PM +0200, David Disseldorp wrote:
> > > Hi Volker,
> > > 
> > > On Thu, 24 Oct 2013 11:41:25 +0200
> > > Volker Lendecke <Volker.Lendecke at SerNet.DE> wrote:
> > > 
> > > > Hi!
> > > > 
> > > > Attached find a few smaller cleanup patches in the oplock
> > > > area. No real functional change around, just rearranging
> > > > code and parameters.
> > > > 
> > > > Please review&push!
> > > 
> > > The changes look good. I've one remaining question after review:
> > > badab2c2bba00ebd8 smbd: Move oplock/sharemode ops into one place
> > > 2690         if (file_existed) {
> > > 2691                 /* stat opens on existing files don't get oplocks. */
> > > 2692                 if (is_stat_open(open_access_mask)) {
> > > 2693                         oplock_request = NO_OPLOCK;
> > > 2694                 }
> > > 2695         }
> > > 
> > > 
> > > Shouldn't the file_existed check be removed completely, to keep the same
> > > behaviour as beforehand?
> > 
> > Still looking. Did you push the patchset by accident?
> 
> There's a behavior change in this hunk I think might need
> addressing.
> 
> Setting "oplock_request = NO_OPLOCK" here loses any
> INTERNAL_OPEN_ONLY flag that was set in oplock_request.
> 
> This matters due to the following code in grant_fsp_oplock_type()
> which is called immediately after:
> 
>         if (oplock_request & INTERNAL_OPEN_ONLY) {
>                 /* No oplocks on internal open. */
>                 fsp->oplock_type = NO_OPLOCK;
>                 DEBUG(10,("grant_fsp_oplock_type: oplock type 0x%x on file %s\n",
>                         fsp->oplock_type, fsp_str_dbg(fsp)));
>                 return;
>         }
> 
> I'm trying (long term) to get rid of INTERNAL_OPEN_ONLY
> but at the moment it's still used, so I think the
> following fix is needed.
> 
> Please review and apply if you agree.

Can you explain the behaviour change with respect to the final
outcome? The only effect this has if I see it correctly is that in
grant_fsp_oplock_type we check for file_has_brlocks. If the access_mask
indicates a stat open, further down we exit grant_fsp_oplock_type without
further action, we already set it to NO_OPLOCK.

Thanks,

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de