| Message ID | 1473255952-27579-1-git-send-email-aaptel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks Aurelien. Comments below. On Wed, 2016-09-07 at 15:45 +0200, Aurelien Aptel wrote: > When a prefix path needs to be added, current code hardcodes the > initial > path separator to backslash. Use CIFS_DIR_SEP instead and properly > convert path separator in the prefix path. > > CIFS_MOUNT_USE_PREFIX_PATH is rarely needed (Windows shares with very > specific ACL or buggy mount call AFAIK) which explains why the > backsplash wasn't a big issue. > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > --- > fs/cifs/dir.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 4716c54..c85a8bb 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -160,13 +160,15 @@ cifs_bp_rename_retry: > > if (pplen) { > int i; > + char oldsep = dirsep == '/' ? '\\' : '/'; > > cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", > cifs_sb->prepath); > + > + full_path[dfsplen] = dirsep; > memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen- > 1); > - full_path[dfsplen] = '\\'; > - for (i = 0; i < pplen-1; i++) > - if (full_path[dfsplen+1+i] == '/') > - full_path[dfsplen+1+i] = > CIFS_DIR_SEP(cifs_sb); > + for (i = 1; i < pplen; i++) > + if (full_path[dfsplen+i] == oldsep) > + full_path[dfsplen+i] = dirsep; We have an inline helper convert_delimiter() which already does this. The block for dfsplen also seems to repeat the code. Can we use the helper for the complete patch instead when the full_path variable is available? I think the code here for copying the treename and the prefixpath name can also be cleaned to make it easier to read ie. move the treeName copy before we copy over the prefix path. Sachin Prabhu > } > > if (dfsplen) { -- 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
Sachin Prabhu <sprabhu@redhat.com> writes: > We have an inline helper convert_delimiter() which already does this. > The block for dfsplen also seems to repeat the code. Correct except for a small detail: the inline helper does the conversion until the end of the string. Here we need to stop in the middle. > Can we use the helper for the complete patch instead when the > full_path variable is available? We do not want to mess with unix path that might contain a valid backslash inside a path component. > I think the code here for copying the treename and the prefixpath name > can also be cleaned to make it easier to read ie. move the treeName > copy before we copy over the prefix path. Agreed. Thanks for the feedback, Sachin. I'll send an updated patch for review.
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 4716c54..c85a8bb 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -160,13 +160,15 @@ cifs_bp_rename_retry: if (pplen) { int i; + char oldsep = dirsep == '/' ? '\\' : '/'; cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath); + + full_path[dfsplen] = dirsep; memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1); - full_path[dfsplen] = '\\'; - for (i = 0; i < pplen-1; i++) - if (full_path[dfsplen+1+i] == '/') - full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb); + for (i = 1; i < pplen; i++) + if (full_path[dfsplen+i] == oldsep) + full_path[dfsplen+i] = dirsep; } if (dfsplen) {
When a prefix path needs to be added, current code hardcodes the initial path separator to backslash. Use CIFS_DIR_SEP instead and properly convert path separator in the prefix path. CIFS_MOUNT_USE_PREFIX_PATH is rarely needed (Windows shares with very specific ACL or buggy mount call AFAIK) which explains why the backsplash wasn't a big issue. Signed-off-by: Aurelien Aptel <aaptel@suse.com> --- fs/cifs/dir.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)