| Message ID | 1302100005-1848-5-git-send-email-seanius@seanius.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 6 Apr 2011 16:26:44 +0200 Sean Finney <seanius@seanius.net> wrote: > A relatively minor nit, but also clarified the "consensus" from the > preceding comments that it is in fact better to try for the kstrdup > early and cleanup while cleaning up is still a simple thing to do. > > Reviewed-By: Steve French <smfrench@gmail.com> > Signed-off-by: Sean Finney <seanius@seanius.net> > --- > fs/cifs/cifsfs.c | 18 +++++++----------- > 1 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index fb6a2ad..cf8a610 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data, > cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages; > > #ifdef CONFIG_CIFS_DFS_UPCALL > - /* copy mount params to sb for use in submounts */ > - /* BB: should we move this after the mount so we > - * do not have to do the copy on failed mounts? > - * BB: May be it is better to do simple copy before > - * complex operation (mount), and in case of fail > - * just exit instead of doing mount and attempting > - * undo it if this copy fails?*/ > + /* > + * Copy mount params to sb for use in submounts. Better to do > + * the copy here and deal with the error before cleanup gets > + * complicated post-mount. > + */ > if (data) { > - int len = strlen(data); > - cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL); > + cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE, > + GFP_KERNEL); > if (cifs_sb->mountdata == NULL) { > bdi_destroy(&cifs_sb->bdi); > kfree(sb->s_fs_info); > sb->s_fs_info = NULL; > return -ENOMEM; > } > - strncpy(cifs_sb->mountdata, data, len + 1); > - cifs_sb->mountdata[len] = '\0'; > } > #endif > I was wrong before, and we should be using PAGE_SIZE here instead of PAGE_CACHE_SIZE. On all arches that I know of those are equivalent currently, but that might not be the case in the future. It might not hurt to change that in these patches though if you're respinning them anyway. Either way, you can add my: Reviewed-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 08, 2011 at 09:53:33AM -0400, Jeff Layton wrote: > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index fb6a2ad..cf8a610 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data, > > cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages; > > > > #ifdef CONFIG_CIFS_DFS_UPCALL > > - /* copy mount params to sb for use in submounts */ > > - /* BB: should we move this after the mount so we > > - * do not have to do the copy on failed mounts? > > - * BB: May be it is better to do simple copy before > > - * complex operation (mount), and in case of fail > > - * just exit instead of doing mount and attempting > > - * undo it if this copy fails?*/ > > + /* > > + * Copy mount params to sb for use in submounts. Better to do > > + * the copy here and deal with the error before cleanup gets > > + * complicated post-mount. > > + */ > > if (data) { > > - int len = strlen(data); > > - cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL); > > + cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE, > > + GFP_KERNEL); > > if (cifs_sb->mountdata == NULL) { > > bdi_destroy(&cifs_sb->bdi); > > kfree(sb->s_fs_info); > > sb->s_fs_info = NULL; > > return -ENOMEM; > > } > > - strncpy(cifs_sb->mountdata, data, len + 1); > > - cifs_sb->mountdata[len] = '\0'; > > } > > #endif > > > > I was wrong before, and we should be using PAGE_SIZE here instead of > PAGE_CACHE_SIZE. On all arches that I know of those are equivalent > currently, but that might not be the case in the future. It might not > hurt to change that in these patches though if you're respinning them > anyway. Okay, no big deal to do so, will update it before I re-submit the patches on monday. Thanks for the review! sean -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index fb6a2ad..cf8a610 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data, cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages; #ifdef CONFIG_CIFS_DFS_UPCALL - /* copy mount params to sb for use in submounts */ - /* BB: should we move this after the mount so we - * do not have to do the copy on failed mounts? - * BB: May be it is better to do simple copy before - * complex operation (mount), and in case of fail - * just exit instead of doing mount and attempting - * undo it if this copy fails?*/ + /* + * Copy mount params to sb for use in submounts. Better to do + * the copy here and deal with the error before cleanup gets + * complicated post-mount. + */ if (data) { - int len = strlen(data); - cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL); + cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE, + GFP_KERNEL); if (cifs_sb->mountdata == NULL) { bdi_destroy(&cifs_sb->bdi); kfree(sb->s_fs_info); sb->s_fs_info = NULL; return -ENOMEM; } - strncpy(cifs_sb->mountdata, data, len + 1); - cifs_sb->mountdata[len] = '\0'; } #endif