On Wed, 30 May 2012 20:38:15 +0400
Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
> 2012/5/30 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>:
> > On Tue, 29 May 2012 20:19:00 +0400
> > Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
> >
> >> Signed-off-by: Pavel Shilovsky <pshilovsky@xxxxxxxxx>
> >> ---
> >> fs/cifs/cifsglob.h | 3 +
> >> fs/cifs/connect.c | 160 +------------------------------------------------
> >> fs/cifs/smb1ops.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 174 insertions(+), 159 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index a660577..3b8828b 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -187,6 +187,9 @@ struct smb_version_operations {
> >> /* verify the message */
> >> int (*check_message)(char *, unsigned int);
> >> bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
> >> + /* process transaction2 response */
> >> + bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
> >> + char *, int);
> >> };
> >>
> >> struct smb_version_values {
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 9362f90..dc96835 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >> return rc;
> >> }
> >>
> >> -/*
> >> - return codes:
> >> - 0 not a transact2, or all data present
> >> - >0 transact2 with that much data missing
> >> - -EINVAL = invalid transact2
> >> -
> >> - */
> >> -static int check2ndT2(char *buf)
> >> -{
> >> - struct smb_hdr *pSMB = (struct smb_hdr *)buf;
> >> - struct smb_t2_rsp *pSMBt;
> >> - int remaining;
> >> - __u16 total_data_size, data_in_this_rsp;
> >> -
> >> - if (pSMB->Command != SMB_COM_TRANSACTION2)
> >> - return 0;
> >> -
> >> - /* check for plausible wct, bcc and t2 data and parm sizes */
> >> - /* check for parm and data offset going beyond end of smb */
> >> - if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
> >> - cFYI(1, "invalid transact2 word count");
> >> - return -EINVAL;
> >> - }
> >> -
> >> - pSMBt = (struct smb_t2_rsp *)pSMB;
> >> -
> >> - total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> >> - data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> >> -
> >> - if (total_data_size == data_in_this_rsp)
> >> - return 0;
> >> - else if (total_data_size < data_in_this_rsp) {
> >> - cFYI(1, "total data %d smaller than data in frame %d",
> >> - total_data_size, data_in_this_rsp);
> >> - return -EINVAL;
> >> - }
> >> -
> >> - remaining = total_data_size - data_in_this_rsp;
> >> -
> >> - cFYI(1, "missing %d bytes from transact2, check next response",
> >> - remaining);
> >> - if (total_data_size > CIFSMaxBufSize) {
> >> - cERROR(1, "TotalDataSize %d is over maximum buffer %d",
> >> - total_data_size, CIFSMaxBufSize);
> >> - return -EINVAL;
> >> - }
> >> - return remaining;
> >> -}
> >> -
> >> -static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
> >> -{
> >> - struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
> >> - struct smb_t2_rsp *pSMBt = (struct smb_t2_rsp *)target_hdr;
> >> - char *data_area_of_tgt;
> >> - char *data_area_of_src;
> >> - int remaining;
> >> - unsigned int byte_count, total_in_tgt;
> >> - __u16 tgt_total_cnt, src_total_cnt, total_in_src;
> >> -
> >> - src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
> >> - tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> >> -
> >> - if (tgt_total_cnt != src_total_cnt)
> >> - cFYI(1, "total data count of primary and secondary t2 differ "
> >> - "source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
> >> -
> >> - total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> >> -
> >> - remaining = tgt_total_cnt - total_in_tgt;
> >> -
> >> - if (remaining < 0) {
> >> - cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
> >> - "total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
> >> - return -EPROTO;
> >> - }
> >> -
> >> - if (remaining == 0) {
> >> - /* nothing to do, ignore */
> >> - cFYI(1, "no more data remains");
> >> - return 0;
> >> - }
> >> -
> >> - total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
> >> - if (remaining < total_in_src)
> >> - cFYI(1, "transact2 2nd response contains too much data");
> >> -
> >> - /* find end of first SMB data area */
> >> - data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
> >> - get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
> >> -
> >> - /* validate target area */
> >> - data_area_of_src = (char *)&pSMBs->hdr.Protocol +
> >> - get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
> >> -
> >> - data_area_of_tgt += total_in_tgt;
> >> -
> >> - total_in_tgt += total_in_src;
> >> - /* is the result too big for the field? */
> >> - if (total_in_tgt > USHRT_MAX) {
> >> - cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
> >> - return -EPROTO;
> >> - }
> >> - put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
> >> -
> >> - /* fix up the BCC */
> >> - byte_count = get_bcc(target_hdr);
> >> - byte_count += total_in_src;
> >> - /* is the result too big for the field? */
> >> - if (byte_count > USHRT_MAX) {
> >> - cFYI(1, "coalesced BCC too large (%u)", byte_count);
> >> - return -EPROTO;
> >> - }
> >> - put_bcc(byte_count, target_hdr);
> >> -
> >> - byte_count = be32_to_cpu(target_hdr->smb_buf_length);
> >> - byte_count += total_in_src;
> >> - /* don't allow buffer to overflow */
> >> - if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
> >> - cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
> >> - return -ENOBUFS;
> >> - }
> >> - target_hdr->smb_buf_length = cpu_to_be32(byte_count);
> >> -
> >> - /* copy second buffer into end of first buffer */
> >> - memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
> >> -
> >> - if (remaining != total_in_src) {
> >> - /* more responses to go */
> >> - cFYI(1, "waiting for more secondary responses");
> >> - return 1;
> >> - }
> >> -
> >> - /* we are done */
> >> - cFYI(1, "found the last secondary response");
> >> - return 0;
> >> -}
> >> -
> >> static void
> >> cifs_echo_request(struct work_struct *work)
> >> {
> >> @@ -803,29 +666,8 @@ static void
> >> handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> >> char *buf, int malformed)
> >> {
> >> - if (malformed == 0 && check2ndT2(buf) > 0) {
> >> - mid->multiRsp = true;
> >> - if (mid->resp_buf) {
> >> - /* merge response - fix up 1st*/
> >> - malformed = coalesce_t2(buf, mid->resp_buf);
> >> - if (malformed > 0)
> >> - return;
> >> -
> >> - /* All parts received or packet is malformed. */
> >> - mid->multiEnd = true;
> >> - return dequeue_mid(mid, malformed);
> >> - }
> >> - if (!server->large_buf) {
> >> - /*FIXME: switch to already allocated largebuf?*/
> >> - cERROR(1, "1st trans2 resp needs bigbuf");
> >> - } else {
> >> - /* Have first buffer */
> >> - mid->resp_buf = buf;
> >> - mid->large_buf = true;
> >> - server->bigbuf = NULL;
> >> - }
> >> + if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
> >
> > Blech, do you really think this makes the code clearer? I personally
> > find this macro ugly, but I suppose it's a matter of taste...
> >
> > Looks fine otherwise, and I'm not concerned enough about the ugliness
> > to NAK it on those grounds...
> >
> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
>
> Thanks for the review!
>
> As for the macro itself. Instead of this:
>
> if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
> return;
>
> we will have:
>
> if (server->ops->check_trans2 && server->ops->check_trans2(mid,
> server, buf, malformed))
> return;
>
> that looks bigger.
>
Sure, it's more verbose, but at least it's clear where each of the
arguments go...
> And for non-void functions:
>
> rc = server->ops->negotiate? server->ops->negotiate(server, xid, ses))
> : -ENOSYS;
>
> the macro reduces this place to
>
> rc = S_OP(server, negotiate, -ENOSYS, xid, ses);
>
> that also looks more compact.
>
> I am not sure that the name of the macro is ideal - suggestions are welcome!
>
The name doesn't much matter to me, but clarity does...
Regardless though, since you're doing the work here I'm inclined to let
you decide on this point. If you find that clearer, then so be it. I
can live with it as long as it ultimately works...
--
Jeff Layton <jlayton@xxxxxxxxx>
--
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
