* [PATCH] CIFS: Simplify cifs_demultiplex_thread
@ 2011-06-09 8:58 Pavel Shilovsky
[not found] ` <1307609933-5632-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Shilovsky @ 2011-06-09 8:58 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
Move several code blocks to separate functions:
1) buffer allocations,
2) rfc1002 header check,
3) read whole smb message from socket,
4) find mid owner.
Signed-off-by: Pavel Shilovsky <piastry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
fs/cifs/cifs_debug.c | 1 -
fs/cifs/connect.c | 440 ++++++++++++++++++++++++++++----------------------
2 files changed, 247 insertions(+), 194 deletions(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 2fe3cf1..c4fc68a 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -66,7 +66,6 @@ void cifs_dump_detail(struct smb_hdr *smb)
cERROR(1, "smb buf %p len %d", smb, smbCalcSize(smb));
}
-
void cifs_dump_mids(struct TCP_Server_Info *server)
{
struct list_head *tmp;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index fb31c2c..23addcb 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -317,24 +317,234 @@ requeue_echo:
queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
}
+static struct mid_q_entry *
+find_cifs_owner(struct TCP_Server_Info *server, struct smb_hdr *buf,
+ int *length, bool isLargeBuf, bool *isMultiRsp, char **pbigbuf)
+{
+ struct list_head *tmp, *tmp2;
+ struct mid_q_entry *mid_entry = NULL;
+
+ spin_lock(&GlobalMid_Lock);
+ list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
+ mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
+
+ if (mid_entry->mid != buf->Mid ||
+ mid_entry->midState != MID_REQUEST_SUBMITTED ||
+ mid_entry->command != buf->Command) {
+ mid_entry = NULL;
+ continue;
+ }
+
+ if (*length == 0 && check2ndT2(buf, server->maxBuf) > 0) {
+ /* We have a multipart transact2 resp */
+ *isMultiRsp = true;
+ if (mid_entry->resp_buf) {
+ /* merge response - fix up 1st*/
+ *length = coalesce_t2(buf, mid_entry->resp_buf);
+ if (*length > 0) {
+ *length = 0;
+ mid_entry->multiRsp = true;
+ break;
+ } else {
+ /* all parts received or
+ * packet is malformed
+ */
+ mid_entry->multiEnd = true;
+ goto multi_t2_fnd;
+ }
+ } else {
+ if (!isLargeBuf) {
+ /*
+ * FIXME: switch to already
+ * allocated largebuf?
+ */
+ cERROR(1, "1st trans2 resp "
+ "needs bigbuf");
+ } else {
+ /* Have first buffer */
+ mid_entry->resp_buf = buf;
+ mid_entry->largeBuf = true;
+ *pbigbuf = NULL;
+ }
+ }
+ break;
+ }
+ mid_entry->resp_buf = buf;
+ mid_entry->largeBuf = isLargeBuf;
+multi_t2_fnd:
+ if (*length == 0)
+ mid_entry->midState = MID_RESPONSE_RECEIVED;
+ else
+ mid_entry->midState = MID_RESPONSE_MALFORMED;
+#ifdef CONFIG_CIFS_STATS2
+ mid_entry->when_received = jiffies;
+#endif
+ list_del_init(&mid_entry->qhead);
+ break;
+ }
+ spin_unlock(&GlobalMid_Lock);
+
+ return mid_entry;
+}
+
+static int
+check_rfc1002_header(struct TCP_Server_Info *server, char *buf, int length,
+ unsigned int pdu_length, struct socket **psocket)
+{
+ char temp = *buf;
+
+ /*
+ * The first byte big endian of the length field,
+ * is actually not part of the length but the type
+ * with the most common, zero, as regular data.
+ */
+ if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
+ return 1;
+ } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
+ cFYI(1, "Good RFC 1002 session rsp");
+ return 1;
+ } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
+ /* we get this from Windows 98 instead of
+ an error on SMB negprot response */
+ cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
+ pdu_length);
+ /* give server a second to clean up */
+ msleep(1000);
+ /* always try 445 first on reconnect since we get NACK
+ * on some if we ever connected to port 139 (the NACK
+ * is since we do not begin with RFC1001 session
+ * initialize frame)
+ */
+ cifs_set_port((struct sockaddr *)
+ &server->dstaddr, CIFS_PORT);
+ cifs_reconnect(server);
+ *psocket = server->ssocket;
+ wake_up(&server->response_q);
+ return 1;
+ } else if (temp != (char) 0) {
+ cERROR(1, "Unknown RFC 1002 frame");
+ cifs_dump_mem(" Received Data: ", buf, length);
+ cifs_reconnect(server);
+ *psocket = server->ssocket;
+ return 1;
+ }
+
+ /* else we have an SMB response */
+ if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
+ (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
+ cERROR(1, "Invalid size SMB length %d pdu_length %d",
+ length, pdu_length+4);
+ cifs_reconnect(server);
+ *psocket = server->ssocket;
+ wake_up(&server->response_q);
+ return 1;
+ }
+
+ return 0;
+}
+
+static int
+allocate_bufs(char **pbigbuf, char **psmallbuf, unsigned int size,
+ bool isLargeBuf)
+{
+ char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
+
+ if (bigbuf == NULL) {
+ bigbuf = (char *)cifs_buf_get();
+ if (!bigbuf) {
+ cERROR(1, "No memory for large SMB response");
+ msleep(3000);
+ /* retry will check if exiting */
+ return 1;
+ }
+ } else if (isLargeBuf) {
+ /* we are reusing a dirty large buf, clear its start */
+ memset(bigbuf, 0, size);
+ }
+
+ if (smallbuf == NULL) {
+ smallbuf = (char *)cifs_small_buf_get();
+ if (!smallbuf) {
+ cERROR(1, "No memory for SMB response");
+ msleep(1000);
+ /* retry will check if exiting */
+ return 1;
+ }
+ /* beginning of smb buffer is cleared in our buf_get */
+ } else /* if existing small buf clear beginning */
+ memset(smallbuf, 0, size);
+
+ *pbigbuf = bigbuf;
+ *psmallbuf = smallbuf;
+
+ return 0;
+}
+
+static int
+read_from_socket(struct TCP_Server_Info *server, unsigned int pdu_length,
+ struct socket **psocket, struct msghdr *smb_msg,
+ struct kvec *iov, unsigned int *ptotal_read)
+{
+ int length, rc = 0;
+ unsigned int total_read;
+
+ for (total_read = 0; total_read < pdu_length;
+ total_read += length) {
+ length = kernel_recvmsg(*psocket, smb_msg, iov, 1,
+ pdu_length - total_read, 0);
+ if (server->tcpStatus == CifsExiting) {
+ /* then will exit */
+ rc = 2;
+ break;
+ } else if (server->tcpStatus == CifsNeedReconnect) {
+ cifs_reconnect(server);
+ *psocket = server->ssocket;
+ /* Reconnect wakes up rspns q */
+ /* Now we will reread sock */
+ rc = 1;
+ break;
+ } else if (length == -ERESTARTSYS ||
+ length == -EAGAIN ||
+ length == -EINTR) {
+ /*
+ * Minimum sleep to prevent looping,
+ * allowing socket to clear and app
+ * threads to set tcpStatus
+ * CifsNeedReconnect if server hung.
+ */
+ usleep_range(1000, 2000);
+ length = 0;
+ continue;
+ } else if (length <= 0) {
+ cERROR(1, "Received no data, expecting %d",
+ pdu_length - total_read);
+ cifs_reconnect(server);
+ *psocket = server->ssocket;
+ rc = 1;
+ break;
+ }
+ }
+
+ *ptotal_read = total_read;
+ return 0;
+}
+
static int
cifs_demultiplex_thread(struct TCP_Server_Info *server)
{
int length;
unsigned int pdu_length, total_read;
+ char *buf = NULL, *smallbuf = NULL, *bigbuf = NULL;
struct smb_hdr *smb_buffer = NULL;
- struct smb_hdr *bigbuf = NULL;
- struct smb_hdr *smallbuf = NULL;
struct msghdr smb_msg;
struct kvec iov;
struct socket *csocket = server->ssocket;
struct list_head *tmp, *tmp2;
struct task_struct *task_to_wake = NULL;
struct mid_q_entry *mid_entry;
- char temp;
bool isLargeBuf = false;
bool isMultiRsp;
- int reconnect;
+ int rc;
current->flags |= PF_MEMALLOC;
cFYI(1, "Demultiplex PID: %d", task_pid_nr(current));
@@ -348,35 +558,17 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
while (server->tcpStatus != CifsExiting) {
if (try_to_freeze())
continue;
- if (bigbuf == NULL) {
- bigbuf = cifs_buf_get();
- if (!bigbuf) {
- cERROR(1, "No memory for large SMB response");
- msleep(3000);
- /* retry will check if exiting */
- continue;
- }
- } else if (isLargeBuf) {
- /* we are reusing a dirty large buf, clear its start */
- memset(bigbuf, 0, sizeof(struct smb_hdr));
- }
- if (smallbuf == NULL) {
- smallbuf = cifs_small_buf_get();
- if (!smallbuf) {
- cERROR(1, "No memory for SMB response");
- msleep(1000);
- /* retry will check if exiting */
- continue;
- }
- /* beginning of smb buffer is cleared in our buf_get */
- } else /* if existing small buf clear beginning */
- memset(smallbuf, 0, sizeof(struct smb_hdr));
+ rc = allocate_bufs(&bigbuf, &smallbuf, sizeof(struct smb_hdr),
+ isLargeBuf);
+ if (rc)
+ continue;
isLargeBuf = false;
isMultiRsp = false;
- smb_buffer = smallbuf;
- iov.iov_base = smb_buffer;
+ smb_buffer = (struct smb_hdr *)smallbuf;
+ buf = smallbuf;
+ iov.iov_base = smallbuf;
iov.iov_len = 4;
smb_msg.msg_control = NULL;
smb_msg.msg_controllen = 0;
@@ -414,8 +606,7 @@ incomplete_rcv:
allowing socket to clear and app threads to set
tcpStatus CifsNeedReconnect if server hung */
if (pdu_length < 4) {
- iov.iov_base = (4 - pdu_length) +
- (char *)smb_buffer;
+ iov.iov_base = (4 - pdu_length) + buf;
iov.iov_len = pdu_length;
smb_msg.msg_control = NULL;
smb_msg.msg_controllen = 0;
@@ -440,114 +631,42 @@ incomplete_rcv:
/* The right amount was read from socket - 4 bytes */
/* so we can now interpret the length field */
- /* the first byte big endian of the length field,
- is actually not part of the length but the type
- with the most common, zero, as regular data */
- temp = *((char *) smb_buffer);
-
- /* Note that FC 1001 length is big endian on the wire,
- but we convert it here so it is always manipulated
- as host byte order */
+ /*
+ * Note that FC 1001 length is big endian on the wire,
+ * but we convert it here so it is always manipulated
+ * as host byte order
+ */
pdu_length = be32_to_cpu(smb_buffer->smb_buf_length);
-
- cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
-
- if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
- continue;
- } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
- cFYI(1, "Good RFC 1002 session rsp");
- continue;
- } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
- /* we get this from Windows 98 instead of
- an error on SMB negprot response */
- cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
- pdu_length);
- /* give server a second to clean up */
- msleep(1000);
- /* always try 445 first on reconnect since we get NACK
- * on some if we ever connected to port 139 (the NACK
- * is since we do not begin with RFC1001 session
- * initialize frame)
- */
- cifs_set_port((struct sockaddr *)
- &server->dstaddr, CIFS_PORT);
- cifs_reconnect(server);
- csocket = server->ssocket;
- wake_up(&server->response_q);
- continue;
- } else if (temp != (char) 0) {
- cERROR(1, "Unknown RFC 1002 frame");
- cifs_dump_mem(" Received Data: ", (char *)smb_buffer,
- length);
- cifs_reconnect(server);
- csocket = server->ssocket;
- continue;
- }
-
- /* else we have an SMB response */
- if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
- (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
- cERROR(1, "Invalid size SMB length %d pdu_length %d",
- length, pdu_length+4);
- cifs_reconnect(server);
- csocket = server->ssocket;
- wake_up(&server->response_q);
+ rc = check_rfc1002_header(server, buf, length, pdu_length,
+ &csocket);
+ if (rc)
continue;
- }
- /* else length ok */
- reconnect = 0;
+ cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
if (pdu_length > MAX_CIFS_SMALL_BUFFER_SIZE - 4) {
isLargeBuf = true;
memcpy(bigbuf, smallbuf, 4);
- smb_buffer = bigbuf;
+ smb_buffer = (struct smb_hdr *)bigbuf;
+ buf = bigbuf;
}
- length = 0;
- iov.iov_base = 4 + (char *)smb_buffer;
+
+ iov.iov_base = 4 + buf;
iov.iov_len = pdu_length;
- for (total_read = 0; total_read < pdu_length;
- total_read += length) {
- length = kernel_recvmsg(csocket, &smb_msg, &iov, 1,
- pdu_length - total_read, 0);
- if (server->tcpStatus == CifsExiting) {
- /* then will exit */
- reconnect = 2;
- break;
- } else if (server->tcpStatus == CifsNeedReconnect) {
- cifs_reconnect(server);
- csocket = server->ssocket;
- /* Reconnect wakes up rspns q */
- /* Now we will reread sock */
- reconnect = 1;
- break;
- } else if (length == -ERESTARTSYS ||
- length == -EAGAIN ||
- length == -EINTR) {
- msleep(1); /* minimum sleep to prevent looping,
- allowing socket to clear and app
- threads to set tcpStatus
- CifsNeedReconnect if server hung*/
- length = 0;
- continue;
- } else if (length <= 0) {
- cERROR(1, "Received no data, expecting %d",
- pdu_length - total_read);
- cifs_reconnect(server);
- csocket = server->ssocket;
- reconnect = 1;
- break;
- }
- }
- if (reconnect == 2)
+ rc = read_from_socket(server, pdu_length, &csocket, &smb_msg,
+ &iov, &total_read);
+ if (rc == 2)
break;
- else if (reconnect == 1)
+ else if (rc == 1)
continue;
total_read += 4; /* account for rfc1002 hdr */
dump_smb(smb_buffer, total_read);
+ server->lstrp = jiffies;
+ length = 0;
+
/*
* We know that we received enough to get to the MID as we
* checked the pdu_length earlier. Now check to see
@@ -559,75 +678,11 @@ incomplete_rcv:
*/
length = checkSMB(smb_buffer, smb_buffer->Mid, total_read);
if (length != 0)
- cifs_dump_mem("Bad SMB: ", smb_buffer,
- min_t(unsigned int, total_read, 48));
-
- mid_entry = NULL;
- server->lstrp = jiffies;
-
- spin_lock(&GlobalMid_Lock);
- list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
- mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
-
- if (mid_entry->mid != smb_buffer->Mid ||
- mid_entry->midState != MID_REQUEST_SUBMITTED ||
- mid_entry->command != smb_buffer->Command) {
- mid_entry = NULL;
- continue;
- }
-
- if (length == 0 &&
- check2ndT2(smb_buffer, server->maxBuf) > 0) {
- /* We have a multipart transact2 resp */
- isMultiRsp = true;
- if (mid_entry->resp_buf) {
- /* merge response - fix up 1st*/
- length = coalesce_t2(smb_buffer,
- mid_entry->resp_buf);
- if (length > 0) {
- length = 0;
- mid_entry->multiRsp = true;
- break;
- } else {
- /* all parts received or
- * packet is malformed
- */
- mid_entry->multiEnd = true;
- goto multi_t2_fnd;
- }
- } else {
- if (!isLargeBuf) {
- /*
- * FIXME: switch to already
- * allocated largebuf?
- */
- cERROR(1, "1st trans2 resp "
- "needs bigbuf");
- } else {
- /* Have first buffer */
- mid_entry->resp_buf =
- smb_buffer;
- mid_entry->largeBuf = true;
- bigbuf = NULL;
- }
- }
- break;
- }
- mid_entry->resp_buf = smb_buffer;
- mid_entry->largeBuf = isLargeBuf;
-multi_t2_fnd:
- if (length == 0)
- mid_entry->midState = MID_RESPONSE_RECEIVED;
- else
- mid_entry->midState = MID_RESPONSE_MALFORMED;
-#ifdef CONFIG_CIFS_STATS2
- mid_entry->when_received = jiffies;
-#endif
- list_del_init(&mid_entry->qhead);
- break;
- }
- spin_unlock(&GlobalMid_Lock);
+ cifs_dump_mem("Bad SMB: ", buf,
+ min_t(unsigned int, total_read, 48));
+ mid_entry = find_cifs_owner(server, smb_buffer, &length,
+ isLargeBuf, &isMultiRsp, &bigbuf);
if (mid_entry != NULL) {
mid_entry->callback(mid_entry);
/* Was previous buf put in mpx struct for multi-rsp? */
@@ -645,13 +700,12 @@ multi_t2_fnd:
!isMultiRsp) {
cERROR(1, "No task to wake, unknown frame received! "
"NumMids %d", atomic_read(&midCount));
- cifs_dump_mem("Received Data is: ", (char *)smb_buffer,
+ cifs_dump_mem("Received Data is: ", buf,
sizeof(struct smb_hdr));
#ifdef CONFIG_CIFS_DEBUG2
cifs_dump_detail(smb_buffer);
cifs_dump_mids(server);
#endif /* CIFS_DEBUG2 */
-
}
} /* end while !EXITING */
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] CIFS: Fix sparse error
[not found] ` <1307609933-5632-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2011-06-09 8:58 ` Pavel Shilovsky
[not found] ` <1307609933-5632-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-06-09 11:54 ` [PATCH] CIFS: Simplify cifs_demultiplex_thread Jeff Layton
1 sibling, 1 reply; 6+ messages in thread
From: Pavel Shilovsky @ 2011-06-09 8:58 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
cifs_sb_master_tlink was declared as inline, but without a definition.
Remove the declaration and move the definition up.
Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
fs/cifs/connect.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 23addcb..76a6670 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2203,7 +2203,10 @@ cifs_put_tlink(struct tcon_link *tlink)
}
static inline struct tcon_link *
-cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
+cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
+{
+ return cifs_sb->master_tlink;
+}
static int
compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
@@ -3538,12 +3541,6 @@ out:
return tcon;
}
-static inline struct tcon_link *
-cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
-{
- return cifs_sb->master_tlink;
-}
-
struct cifs_tcon *
cifs_sb_master_tcon(struct cifs_sb_info *cifs_sb)
{
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] CIFS: Fix sparse error
[not found] ` <1307609933-5632-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2011-06-09 11:11 ` Jeff Layton
2011-06-09 14:13 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2011-06-09 11:11 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Thu, 9 Jun 2011 12:58:53 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> cifs_sb_master_tlink was declared as inline, but without a definition.
> Remove the declaration and move the definition up.
>
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/connect.c | 11 ++++-------
> 1 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 23addcb..76a6670 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2203,7 +2203,10 @@ cifs_put_tlink(struct tcon_link *tlink)
> }
>
> static inline struct tcon_link *
> -cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
> +cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
> +{
> + return cifs_sb->master_tlink;
> +}
>
> static int
> compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
> @@ -3538,12 +3541,6 @@ out:
> return tcon;
> }
>
> -static inline struct tcon_link *
> -cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
> -{
> - return cifs_sb->master_tlink;
> -}
> -
> struct cifs_tcon *
> cifs_sb_master_tcon(struct cifs_sb_info *cifs_sb)
> {
Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CIFS: Simplify cifs_demultiplex_thread
[not found] ` <1307609933-5632-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-06-09 8:58 ` [PATCH] CIFS: Fix sparse error Pavel Shilovsky
@ 2011-06-09 11:54 ` Jeff Layton
[not found] ` <20110609075411.040bf5d1-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2011-06-09 11:54 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Thu, 9 Jun 2011 12:58:52 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Move several code blocks to separate functions:
> 1) buffer allocations,
> 2) rfc1002 header check,
> 3) read whole smb message from socket,
> 4) find mid owner.
>
> Signed-off-by: Pavel Shilovsky <piastry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/cifs_debug.c | 1 -
> fs/cifs/connect.c | 440 ++++++++++++++++++++++++++++----------------------
> 2 files changed, 247 insertions(+), 194 deletions(-)
>
Nice. This is long overdue.
That said, speaking from the experience of having to chase down subtle
bugs in this code before, I think this would be best done as a set of
patches so that we can potentially bisect in case this breaks
something. Maybe break out each function in a separate patch?
The main reason that I've not done this before is that the code doesn't
lend itself well to separate functions. You've "solved" that here with a
lot of pointer passing. That breaks up the code into separate functions
but doesn't really clean up the code. So let's consider this a good
starting point, but more work is really needed here.
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index 2fe3cf1..c4fc68a 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -66,7 +66,6 @@ void cifs_dump_detail(struct smb_hdr *smb)
> cERROR(1, "smb buf %p len %d", smb, smbCalcSize(smb));
> }
>
> -
> void cifs_dump_mids(struct TCP_Server_Info *server)
> {
> struct list_head *tmp;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index fb31c2c..23addcb 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -317,24 +317,234 @@ requeue_echo:
> queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
> }
>
> +static struct mid_q_entry *
> +find_cifs_owner(struct TCP_Server_Info *server, struct smb_hdr *buf,
> + int *length, bool isLargeBuf, bool *isMultiRsp, char **pbigbuf)
This function name seems a little odd. Would find_cifs_mid be better?
Usually when I see a lot of pointer argument passing like this in a
function, that's a red flag to me of a poor design. In this case,
that's not really your fault as this code was already a mess. So, I'm
ok with this in principle as it might make it easier to clean up this
code later.
While we're considering overhauling this code -- perhaps, long-term we
should move this to be a state machine?
> +{
> + struct list_head *tmp, *tmp2;
> + struct mid_q_entry *mid_entry = NULL;
> +
> + spin_lock(&GlobalMid_Lock);
> + list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> + mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> +
> + if (mid_entry->mid != buf->Mid ||
> + mid_entry->midState != MID_REQUEST_SUBMITTED ||
> + mid_entry->command != buf->Command) {
> + mid_entry = NULL;
> + continue;
> + }
> +
> + if (*length == 0 && check2ndT2(buf, server->maxBuf) > 0) {
> + /* We have a multipart transact2 resp */
> + *isMultiRsp = true;
> + if (mid_entry->resp_buf) {
> + /* merge response - fix up 1st*/
> + *length = coalesce_t2(buf, mid_entry->resp_buf);
> + if (*length > 0) {
> + *length = 0;
> + mid_entry->multiRsp = true;
> + break;
> + } else {
> + /* all parts received or
> + * packet is malformed
> + */
> + mid_entry->multiEnd = true;
> + goto multi_t2_fnd;
> + }
> + } else {
> + if (!isLargeBuf) {
> + /*
> + * FIXME: switch to already
> + * allocated largebuf?
> + */
> + cERROR(1, "1st trans2 resp "
> + "needs bigbuf");
> + } else {
> + /* Have first buffer */
> + mid_entry->resp_buf = buf;
> + mid_entry->largeBuf = true;
> + *pbigbuf = NULL;
> + }
> + }
> + break;
> + }
> + mid_entry->resp_buf = buf;
> + mid_entry->largeBuf = isLargeBuf;
> +multi_t2_fnd:
> + if (*length == 0)
> + mid_entry->midState = MID_RESPONSE_RECEIVED;
> + else
> + mid_entry->midState = MID_RESPONSE_MALFORMED;
> +#ifdef CONFIG_CIFS_STATS2
> + mid_entry->when_received = jiffies;
> +#endif
> + list_del_init(&mid_entry->qhead);
> + break;
> + }
> + spin_unlock(&GlobalMid_Lock);
> +
> + return mid_entry;
> +}
> +
> +static int
> +check_rfc1002_header(struct TCP_Server_Info *server, char *buf, int length,
> + unsigned int pdu_length, struct socket **psocket)
I don't think you really need psocket here. That should just be
server->ssocket. For that matter, pdu_length should be readable from
buf, and "length" is always going to be 4 anyway, right?
> +{
> + char temp = *buf;
> +
> + /*
> + * The first byte big endian of the length field,
> + * is actually not part of the length but the type
> + * with the most common, zero, as regular data.
> + */
> + if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
> + return 1;
> + } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
> + cFYI(1, "Good RFC 1002 session rsp");
> + return 1;
> + } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
> + /* we get this from Windows 98 instead of
> + an error on SMB negprot response */
> + cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
> + pdu_length);
> + /* give server a second to clean up */
> + msleep(1000);
> + /* always try 445 first on reconnect since we get NACK
> + * on some if we ever connected to port 139 (the NACK
> + * is since we do not begin with RFC1001 session
> + * initialize frame)
> + */
> + cifs_set_port((struct sockaddr *)
> + &server->dstaddr, CIFS_PORT);
> + cifs_reconnect(server);
> + *psocket = server->ssocket;
> + wake_up(&server->response_q);
> + return 1;
> + } else if (temp != (char) 0) {
> + cERROR(1, "Unknown RFC 1002 frame");
> + cifs_dump_mem(" Received Data: ", buf, length);
> + cifs_reconnect(server);
> + *psocket = server->ssocket;
> + return 1;
> + }
> +
> + /* else we have an SMB response */
> + if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
> + (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
> + cERROR(1, "Invalid size SMB length %d pdu_length %d",
> + length, pdu_length+4);
> + cifs_reconnect(server);
> + *psocket = server->ssocket;
> + wake_up(&server->response_q);
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +allocate_bufs(char **pbigbuf, char **psmallbuf, unsigned int size,
> + bool isLargeBuf)
^^^
nit: This can probably be a bool return.
> +{
> + char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
> +
> + if (bigbuf == NULL) {
> + bigbuf = (char *)cifs_buf_get();
> + if (!bigbuf) {
> + cERROR(1, "No memory for large SMB response");
> + msleep(3000);
> + /* retry will check if exiting */
> + return 1;
> + }
> + } else if (isLargeBuf) {
> + /* we are reusing a dirty large buf, clear its start */
> + memset(bigbuf, 0, size);
> + }
> +
> + if (smallbuf == NULL) {
> + smallbuf = (char *)cifs_small_buf_get();
> + if (!smallbuf) {
> + cERROR(1, "No memory for SMB response");
> + msleep(1000);
> + /* retry will check if exiting */
> + return 1;
> + }
> + /* beginning of smb buffer is cleared in our buf_get */
> + } else /* if existing small buf clear beginning */
> + memset(smallbuf, 0, size);
> +
> + *pbigbuf = bigbuf;
> + *psmallbuf = smallbuf;
> +
> + return 0;
> +}
> +
^^^
The buffer handling could really stand to be redesigned, but splitting
it out into a separate function is a decent start.
> +static int
> +read_from_socket(struct TCP_Server_Info *server, unsigned int pdu_length,
> + struct socket **psocket, struct msghdr *smb_msg,
> + struct kvec *iov, unsigned int *ptotal_read)
> +{
> + int length, rc = 0;
> + unsigned int total_read;
> +
> + for (total_read = 0; total_read < pdu_length;
> + total_read += length) {
> + length = kernel_recvmsg(*psocket, smb_msg, iov, 1,
> + pdu_length - total_read, 0);
> + if (server->tcpStatus == CifsExiting) {
> + /* then will exit */
> + rc = 2;
> + break;
> + } else if (server->tcpStatus == CifsNeedReconnect) {
> + cifs_reconnect(server);
> + *psocket = server->ssocket;
> + /* Reconnect wakes up rspns q */
> + /* Now we will reread sock */
> + rc = 1;
> + break;
> + } else if (length == -ERESTARTSYS ||
> + length == -EAGAIN ||
> + length == -EINTR) {
> + /*
> + * Minimum sleep to prevent looping,
> + * allowing socket to clear and app
> + * threads to set tcpStatus
> + * CifsNeedReconnect if server hung.
> + */
> + usleep_range(1000, 2000);
> + length = 0;
> + continue;
> + } else if (length <= 0) {
> + cERROR(1, "Received no data, expecting %d",
> + pdu_length - total_read);
> + cifs_reconnect(server);
> + *psocket = server->ssocket;
> + rc = 1;
> + break;
> + }
> + }
> +
> + *ptotal_read = total_read;
> + return 0;
> +}
> +
> static int
> cifs_demultiplex_thread(struct TCP_Server_Info *server)
> {
> int length;
> unsigned int pdu_length, total_read;
> + char *buf = NULL, *smallbuf = NULL, *bigbuf = NULL;
> struct smb_hdr *smb_buffer = NULL;
> - struct smb_hdr *bigbuf = NULL;
> - struct smb_hdr *smallbuf = NULL;
> struct msghdr smb_msg;
> struct kvec iov;
> struct socket *csocket = server->ssocket;
> struct list_head *tmp, *tmp2;
> struct task_struct *task_to_wake = NULL;
> struct mid_q_entry *mid_entry;
> - char temp;
> bool isLargeBuf = false;
> bool isMultiRsp;
> - int reconnect;
> + int rc;
>
> current->flags |= PF_MEMALLOC;
> cFYI(1, "Demultiplex PID: %d", task_pid_nr(current));
> @@ -348,35 +558,17 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
> while (server->tcpStatus != CifsExiting) {
> if (try_to_freeze())
> continue;
> - if (bigbuf == NULL) {
> - bigbuf = cifs_buf_get();
> - if (!bigbuf) {
> - cERROR(1, "No memory for large SMB response");
> - msleep(3000);
> - /* retry will check if exiting */
> - continue;
> - }
> - } else if (isLargeBuf) {
> - /* we are reusing a dirty large buf, clear its start */
> - memset(bigbuf, 0, sizeof(struct smb_hdr));
> - }
>
> - if (smallbuf == NULL) {
> - smallbuf = cifs_small_buf_get();
> - if (!smallbuf) {
> - cERROR(1, "No memory for SMB response");
> - msleep(1000);
> - /* retry will check if exiting */
> - continue;
> - }
> - /* beginning of smb buffer is cleared in our buf_get */
> - } else /* if existing small buf clear beginning */
> - memset(smallbuf, 0, sizeof(struct smb_hdr));
> + rc = allocate_bufs(&bigbuf, &smallbuf, sizeof(struct smb_hdr),
> + isLargeBuf);
> + if (rc)
> + continue;
>
> isLargeBuf = false;
> isMultiRsp = false;
> - smb_buffer = smallbuf;
> - iov.iov_base = smb_buffer;
> + smb_buffer = (struct smb_hdr *)smallbuf;
> + buf = smallbuf;
> + iov.iov_base = smallbuf;
> iov.iov_len = 4;
> smb_msg.msg_control = NULL;
> smb_msg.msg_controllen = 0;
> @@ -414,8 +606,7 @@ incomplete_rcv:
> allowing socket to clear and app threads to set
> tcpStatus CifsNeedReconnect if server hung */
> if (pdu_length < 4) {
> - iov.iov_base = (4 - pdu_length) +
> - (char *)smb_buffer;
> + iov.iov_base = (4 - pdu_length) + buf;
> iov.iov_len = pdu_length;
> smb_msg.msg_control = NULL;
> smb_msg.msg_controllen = 0;
Should the above code be using read_from_socket() as
well? It would be nice if we had a single function that
we always called to do reads off the socket and simply
check the return value from that. That may be reasonable
for a later patch.
> @@ -440,114 +631,42 @@ incomplete_rcv:
> /* The right amount was read from socket - 4 bytes */
> /* so we can now interpret the length field */
>
> - /* the first byte big endian of the length field,
> - is actually not part of the length but the type
> - with the most common, zero, as regular data */
> - temp = *((char *) smb_buffer);
> -
> - /* Note that FC 1001 length is big endian on the wire,
> - but we convert it here so it is always manipulated
> - as host byte order */
> + /*
> + * Note that FC 1001 length is big endian on the wire,
^^^
nit: might as well add the missing "R" here
> + * but we convert it here so it is always manipulated
> + * as host byte order
> + */
> pdu_length = be32_to_cpu(smb_buffer->smb_buf_length);
> -
> - cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
> -
> - if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
> - continue;
> - } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
> - cFYI(1, "Good RFC 1002 session rsp");
> - continue;
> - } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
> - /* we get this from Windows 98 instead of
> - an error on SMB negprot response */
> - cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
> - pdu_length);
> - /* give server a second to clean up */
> - msleep(1000);
> - /* always try 445 first on reconnect since we get NACK
> - * on some if we ever connected to port 139 (the NACK
> - * is since we do not begin with RFC1001 session
> - * initialize frame)
> - */
> - cifs_set_port((struct sockaddr *)
> - &server->dstaddr, CIFS_PORT);
> - cifs_reconnect(server);
> - csocket = server->ssocket;
> - wake_up(&server->response_q);
> - continue;
> - } else if (temp != (char) 0) {
> - cERROR(1, "Unknown RFC 1002 frame");
> - cifs_dump_mem(" Received Data: ", (char *)smb_buffer,
> - length);
> - cifs_reconnect(server);
> - csocket = server->ssocket;
> - continue;
> - }
> -
> - /* else we have an SMB response */
> - if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
> - (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
> - cERROR(1, "Invalid size SMB length %d pdu_length %d",
> - length, pdu_length+4);
> - cifs_reconnect(server);
> - csocket = server->ssocket;
> - wake_up(&server->response_q);
> + rc = check_rfc1002_header(server, buf, length, pdu_length,
> + &csocket);
> + if (rc)
> continue;
> - }
>
> - /* else length ok */
> - reconnect = 0;
> + cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
>
> if (pdu_length > MAX_CIFS_SMALL_BUFFER_SIZE - 4) {
> isLargeBuf = true;
> memcpy(bigbuf, smallbuf, 4);
> - smb_buffer = bigbuf;
> + smb_buffer = (struct smb_hdr *)bigbuf;
> + buf = bigbuf;
> }
> - length = 0;
> - iov.iov_base = 4 + (char *)smb_buffer;
> +
> + iov.iov_base = 4 + buf;
> iov.iov_len = pdu_length;
> - for (total_read = 0; total_read < pdu_length;
> - total_read += length) {
> - length = kernel_recvmsg(csocket, &smb_msg, &iov, 1,
> - pdu_length - total_read, 0);
> - if (server->tcpStatus == CifsExiting) {
> - /* then will exit */
> - reconnect = 2;
> - break;
> - } else if (server->tcpStatus == CifsNeedReconnect) {
> - cifs_reconnect(server);
> - csocket = server->ssocket;
> - /* Reconnect wakes up rspns q */
> - /* Now we will reread sock */
> - reconnect = 1;
> - break;
> - } else if (length == -ERESTARTSYS ||
> - length == -EAGAIN ||
> - length == -EINTR) {
> - msleep(1); /* minimum sleep to prevent looping,
> - allowing socket to clear and app
> - threads to set tcpStatus
> - CifsNeedReconnect if server hung*/
> - length = 0;
> - continue;
> - } else if (length <= 0) {
> - cERROR(1, "Received no data, expecting %d",
> - pdu_length - total_read);
> - cifs_reconnect(server);
> - csocket = server->ssocket;
> - reconnect = 1;
> - break;
> - }
> - }
> - if (reconnect == 2)
> + rc = read_from_socket(server, pdu_length, &csocket, &smb_msg,
> + &iov, &total_read);
> + if (rc == 2)
> break;
> - else if (reconnect == 1)
> + else if (rc == 1)
> continue;
>
> total_read += 4; /* account for rfc1002 hdr */
>
> dump_smb(smb_buffer, total_read);
>
> + server->lstrp = jiffies;
> + length = 0;
> +
> /*
> * We know that we received enough to get to the MID as we
> * checked the pdu_length earlier. Now check to see
> @@ -559,75 +678,11 @@ incomplete_rcv:
> */
> length = checkSMB(smb_buffer, smb_buffer->Mid, total_read);
> if (length != 0)
> - cifs_dump_mem("Bad SMB: ", smb_buffer,
> - min_t(unsigned int, total_read, 48));
> -
> - mid_entry = NULL;
> - server->lstrp = jiffies;
> -
> - spin_lock(&GlobalMid_Lock);
> - list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
> - mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
> -
> - if (mid_entry->mid != smb_buffer->Mid ||
> - mid_entry->midState != MID_REQUEST_SUBMITTED ||
> - mid_entry->command != smb_buffer->Command) {
> - mid_entry = NULL;
> - continue;
> - }
> -
> - if (length == 0 &&
> - check2ndT2(smb_buffer, server->maxBuf) > 0) {
> - /* We have a multipart transact2 resp */
> - isMultiRsp = true;
> - if (mid_entry->resp_buf) {
> - /* merge response - fix up 1st*/
> - length = coalesce_t2(smb_buffer,
> - mid_entry->resp_buf);
> - if (length > 0) {
> - length = 0;
> - mid_entry->multiRsp = true;
> - break;
> - } else {
> - /* all parts received or
> - * packet is malformed
> - */
> - mid_entry->multiEnd = true;
> - goto multi_t2_fnd;
> - }
> - } else {
> - if (!isLargeBuf) {
> - /*
> - * FIXME: switch to already
> - * allocated largebuf?
> - */
> - cERROR(1, "1st trans2 resp "
> - "needs bigbuf");
> - } else {
> - /* Have first buffer */
> - mid_entry->resp_buf =
> - smb_buffer;
> - mid_entry->largeBuf = true;
> - bigbuf = NULL;
> - }
> - }
> - break;
> - }
> - mid_entry->resp_buf = smb_buffer;
> - mid_entry->largeBuf = isLargeBuf;
> -multi_t2_fnd:
> - if (length == 0)
> - mid_entry->midState = MID_RESPONSE_RECEIVED;
> - else
> - mid_entry->midState = MID_RESPONSE_MALFORMED;
> -#ifdef CONFIG_CIFS_STATS2
> - mid_entry->when_received = jiffies;
> -#endif
> - list_del_init(&mid_entry->qhead);
> - break;
> - }
> - spin_unlock(&GlobalMid_Lock);
> + cifs_dump_mem("Bad SMB: ", buf,
> + min_t(unsigned int, total_read, 48));
>
> + mid_entry = find_cifs_owner(server, smb_buffer, &length,
> + isLargeBuf, &isMultiRsp, &bigbuf);
> if (mid_entry != NULL) {
> mid_entry->callback(mid_entry);
> /* Was previous buf put in mpx struct for multi-rsp? */
> @@ -645,13 +700,12 @@ multi_t2_fnd:
> !isMultiRsp) {
> cERROR(1, "No task to wake, unknown frame received! "
> "NumMids %d", atomic_read(&midCount));
> - cifs_dump_mem("Received Data is: ", (char *)smb_buffer,
> + cifs_dump_mem("Received Data is: ", buf,
> sizeof(struct smb_hdr));
> #ifdef CONFIG_CIFS_DEBUG2
> cifs_dump_detail(smb_buffer);
> cifs_dump_mids(server);
> #endif /* CIFS_DEBUG2 */
> -
> }
> } /* end while !EXITING */
>
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CIFS: Simplify cifs_demultiplex_thread
[not found] ` <20110609075411.040bf5d1-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2011-06-09 12:10 ` Pavel Shilovsky
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Shilovsky @ 2011-06-09 12:10 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
2011/6/9 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Thu, 9 Jun 2011 12:58:52 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Move several code blocks to separate functions:
>> 1) buffer allocations,
>> 2) rfc1002 header check,
>> 3) read whole smb message from socket,
>> 4) find mid owner.
>>
>> Signed-off-by: Pavel Shilovsky <piastry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> fs/cifs/cifs_debug.c | 1 -
>> fs/cifs/connect.c | 440 ++++++++++++++++++++++++++++----------------------
>> 2 files changed, 247 insertions(+), 194 deletions(-)
>>
>
> Nice. This is long overdue.
>
> That said, speaking from the experience of having to chase down subtle
> bugs in this code before, I think this would be best done as a set of
> patches so that we can potentially bisect in case this breaks
> something. Maybe break out each function in a separate patch?
>
> The main reason that I've not done this before is that the code doesn't
> lend itself well to separate functions. You've "solved" that here with a
> lot of pointer passing. That breaks up the code into separate functions
> but doesn't really clean up the code. So let's consider this a good
> starting point, but more work is really needed here.
>
>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
>> index 2fe3cf1..c4fc68a 100644
>> --- a/fs/cifs/cifs_debug.c
>> +++ b/fs/cifs/cifs_debug.c
>> @@ -66,7 +66,6 @@ void cifs_dump_detail(struct smb_hdr *smb)
>> cERROR(1, "smb buf %p len %d", smb, smbCalcSize(smb));
>> }
>>
>> -
>> void cifs_dump_mids(struct TCP_Server_Info *server)
>> {
>> struct list_head *tmp;
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index fb31c2c..23addcb 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -317,24 +317,234 @@ requeue_echo:
>> queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
>> }
>>
>> +static struct mid_q_entry *
>> +find_cifs_owner(struct TCP_Server_Info *server, struct smb_hdr *buf,
>> + int *length, bool isLargeBuf, bool *isMultiRsp, char **pbigbuf)
>
> This function name seems a little odd. Would find_cifs_mid be better?
>
> Usually when I see a lot of pointer argument passing like this in a
> function, that's a red flag to me of a poor design. In this case,
> that's not really your fault as this code was already a mess. So, I'm
> ok with this in principle as it might make it easier to clean up this
> code later.
>
> While we're considering overhauling this code -- perhaps, long-term we
> should move this to be a state machine?
>
>> +{
>> + struct list_head *tmp, *tmp2;
>> + struct mid_q_entry *mid_entry = NULL;
>> +
>> + spin_lock(&GlobalMid_Lock);
>> + list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>> + mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> +
>> + if (mid_entry->mid != buf->Mid ||
>> + mid_entry->midState != MID_REQUEST_SUBMITTED ||
>> + mid_entry->command != buf->Command) {
>> + mid_entry = NULL;
>> + continue;
>> + }
>> +
>> + if (*length == 0 && check2ndT2(buf, server->maxBuf) > 0) {
>> + /* We have a multipart transact2 resp */
>> + *isMultiRsp = true;
>> + if (mid_entry->resp_buf) {
>> + /* merge response - fix up 1st*/
>> + *length = coalesce_t2(buf, mid_entry->resp_buf);
>> + if (*length > 0) {
>> + *length = 0;
>> + mid_entry->multiRsp = true;
>> + break;
>> + } else {
>> + /* all parts received or
>> + * packet is malformed
>> + */
>> + mid_entry->multiEnd = true;
>> + goto multi_t2_fnd;
>> + }
>> + } else {
>> + if (!isLargeBuf) {
>> + /*
>> + * FIXME: switch to already
>> + * allocated largebuf?
>> + */
>> + cERROR(1, "1st trans2 resp "
>> + "needs bigbuf");
>> + } else {
>> + /* Have first buffer */
>> + mid_entry->resp_buf = buf;
>> + mid_entry->largeBuf = true;
>> + *pbigbuf = NULL;
>> + }
>> + }
>> + break;
>> + }
>> + mid_entry->resp_buf = buf;
>> + mid_entry->largeBuf = isLargeBuf;
>> +multi_t2_fnd:
>> + if (*length == 0)
>> + mid_entry->midState = MID_RESPONSE_RECEIVED;
>> + else
>> + mid_entry->midState = MID_RESPONSE_MALFORMED;
>> +#ifdef CONFIG_CIFS_STATS2
>> + mid_entry->when_received = jiffies;
>> +#endif
>> + list_del_init(&mid_entry->qhead);
>> + break;
>> + }
>> + spin_unlock(&GlobalMid_Lock);
>> +
>> + return mid_entry;
>> +}
>> +
>> +static int
>> +check_rfc1002_header(struct TCP_Server_Info *server, char *buf, int length,
>> + unsigned int pdu_length, struct socket **psocket)
>
> I don't think you really need psocket here. That should just be
> server->ssocket. For that matter, pdu_length should be readable from
> buf, and "length" is always going to be 4 anyway, right?
>
>> +{
>> + char temp = *buf;
>> +
>> + /*
>> + * The first byte big endian of the length field,
>> + * is actually not part of the length but the type
>> + * with the most common, zero, as regular data.
>> + */
>> + if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
>> + return 1;
>> + } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
>> + cFYI(1, "Good RFC 1002 session rsp");
>> + return 1;
>> + } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
>> + /* we get this from Windows 98 instead of
>> + an error on SMB negprot response */
>> + cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
>> + pdu_length);
>> + /* give server a second to clean up */
>> + msleep(1000);
>> + /* always try 445 first on reconnect since we get NACK
>> + * on some if we ever connected to port 139 (the NACK
>> + * is since we do not begin with RFC1001 session
>> + * initialize frame)
>> + */
>> + cifs_set_port((struct sockaddr *)
>> + &server->dstaddr, CIFS_PORT);
>> + cifs_reconnect(server);
>> + *psocket = server->ssocket;
>> + wake_up(&server->response_q);
>> + return 1;
>> + } else if (temp != (char) 0) {
>> + cERROR(1, "Unknown RFC 1002 frame");
>> + cifs_dump_mem(" Received Data: ", buf, length);
>> + cifs_reconnect(server);
>> + *psocket = server->ssocket;
>> + return 1;
>> + }
>> +
>> + /* else we have an SMB response */
>> + if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
>> + (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
>> + cERROR(1, "Invalid size SMB length %d pdu_length %d",
>> + length, pdu_length+4);
>> + cifs_reconnect(server);
>> + *psocket = server->ssocket;
>> + wake_up(&server->response_q);
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +allocate_bufs(char **pbigbuf, char **psmallbuf, unsigned int size,
>> + bool isLargeBuf)
> ^^^
> nit: This can probably be a bool return.
>
>> +{
>> + char *bigbuf = *pbigbuf, *smallbuf = *psmallbuf;
>> +
>> + if (bigbuf == NULL) {
>> + bigbuf = (char *)cifs_buf_get();
>> + if (!bigbuf) {
>> + cERROR(1, "No memory for large SMB response");
>> + msleep(3000);
>> + /* retry will check if exiting */
>> + return 1;
>> + }
>> + } else if (isLargeBuf) {
>> + /* we are reusing a dirty large buf, clear its start */
>> + memset(bigbuf, 0, size);
>> + }
>> +
>> + if (smallbuf == NULL) {
>> + smallbuf = (char *)cifs_small_buf_get();
>> + if (!smallbuf) {
>> + cERROR(1, "No memory for SMB response");
>> + msleep(1000);
>> + /* retry will check if exiting */
>> + return 1;
>> + }
>> + /* beginning of smb buffer is cleared in our buf_get */
>> + } else /* if existing small buf clear beginning */
>> + memset(smallbuf, 0, size);
>> +
>> + *pbigbuf = bigbuf;
>> + *psmallbuf = smallbuf;
>> +
>> + return 0;
>> +}
>> +
> ^^^
> The buffer handling could really stand to be redesigned, but splitting
> it out into a separate function is a decent start.
>
>> +static int
>> +read_from_socket(struct TCP_Server_Info *server, unsigned int pdu_length,
>> + struct socket **psocket, struct msghdr *smb_msg,
>> + struct kvec *iov, unsigned int *ptotal_read)
>> +{
>> + int length, rc = 0;
>> + unsigned int total_read;
>> +
>> + for (total_read = 0; total_read < pdu_length;
>> + total_read += length) {
>> + length = kernel_recvmsg(*psocket, smb_msg, iov, 1,
>> + pdu_length - total_read, 0);
>> + if (server->tcpStatus == CifsExiting) {
>> + /* then will exit */
>> + rc = 2;
>> + break;
>> + } else if (server->tcpStatus == CifsNeedReconnect) {
>> + cifs_reconnect(server);
>> + *psocket = server->ssocket;
>> + /* Reconnect wakes up rspns q */
>> + /* Now we will reread sock */
>> + rc = 1;
>> + break;
>> + } else if (length == -ERESTARTSYS ||
>> + length == -EAGAIN ||
>> + length == -EINTR) {
>> + /*
>> + * Minimum sleep to prevent looping,
>> + * allowing socket to clear and app
>> + * threads to set tcpStatus
>> + * CifsNeedReconnect if server hung.
>> + */
>> + usleep_range(1000, 2000);
>> + length = 0;
>> + continue;
>> + } else if (length <= 0) {
>> + cERROR(1, "Received no data, expecting %d",
>> + pdu_length - total_read);
>> + cifs_reconnect(server);
>> + *psocket = server->ssocket;
>> + rc = 1;
>> + break;
>> + }
>> + }
>> +
>> + *ptotal_read = total_read;
>> + return 0;
>> +}
>> +
>> static int
>> cifs_demultiplex_thread(struct TCP_Server_Info *server)
>> {
>> int length;
>> unsigned int pdu_length, total_read;
>> + char *buf = NULL, *smallbuf = NULL, *bigbuf = NULL;
>> struct smb_hdr *smb_buffer = NULL;
>> - struct smb_hdr *bigbuf = NULL;
>> - struct smb_hdr *smallbuf = NULL;
>> struct msghdr smb_msg;
>> struct kvec iov;
>> struct socket *csocket = server->ssocket;
>> struct list_head *tmp, *tmp2;
>> struct task_struct *task_to_wake = NULL;
>> struct mid_q_entry *mid_entry;
>> - char temp;
>> bool isLargeBuf = false;
>> bool isMultiRsp;
>> - int reconnect;
>> + int rc;
>>
>> current->flags |= PF_MEMALLOC;
>> cFYI(1, "Demultiplex PID: %d", task_pid_nr(current));
>> @@ -348,35 +558,17 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>> while (server->tcpStatus != CifsExiting) {
>> if (try_to_freeze())
>> continue;
>> - if (bigbuf == NULL) {
>> - bigbuf = cifs_buf_get();
>> - if (!bigbuf) {
>> - cERROR(1, "No memory for large SMB response");
>> - msleep(3000);
>> - /* retry will check if exiting */
>> - continue;
>> - }
>> - } else if (isLargeBuf) {
>> - /* we are reusing a dirty large buf, clear its start */
>> - memset(bigbuf, 0, sizeof(struct smb_hdr));
>> - }
>>
>> - if (smallbuf == NULL) {
>> - smallbuf = cifs_small_buf_get();
>> - if (!smallbuf) {
>> - cERROR(1, "No memory for SMB response");
>> - msleep(1000);
>> - /* retry will check if exiting */
>> - continue;
>> - }
>> - /* beginning of smb buffer is cleared in our buf_get */
>> - } else /* if existing small buf clear beginning */
>> - memset(smallbuf, 0, sizeof(struct smb_hdr));
>> + rc = allocate_bufs(&bigbuf, &smallbuf, sizeof(struct smb_hdr),
>> + isLargeBuf);
>> + if (rc)
>> + continue;
>>
>> isLargeBuf = false;
>> isMultiRsp = false;
>> - smb_buffer = smallbuf;
>> - iov.iov_base = smb_buffer;
>> + smb_buffer = (struct smb_hdr *)smallbuf;
>> + buf = smallbuf;
>> + iov.iov_base = smallbuf;
>> iov.iov_len = 4;
>> smb_msg.msg_control = NULL;
>> smb_msg.msg_controllen = 0;
>> @@ -414,8 +606,7 @@ incomplete_rcv:
>> allowing socket to clear and app threads to set
>> tcpStatus CifsNeedReconnect if server hung */
>> if (pdu_length < 4) {
>> - iov.iov_base = (4 - pdu_length) +
>> - (char *)smb_buffer;
>> + iov.iov_base = (4 - pdu_length) + buf;
>> iov.iov_len = pdu_length;
>> smb_msg.msg_control = NULL;
>> smb_msg.msg_controllen = 0;
>
> Should the above code be using read_from_socket() as
> well? It would be nice if we had a single function that
> we always called to do reads off the socket and simply
> check the return value from that. That may be reasonable
> for a later patch.
>
>> @@ -440,114 +631,42 @@ incomplete_rcv:
>> /* The right amount was read from socket - 4 bytes */
>> /* so we can now interpret the length field */
>>
>> - /* the first byte big endian of the length field,
>> - is actually not part of the length but the type
>> - with the most common, zero, as regular data */
>> - temp = *((char *) smb_buffer);
>> -
>> - /* Note that FC 1001 length is big endian on the wire,
>> - but we convert it here so it is always manipulated
>> - as host byte order */
>> + /*
>> + * Note that FC 1001 length is big endian on the wire,
> ^^^
> nit: might as well add the missing "R" here
>> + * but we convert it here so it is always manipulated
>> + * as host byte order
>> + */
>> pdu_length = be32_to_cpu(smb_buffer->smb_buf_length);
>> -
>> - cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
>> -
>> - if (temp == (char) RFC1002_SESSION_KEEP_ALIVE) {
>> - continue;
>> - } else if (temp == (char)RFC1002_POSITIVE_SESSION_RESPONSE) {
>> - cFYI(1, "Good RFC 1002 session rsp");
>> - continue;
>> - } else if (temp == (char)RFC1002_NEGATIVE_SESSION_RESPONSE) {
>> - /* we get this from Windows 98 instead of
>> - an error on SMB negprot response */
>> - cFYI(1, "Negative RFC1002 Session Response Error 0x%x)",
>> - pdu_length);
>> - /* give server a second to clean up */
>> - msleep(1000);
>> - /* always try 445 first on reconnect since we get NACK
>> - * on some if we ever connected to port 139 (the NACK
>> - * is since we do not begin with RFC1001 session
>> - * initialize frame)
>> - */
>> - cifs_set_port((struct sockaddr *)
>> - &server->dstaddr, CIFS_PORT);
>> - cifs_reconnect(server);
>> - csocket = server->ssocket;
>> - wake_up(&server->response_q);
>> - continue;
>> - } else if (temp != (char) 0) {
>> - cERROR(1, "Unknown RFC 1002 frame");
>> - cifs_dump_mem(" Received Data: ", (char *)smb_buffer,
>> - length);
>> - cifs_reconnect(server);
>> - csocket = server->ssocket;
>> - continue;
>> - }
>> -
>> - /* else we have an SMB response */
>> - if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
>> - (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
>> - cERROR(1, "Invalid size SMB length %d pdu_length %d",
>> - length, pdu_length+4);
>> - cifs_reconnect(server);
>> - csocket = server->ssocket;
>> - wake_up(&server->response_q);
>> + rc = check_rfc1002_header(server, buf, length, pdu_length,
>> + &csocket);
>> + if (rc)
>> continue;
>> - }
>>
>> - /* else length ok */
>> - reconnect = 0;
>> + cFYI(1, "rfc1002 length 0x%x", pdu_length+4);
>>
>> if (pdu_length > MAX_CIFS_SMALL_BUFFER_SIZE - 4) {
>> isLargeBuf = true;
>> memcpy(bigbuf, smallbuf, 4);
>> - smb_buffer = bigbuf;
>> + smb_buffer = (struct smb_hdr *)bigbuf;
>> + buf = bigbuf;
>> }
>> - length = 0;
>> - iov.iov_base = 4 + (char *)smb_buffer;
>> +
>> + iov.iov_base = 4 + buf;
>> iov.iov_len = pdu_length;
>> - for (total_read = 0; total_read < pdu_length;
>> - total_read += length) {
>> - length = kernel_recvmsg(csocket, &smb_msg, &iov, 1,
>> - pdu_length - total_read, 0);
>> - if (server->tcpStatus == CifsExiting) {
>> - /* then will exit */
>> - reconnect = 2;
>> - break;
>> - } else if (server->tcpStatus == CifsNeedReconnect) {
>> - cifs_reconnect(server);
>> - csocket = server->ssocket;
>> - /* Reconnect wakes up rspns q */
>> - /* Now we will reread sock */
>> - reconnect = 1;
>> - break;
>> - } else if (length == -ERESTARTSYS ||
>> - length == -EAGAIN ||
>> - length == -EINTR) {
>> - msleep(1); /* minimum sleep to prevent looping,
>> - allowing socket to clear and app
>> - threads to set tcpStatus
>> - CifsNeedReconnect if server hung*/
>> - length = 0;
>> - continue;
>> - } else if (length <= 0) {
>> - cERROR(1, "Received no data, expecting %d",
>> - pdu_length - total_read);
>> - cifs_reconnect(server);
>> - csocket = server->ssocket;
>> - reconnect = 1;
>> - break;
>> - }
>> - }
>> - if (reconnect == 2)
>> + rc = read_from_socket(server, pdu_length, &csocket, &smb_msg,
>> + &iov, &total_read);
>> + if (rc == 2)
>> break;
>> - else if (reconnect == 1)
>> + else if (rc == 1)
>> continue;
>>
>> total_read += 4; /* account for rfc1002 hdr */
>>
>> dump_smb(smb_buffer, total_read);
>>
>> + server->lstrp = jiffies;
>> + length = 0;
>> +
>> /*
>> * We know that we received enough to get to the MID as we
>> * checked the pdu_length earlier. Now check to see
>> @@ -559,75 +678,11 @@ incomplete_rcv:
>> */
>> length = checkSMB(smb_buffer, smb_buffer->Mid, total_read);
>> if (length != 0)
>> - cifs_dump_mem("Bad SMB: ", smb_buffer,
>> - min_t(unsigned int, total_read, 48));
>> -
>> - mid_entry = NULL;
>> - server->lstrp = jiffies;
>> -
>> - spin_lock(&GlobalMid_Lock);
>> - list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
>> - mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
>> -
>> - if (mid_entry->mid != smb_buffer->Mid ||
>> - mid_entry->midState != MID_REQUEST_SUBMITTED ||
>> - mid_entry->command != smb_buffer->Command) {
>> - mid_entry = NULL;
>> - continue;
>> - }
>> -
>> - if (length == 0 &&
>> - check2ndT2(smb_buffer, server->maxBuf) > 0) {
>> - /* We have a multipart transact2 resp */
>> - isMultiRsp = true;
>> - if (mid_entry->resp_buf) {
>> - /* merge response - fix up 1st*/
>> - length = coalesce_t2(smb_buffer,
>> - mid_entry->resp_buf);
>> - if (length > 0) {
>> - length = 0;
>> - mid_entry->multiRsp = true;
>> - break;
>> - } else {
>> - /* all parts received or
>> - * packet is malformed
>> - */
>> - mid_entry->multiEnd = true;
>> - goto multi_t2_fnd;
>> - }
>> - } else {
>> - if (!isLargeBuf) {
>> - /*
>> - * FIXME: switch to already
>> - * allocated largebuf?
>> - */
>> - cERROR(1, "1st trans2 resp "
>> - "needs bigbuf");
>> - } else {
>> - /* Have first buffer */
>> - mid_entry->resp_buf =
>> - smb_buffer;
>> - mid_entry->largeBuf = true;
>> - bigbuf = NULL;
>> - }
>> - }
>> - break;
>> - }
>> - mid_entry->resp_buf = smb_buffer;
>> - mid_entry->largeBuf = isLargeBuf;
>> -multi_t2_fnd:
>> - if (length == 0)
>> - mid_entry->midState = MID_RESPONSE_RECEIVED;
>> - else
>> - mid_entry->midState = MID_RESPONSE_MALFORMED;
>> -#ifdef CONFIG_CIFS_STATS2
>> - mid_entry->when_received = jiffies;
>> -#endif
>> - list_del_init(&mid_entry->qhead);
>> - break;
>> - }
>> - spin_unlock(&GlobalMid_Lock);
>> + cifs_dump_mem("Bad SMB: ", buf,
>> + min_t(unsigned int, total_read, 48));
>>
>> + mid_entry = find_cifs_owner(server, smb_buffer, &length,
>> + isLargeBuf, &isMultiRsp, &bigbuf);
>> if (mid_entry != NULL) {
>> mid_entry->callback(mid_entry);
>> /* Was previous buf put in mpx struct for multi-rsp? */
>> @@ -645,13 +700,12 @@ multi_t2_fnd:
>> !isMultiRsp) {
>> cERROR(1, "No task to wake, unknown frame received! "
>> "NumMids %d", atomic_read(&midCount));
>> - cifs_dump_mem("Received Data is: ", (char *)smb_buffer,
>> + cifs_dump_mem("Received Data is: ", buf,
>> sizeof(struct smb_hdr));
>> #ifdef CONFIG_CIFS_DEBUG2
>> cifs_dump_detail(smb_buffer);
>> cifs_dump_mids(server);
>> #endif /* CIFS_DEBUG2 */
>> -
>> }
>> } /* end while !EXITING */
>>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
Thanks for the review!
I agree with your points - so, I am going to split it into several
patches, rework places you mentioned and post whole series soon.
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CIFS: Fix sparse error
[not found] ` <1307609933-5632-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-06-09 11:11 ` Jeff Layton
@ 2011-06-09 14:13 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Steve French @ 2011-06-09 14:13 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
merged
On Thu, Jun 9, 2011 at 3:58 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> cifs_sb_master_tlink was declared as inline, but without a definition.
> Remove the declaration and move the definition up.
>
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/connect.c | 11 ++++-------
> 1 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 23addcb..76a6670 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2203,7 +2203,10 @@ cifs_put_tlink(struct tcon_link *tlink)
> }
>
> static inline struct tcon_link *
> -cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb);
> +cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
> +{
> + return cifs_sb->master_tlink;
> +}
>
> static int
> compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
> @@ -3538,12 +3541,6 @@ out:
> return tcon;
> }
>
> -static inline struct tcon_link *
> -cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
> -{
> - return cifs_sb->master_tlink;
> -}
> -
> struct cifs_tcon *
> cifs_sb_master_tcon(struct cifs_sb_info *cifs_sb)
> {
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-06-09 14:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-09 8:58 [PATCH] CIFS: Simplify cifs_demultiplex_thread Pavel Shilovsky
[not found] ` <1307609933-5632-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-06-09 8:58 ` [PATCH] CIFS: Fix sparse error Pavel Shilovsky
[not found] ` <1307609933-5632-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-06-09 11:11 ` Jeff Layton
2011-06-09 14:13 ` Steve French
2011-06-09 11:54 ` [PATCH] CIFS: Simplify cifs_demultiplex_thread Jeff Layton
[not found] ` <20110609075411.040bf5d1-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2011-06-09 12:10 ` Pavel Shilovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.