Hi Jeff,
On Fri, Apr 01, 2011 at 11:01:57AM -0400, Jeff Layton wrote:
> On Fri, 1 Apr 2011 16:26:25 +0200
> sean finney <seanius@xxxxxxxxxxx> wrote:
> > On Fri, Apr 01, 2011 at 09:09:08AM -0400, Jeff Layton wrote:
> > > Thanks for respinning the set, this looks much easier to review. One
> > > thing I notice right offhand is that the new code doesn't check to see
> > > whether referral_walks_count > MAX_NESTED_LINKS (like the original call
> > > site does).
> > >
> > > That means the client can loop here indefinitely if there is a referral
> > > loop. I think we need that check in there.
> >
> > Oops, that wasn't intentional to be removed. Will restore it.
Or... hm.. no i still see the check in place, doesn't look to me like
I did remove it. The other places where we attempt to expand the
referral (the code added in this patch) shouldn't be affected by loops
either since it's only done before the referall walking has started:
if (referral_walks_count == 0) {
int refrc = expand_dfs_referral(xid, pSesInfo, volume_info,
cifs_sb, false);
if (!refrc) {
referral_walks_count++;
goto try_mount_again;
}
}
Or am I missing something?
> > On Fri, Apr 01, 2011 at 09:27:17AM -0400, Jeff Layton wrote:
> > > On Mon, 28 Mar 2011 12:19:29 +0200
> > > Sean Finney <seanius@xxxxxxxxxxx> wrote:
> > While doing this I was thinking to myself that it might make more sense
> > to unconditionally have cifs_sb->mountdata available, and always kstr(n)dup
> > it before passing it along instead of the global mountdata option, but felt
> > I might be overstepping the scope of what I was originally intending to do.
> > What do you think?
> >
>
> Good point. I wasn't thinking about that case.
>
> I think having cifs_sb->mountdata as an unconditional member would be a
> better option. We don't generally have *that* many superblocks and it
> would be nice to get rid of some #ifdef's. In most cases these strings
> are quite small so if someone compiles DFS support out, it won't be
> wasting that much memory.
Okay, I'll fix "Simplify handling of submount options" to do this then.
On Fri, Apr 01, 2011 at 11:04:12AM -0400, Jeff Layton wrote:
> Another argument for this solution...
>
> The CIFS code currently handles remounts poorly. If we make it a point
> to keep the original mount options around it may make it easier to
> eventually fix that.
Additionally, from what I can tell with DFS there are two sets of
"options", the ones that were passed from mount() and the ones that were
generated by taking the previous options and overlaying them with the
referral's mount data. If we do the above, cifs_sb->mountdata will have
the latter, which woudl probably not do exactly the right thing when
the DFS share needed to be remounted (i.e. an admin takes one of the
servers out of the DFS pool we should probably walk the referrals all
over again, not just try to reconnect).
But I don't think it'd be a great deal of effort to support that case
as well while fixing the above, at the expense of keeping both
of these mountdata informations in seperate members in cifs_sb (the
submount data could be null for non DFS shares, at least).
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
