diff mbox

[linux-cifs-client,08/10] cifs: remove cifs_readlink and CIFSSMBQueryReparseLinkInfo

Message ID 1241011759-7632-9-git-send-email-jlayton@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton April 29, 2009, 1:29 p.m. UTC
CIFSSMBQueryReparseLinkInfo is only called by cifs_readlink, and
cifs_readlink is never called by anything. A quick look at
CIFSSMBQueryReparseLinkInfo shows that it has several problems
with handling of endianness as well.

Remove them.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.h  |    2 -
 fs/cifs/cifspdu.h |   11 -----
 fs/cifs/cifssmb.c |  108 -----------------------------------------------------
 fs/cifs/link.c    |   92 ---------------------------------------------
 4 files changed, 0 insertions(+), 213 deletions(-)

Comments

Steve French April 29, 2009, 2:27 p.m. UTC | #1
On Wed, Apr 29, 2009 at 8:29 AM, Jeff Layton <jlayton@redhat.com> wrote:
> CIFSSMBQueryReparseLinkInfo is only called by cifs_readlink, and
> cifs_readlink is never called by anything. A quick look at
> CIFSSMBQueryReparseLinkInfo shows that it has several problems
> with handling of endianness as well.

What happens when we read a Junction with current code then?

Windows allows users to create a "junction" (see the Windows utilities
"linkd.exe",  "ln.exe" and "fsutil").  Junctions look a little like a
symlink.  Although usually they have absolute paths, I think that they
can be created with relative paths like a symlink.    We probably
ought to make sure we can handle this before we remove this.
Jeff Layton April 29, 2009, 2:35 p.m. UTC | #2
On Wed, 29 Apr 2009 09:27:55 -0500
Steve French <smfrench@gmail.com> wrote:

> On Wed, Apr 29, 2009 at 8:29 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > CIFSSMBQueryReparseLinkInfo is only called by cifs_readlink, and
> > cifs_readlink is never called by anything. A quick look at
> > CIFSSMBQueryReparseLinkInfo shows that it has several problems
> > with handling of endianness as well.
> 
> What happens when we read a Junction with current code then?
> 
> Windows allows users to create a "junction" (see the Windows utilities
> "linkd.exe",  "ln.exe" and "fsutil").  Junctions look a little like a
> symlink.  Although usually they have absolute paths, I think that they
> can be created with relative paths like a symlink.    We probably
> ought to make sure we can handle this before we remove this.
> 
> 

Why? This code is officially unused since there's no way to call into
it. I need to change the calling conventions for the unix symlink
function, and this code also calls that. I don't really think my time
is well spent fixing a dead code path.

If/when you decide to fix the junction parsing code, this patch could
be reverted. Until then, keeping this code around bloats out cifs.ko
and adds to the maintenance burden with no benefit.
Steve French April 29, 2009, 2:40 p.m. UTC | #3
I am just trying to figure out what happens when we hit this - if we
have a bug, lets fix it.  I haven't tried this in a while.

On Wed, Apr 29, 2009 at 9:35 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 29 Apr 2009 09:27:55 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> On Wed, Apr 29, 2009 at 8:29 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > CIFSSMBQueryReparseLinkInfo is only called by cifs_readlink, and
>> > cifs_readlink is never called by anything. A quick look at
>> > CIFSSMBQueryReparseLinkInfo shows that it has several problems
>> > with handling of endianness as well.
>>
>> What happens when we read a Junction with current code then?
>>
>> Windows allows users to create a "junction" (see the Windows utilities
>> "linkd.exe",  "ln.exe" and "fsutil").  Junctions look a little like a
>> symlink.  Although usually they have absolute paths, I think that they
>> can be created with relative paths like a symlink.    We probably
>> ought to make sure we can handle this before we remove this.
>>
>>
>
> Why? This code is officially unused since there's no way to call into
> it. I need to change the calling conventions for the unix symlink
> function, and this code also calls that. I don't really think my time
> is well spent fixing a dead code path.
>
> If/when you decide to fix the junction parsing code, this patch could
> be reverted. Until then, keeping this code around bloats out cifs.ko
> and adds to the maintenance burden with no benefit.
>
> --
> Jeff Layton <jlayton@redhat.com>
>
Jeff Layton April 29, 2009, 2:54 p.m. UTC | #4
On Wed, 29 Apr 2009 09:40:42 -0500
Steve French <smfrench@gmail.com> wrote:

> I am just trying to figure out what happens when we hit this - if we
> have a bug, lets fix it.  I haven't tried this in a while.
> 

Hit it how? This code is dead. It's not reachable from any current
codepath. I'm not even sure how it was intended to be used in the first
place.

If you want to fix it, then that's fine, but I don't really want to
work on it at this point in time. It's also not appropriate to do that
in the context of this patchset which is about fixing the unicode
string handling problems. I suggest that we just remove this code for
now, and then you can put it back later when these functions are fixed,
tested and actually work.
diff mbox

Patch

diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 051b71c..5d04417 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -85,8 +85,6 @@  extern const struct dentry_operations cifs_ci_dentry_ops;
 extern void *cifs_follow_link(struct dentry *direntry, struct nameidata *nd);
 extern void cifs_put_link(struct dentry *direntry,
 			  struct nameidata *nd, void *);
-extern int cifs_readlink(struct dentry *direntry, char __user *buffer,
-			 int buflen);
 extern int cifs_symlink(struct inode *inode, struct dentry *direntry,
 			const char *symname);
 extern int	cifs_removexattr(struct dentry *, const char *);
diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
index a785f69..326bd88 100644
--- a/fs/cifs/cifspdu.h
+++ b/fs/cifs/cifspdu.h
@@ -1432,17 +1432,6 @@  struct file_notify_information {
 	__u8  FileName[0];
 } __attribute__((packed));
 
-struct reparse_data {
-	__u32	ReparseTag;
-	__u16	ReparseDataLength;
-	__u16	Reserved;
-	__u16	AltNameOffset;
-	__u16	AltNameLen;
-	__u16	TargetNameOffset;
-	__u16	TargetNameLen;
-	char	LinkNamesBuf[1];
-} __attribute__((packed));
-
 struct cifs_quota_data {
 	__u32	rsrvd1;  /* 0 */
 	__u32	sid_size;
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 6de17ef..231a564 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2570,115 +2570,7 @@  validate_ntransact(char *buf, char **ppparm, char **ppdata,
 }
 #endif /* CIFS_EXPERIMENTAL */
 
-int
-CIFSSMBQueryReparseLinkInfo(const int xid, struct cifsTconInfo *tcon,
-			const unsigned char *searchName,
-			char *symlinkinfo, const int buflen, __u16 fid,
-			const struct nls_table *nls_codepage)
-{
-	int rc = 0;
-	int bytes_returned;
-	int name_len;
-	struct smb_com_transaction_ioctl_req *pSMB;
-	struct smb_com_transaction_ioctl_rsp *pSMBr;
-
-	cFYI(1, ("In Windows reparse style QueryLink for path %s", searchName));
-	rc = smb_init(SMB_COM_NT_TRANSACT, 23, tcon, (void **) &pSMB,
-		      (void **) &pSMBr);
-	if (rc)
-		return rc;
-
-	pSMB->TotalParameterCount = 0 ;
-	pSMB->TotalDataCount = 0;
-	pSMB->MaxParameterCount = cpu_to_le32(2);
-	/* BB find exact data count max from sess structure BB */
-	pSMB->MaxDataCount = cpu_to_le32((tcon->ses->server->maxBuf -
-					  MAX_CIFS_HDR_SIZE) & 0xFFFFFF00);
-	pSMB->MaxSetupCount = 4;
-	pSMB->Reserved = 0;
-	pSMB->ParameterOffset = 0;
-	pSMB->DataCount = 0;
-	pSMB->DataOffset = 0;
-	pSMB->SetupCount = 4;
-	pSMB->SubCommand = cpu_to_le16(NT_TRANSACT_IOCTL);
-	pSMB->ParameterCount = pSMB->TotalParameterCount;
-	pSMB->FunctionCode = cpu_to_le32(FSCTL_GET_REPARSE_POINT);
-	pSMB->IsFsctl = 1; /* FSCTL */
-	pSMB->IsRootFlag = 0;
-	pSMB->Fid = fid; /* file handle always le */
-	pSMB->ByteCount = 0;
-
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	if (rc) {
-		cFYI(1, ("Send error in QueryReparseLinkInfo = %d", rc));
-	} else {		/* decode response */
-		__u32 data_offset = le32_to_cpu(pSMBr->DataOffset);
-		__u32 data_count = le32_to_cpu(pSMBr->DataCount);
-		if ((pSMBr->ByteCount < 2) || (data_offset > 512))
-		/* BB also check enough total bytes returned */
-			rc = -EIO;	/* bad smb */
-		else {
-			if (data_count && (data_count < 2048)) {
-				char *end_of_smb = 2 /* sizeof byte count */ +
-						pSMBr->ByteCount +
-						(char *)&pSMBr->ByteCount;
-
-				struct reparse_data *reparse_buf =
-						(struct reparse_data *)
-						((char *)&pSMBr->hdr.Protocol
-								 + data_offset);
-				if ((char *)reparse_buf >= end_of_smb) {
-					rc = -EIO;
-					goto qreparse_out;
-				}
-				if ((reparse_buf->LinkNamesBuf +
-					reparse_buf->TargetNameOffset +
-					reparse_buf->TargetNameLen) >
-						end_of_smb) {
-					cFYI(1, ("reparse buf beyond SMB"));
-					rc = -EIO;
-					goto qreparse_out;
-				}
-
-				if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) {
-					name_len = UniStrnlen((wchar_t *)
-						(reparse_buf->LinkNamesBuf +
-						reparse_buf->TargetNameOffset),
-						min(buflen/2,
-						reparse_buf->TargetNameLen / 2));
-					cifs_strfromUCS_le(symlinkinfo,
-						(__le16 *) (reparse_buf->LinkNamesBuf +
-						reparse_buf->TargetNameOffset),
-						name_len, nls_codepage);
-				} else { /* ASCII names */
-					strncpy(symlinkinfo,
-						reparse_buf->LinkNamesBuf +
-						reparse_buf->TargetNameOffset,
-						min_t(const int, buflen,
-						   reparse_buf->TargetNameLen));
-				}
-			} else {
-				rc = -EIO;
-				cFYI(1, ("Invalid return data count on "
-					 "get reparse info ioctl"));
-			}
-			symlinkinfo[buflen] = 0; /* just in case so the caller
-					does not go off the end of the buffer */
-			cFYI(1, ("readlink result - %s", symlinkinfo));
-		}
-	}
-qreparse_out:
-	cifs_buf_release(pSMB);
-
-	/* Note: On -EAGAIN error only caller can retry on handle based calls
-		since file handle passed in no longer valid */
-
-	return rc;
-}
-
 #ifdef CONFIG_CIFS_POSIX
