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.
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!
--
Best regards,
Pavel Shilovsky.
--
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
