Hi Jeff, Thanks for taking the time with these patches. I think there's only one point where I'll need a further clarification (cifs_sb->mountdata as discussed below). In the meantime, I'll fix up the patches as agreed and after clarification I'll resubmit them (making sure that I stop back-dating the messages as well, oops).
On Fri, Apr 01, 2011 at 09:08:29AM -0400, Jeff Layton wrote: > On Fri, 18 Mar 2011 10:02:07 +0100 > Sean Finney <[email protected]> wrote: > > Nit: > > It's a good idea to prefix the subject with "cifs:" so that when the > patches are committed the subsystem they affect is clear. Makes it > easier for people to scan "git log" by eye. Will do. <snip> > > try_mount_again: > > + > > + /* cleanup activities if we're chasing a referral */ > > + if (referral_walks_count) { > > + if (tcon) > > + cifs_put_tcon(tcon); > > + else if (pSesInfo) > > + cifs_put_smb_ses(pSesInfo); > > + > > + cleanup_volume_info(&volume_info); > > + FreeXid(xid); > > + } > > #endif > > It's a little ugly to put error handling code at the top like this, but > it's probably ok. We need to do a better job of moving the code out of > the CONFIG_CIFS_DFS_UPCALL macro, but that's really beyond the scope of > this. More cleanup than error-handling, but yeah it does make a top-down read a bit awkward. there's also some similar code pre-existing down at the check_mount_failed label. The alternatives that come to mind are * putting another goto label at the bottom, and goto there instead of try_mount_again, do the cleanup, and then goto try_mount_again * extract the cleanup into another external function and call it before doing a goto try_mount_again But I'm happy to let sleeping dogs lie if you're willing to overlook it. <snip> > Reviewed-by: Jeff Layton <[email protected]> Noted. 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. On Fri, Apr 01, 2011 at 09:27:17AM -0400, Jeff Layton wrote: > On Mon, 28 Mar 2011 12:19:29 +0200 > Sean Finney <[email protected]> wrote: <snip> > > @@ -2794,11 +2794,14 @@ expand_dfs_referral(int xid, struct cifs_ses > > *pSesInfo, > > free_dfs_info_array(referrals, num_referrals); > > kfree(fake_devname); > > > > + if (cifs_sb->mountdata != NULL) > > + kfree(cifs_sb->mountdata); > > + > > if (IS_ERR(mdata)) { > > rc = PTR_ERR(mdata); > > mdata = NULL; > > } > > - *mount_data = mdata; > > + cifs_sb->mountdata = mdata; > > } > > kfree(full_path); > > return rc; > > @@ -2821,6 +2824,7 @@ cifs_mount(struct super_block *sb, struct > > cifs_sb_info *cifs_sb, > > #ifdef CONFIG_CIFS_DFS_UPCALL > > int referral_walks_count = 0; > > try_mount_again: > > + mount_data = cifs_sb->mountdata; > > > > ^^^^ > With this, the mount_data_global parm to cifs_mount is vestigial. I > think you can remove that and just plan to pass in the options via > cifs_sb->mountdata. But if CONFIG_CIFS_DFS_UPCALL isn't enabled you don't have cifs_sb->mountdata in the struct at all, so you'd still need some extra #ifdef logic in the caller, right? I felt the one-liner after try_mount_again was clean enough but can do something else if you prefer. 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? > Reviewed-by: Jeff Layton <[email protected]> Since there isn't clear agreement above I'll wait for clarification. On Fri, Apr 01, 2011 at 09:14:18AM -0400, Jeff Layton wrote: > On Wed, 30 Mar 2011 14:54:27 +0200 > Sean Finney <[email protected]> wrote: <snip> > > - if (!options) > > - return 1; > > + if (!mountdata) > > + goto cifs_parse_mount_err; > > + > > + mountdata_copy = kstrdup(mountdata, GFP_KERNEL); > > + if (!mountdata_copy) > > + goto cifs_parse_mount_err; > > > > I worry (slightly) about bounds checking here. Yes, I know we do a poor > job of that in this code, but this is probably a good time to fix that. > I'm pretty sure that mount options are limited to a page. Maybe turn > the above into a kstrndup() and limit it to PAGE_CACHE_SIZE? > > Might not hurt also to explicitly set the last byte in the allocation > to 0. Will do. <snip> > > if (vol->multiuser && !(vol->secFlg & CIFSSEC_MAY_KRB5)) { > > cERROR(1, "Multiuser mounts currently require krb5 " > > "authentication!"); > > - return 1; > > + goto cifs_parse_mount_err; > > } > > > > if (vol->UNCip == NULL) > > @@ -1455,6 +1461,10 @@ cifs_parse_mount_options(char *options, const char > > *devname, > > "specified with no gid= option.\n"); > > > > return 0; > > ^^^^^^^^^^ > In the event that there's no error, what frees mountdata_copy here? Oops! Good catch. Will fix. sean -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