-
 /*Convert an Access Control Entry from wire format to local POSIX xattr format*/
 static void cifs_convert_ace(posix_acl_xattr_entry *ace,
 			     struct cifs_posix_ace *cifs_ace)
diff --git a/fs/cifs/link.c b/fs/cifs/link.c
index 63f6440..9926cf7 100644
--- a/fs/cifs/link.c
+++ b/fs/cifs/link.c
@@ -224,98 +224,6 @@  cifs_symlink(struct inode *inode, struct dentry *direntry, const char *symname)
 	return rc;
 }
 
-int
-cifs_readlink(struct dentry *direntry, char __user *pBuffer, int buflen)
-{
-	struct inode *inode = direntry->d_inode;
-	int rc = -EACCES;
-	int xid;
-	int oplock = 0;
-	struct cifs_sb_info *cifs_sb;
-	struct cifsTconInfo *pTcon;
-	char *full_path = NULL;
-	char *tmpbuffer;
-	int len;
-	__u16 fid;
-
-	xid = GetXid();
-	cifs_sb = CIFS_SB(inode->i_sb);
-	pTcon = cifs_sb->tcon;
-
-/* BB would it be safe against deadlock to grab this sem
-      even though rename itself grabs the sem and calls lookup? */
-/*       mutex_lock(&inode->i_sb->s_vfs_rename_mutex);*/
-	full_path = build_path_from_dentry(direntry);
-/*       mutex_unlock(&inode->i_sb->s_vfs_rename_mutex);*/
-
-	if (full_path == NULL) {
-		FreeXid(xid);
-		return -ENOMEM;
-	}
-
-	cFYI(1,
-	     ("Full path: %s inode = 0x%p pBuffer = 0x%p buflen = %d",
-	      full_path, inode, pBuffer, buflen));
-	if (buflen > PATH_MAX)
-		len = PATH_MAX;
-	else
-		len = buflen;
-	tmpbuffer = kmalloc(len, GFP_KERNEL);
-	if (tmpbuffer == NULL) {
-		kfree(full_path);
-		FreeXid(xid);
-		return -ENOMEM;
-	}
-
-/* BB add read reparse point symlink code and
-	Unix extensions symlink code here BB */
-/* We could disable this based on pTcon->unix_ext flag instead ... but why? */
-	if (cifs_sb->tcon->ses->capabilities & CAP_UNIX)
-		rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path,
-				tmpbuffer,
-				len - 1,
-				cifs_sb->local_nls);
-	else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) {
-		cERROR(1, ("SFU style symlinks not implemented yet"));
-		/* add open and read as in fs/cifs/inode.c */
-	} else {
-		rc = CIFSSMBOpen(xid, pTcon, full_path, FILE_OPEN, GENERIC_READ,
-				OPEN_REPARSE_POINT, &fid, &oplock, NULL,
-				cifs_sb->local_nls,
-				cifs_sb->mnt_cifs_flags &
-					CIFS_MOUNT_MAP_SPECIAL_CHR);
-		if (!rc) {
-			rc = CIFSSMBQueryReparseLinkInfo(xid, pTcon, full_path,
-				tmpbuffer,
-				len - 1,
-				fid,
-				cifs_sb->local_nls);
-			if (CIFSSMBClose(xid, pTcon, fid)) {
-				cFYI(1, ("Error closing junction point "
-					 "(open for ioctl)"));
-			}
-			/* If it is a DFS junction earlier we would have gotten
-			   PATH_NOT_COVERED returned from server so we do
-			   not need to request the DFS info here */
-		}
-	}
-	/* BB Anything else to do to handle recursive links? */
-	/* BB Should we be using page ops here? */
-
-	/* BB null terminate returned string in pBuffer? BB */
-	if (rc == 0) {
-		rc = vfs_readlink(direntry, pBuffer, len, tmpbuffer);
-		cFYI(1,
-		     ("vfs_readlink called from cifs_readlink returned %d",
-		      rc));
-	}
-
-	kfree(tmpbuffer);
-	kfree(full_path);
-	FreeXid(xid);
-	return rc;
-}
-
 void cifs_put_link(struct dentry *direntry, struct nameidata *nd, void *cookie)
 {
 	char *p = nd_get_link(nd);