Hi Jeff,
On Fri, Mar 25, 2011 at 04:51:10PM -0400, Jeff Layton wrote:
> Not that I'm trying to give you more work to do, but I think it would
> have meant less code churn to break the existing code out into a
> separate function first, and then add the new call site to this
> function second. It certainly would make this easier to review...
I thought about that afterwards, actually, but (a) Since I'm not familiar with
the codebase I had to attack the problem from the other direction (I
wasn't sure what to split out without having the duplicate code to look
over) and (b) I was on the bus by the time I thought about that, meaning
it would have to wait until today :)
If it's a big problem let me know and I'll re-re-factor.
> > + /* For DFS paths, skip the first '\' of the UNC */
> > + ref_path = (check_prefix)?(full_path + 1):(volume_info->UNC + 1);
> > +
>
> No need for extra parenthesis here. You should run this through
> scripts/checkpatch.pl and fix the style warnings.
Will do.
> > - if (mount_data != mount_data_global)
> > - kfree(mount_data);
> > -
> > - mount_data = cifs_compose_mount_options(
> > - cifs_sb->mountdata, full_path + 1,
> > - referrals, &fake_devname);
> > -
> > - free_dfs_info_array(referrals, num_referrals);
> > - kfree(fake_devname);
> > - kfree(full_path);
> >
> > - if (IS_ERR(mount_data)) {
> > - rc = PTR_ERR(mount_data);
> > - mount_data = NULL;
> > - goto mount_fail_check;
> > - }
> > + if (mount_data != mount_data_global)
> > + kfree(mount_data);
> >
>
> Problem: suppose mount_data != mount_data_global. You kfree it here but
> the pointer is still the same. Now you call expand_dfs_referral and
> let's suppose that get_dfs_path returns an error. We eventually find
> our way to the bottom of the function and free the pointer again.
>
> This is not your fault of course, but rather due to the fact that this
> mount_data/mount_data_global thing is completely fubar. Since you're in
> here anyway, care to see about cleaning that up? It seems like there
> has to be a less error prone way to handle that stuff.
Yeah I'll take a look. I'll poke you on IRC if I have any questions.
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
