On Tue, 20 Mar 2012 11:29:45 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 19 марта 2012 г. 23:41 пользователь Jeff Layton <jlayton@xxxxxxxxxx> написал:
> > On Fri, 16 Mar 2012 18:09:31 +0300
> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >
> >> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >> ---
> >> fs/cifs/cifs_debug.c | 5 ++-
> >> fs/cifs/cifs_debug.h | 2 +-
> >> fs/cifs/cifsglob.h | 2 +-
> >> fs/cifs/cifsproto.h | 5 +--
> >> fs/cifs/connect.c | 78 +++++++++++++++++++++++++++-----------------------
> >> fs/cifs/misc.c | 7 +++-
> >> fs/cifs/transport.c | 4 +-
> >> 7 files changed, 56 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> >> index 573b899..bcd0db7 100644
> >> --- a/fs/cifs/cifs_debug.c
> >> +++ b/fs/cifs/cifs_debug.c
> >> @@ -58,15 +58,16 @@ cifs_dump_mem(char *label, void *data, int length)
> >> }
> >>
> >> #ifdef CONFIG_CIFS_DEBUG2
> >> -void cifs_dump_detail(struct smb_hdr *smb)
> >> +void cifs_dump_detail(void *buf)
> >> {
> >> + struct smb_hdr *smb = (struct smb_hdr *)buf;
> >> +
> >> cERROR(1, "Cmd: %d Err: 0x%x Flags: 0x%x Flgs2: 0x%x Mid: %d Pid: %d",
> >> smb->Command, smb->Status.CifsError,
> >> smb->Flags, smb->Flags2, smb->Mid, smb->Pid);
> >> 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/cifs_debug.h b/fs/cifs/cifs_debug.h
> >> index 0a234c1..566e0ae 100644
> >> --- a/fs/cifs/cifs_debug.h
> >> +++ b/fs/cifs/cifs_debug.h
> >> @@ -26,7 +26,7 @@
> >> void cifs_dump_mem(char *label, void *data, int length);
> >> #ifdef CONFIG_CIFS_DEBUG2
> >> #define DBG2 2
> >> -void cifs_dump_detail(struct smb_hdr *);
> >> +void cifs_dump_detail(void *);
> >> void cifs_dump_mids(struct TCP_Server_Info *);
> >> #else
> >> #define DBG2 0
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index c3c7d7c..34a897e 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -730,7 +730,7 @@ struct mid_q_entry {
> >> mid_receive_t *receive; /* call receive callback */
> >> mid_callback_t *callback; /* call completion callback */
> >> void *callback_data; /* general purpose pointer for callback */
> >> - struct smb_hdr *resp_buf; /* pointer to received SMB header */
> >> + void *resp_buf; /* pointer to received SMB header */
> >> int midState; /* wish this were enum but can not pass to wait_event */
> >> __u8 command; /* smb command code */
> >> bool largeBuf:1; /* if valid response, is pointer to large buf */
> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >> index 2d506e3..15c9b59 100644
> >> --- a/fs/cifs/cifsproto.h
> >> +++ b/fs/cifs/cifsproto.h
> >> @@ -91,9 +91,8 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
> >> extern void cifs_add_credits(struct TCP_Server_Info *server,
> >> const unsigned int add);
> >> extern void cifs_set_credits(struct TCP_Server_Info *server, const int val);
> >> -extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length);
> >> -extern bool is_valid_oplock_break(struct smb_hdr *smb,
> >> - struct TCP_Server_Info *);
> >> +extern int checkSMB(char *buf, unsigned int length);
> >> +extern bool is_valid_oplock_break(char *, struct TCP_Server_Info *);
> >> extern bool backup_cred(struct cifs_sb_info *);
> >> extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
> >> extern void cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 4156351..76cffc9 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -183,8 +183,9 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >> -EINVAL = invalid transact2
> >>
> >> */
> >> -static int check2ndT2(struct smb_hdr *pSMB)
> >> +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;
> >> @@ -224,10 +225,10 @@ static int check2ndT2(struct smb_hdr *pSMB)
> >> return remaining;
> >> }
> >>
> >> -static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
> >> +static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
> >> {
> >> - struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)psecond;
> >> - struct smb_t2_rsp *pSMBt = (struct smb_t2_rsp *)pTargetSMB;
> >> + 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;
> >> @@ -280,23 +281,23 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
> >> put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
> >>
> >> /* fix up the BCC */
> >> - byte_count = get_bcc(pTargetSMB);
> >> + 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, pTargetSMB);
> >> + put_bcc(byte_count, target_hdr);
> >>
> >> - byte_count = be32_to_cpu(pTargetSMB->smb_buf_length);
> >> + 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;
> >> }
> >> - pTargetSMB->smb_buf_length = cpu_to_be32(byte_count);
> >> + 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);
> >> @@ -337,6 +338,18 @@ requeue_echo:
> >> queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
> >> }
> >>
> >> +static inline size_t
> >> +header_size(void)
> >> +{
> >> + return sizeof(struct smb_hdr);
> >> +}
> >> +
> >
> > I guess that eventually things like this will have a smb2 variant, and
> > you'll bundle them all up into a set of "transport ops" or something?
>
> Now I modify these functions for SMB2 code to this variant:
>
> static inline size_t
> header_size(server)
> {
> #ifdef CONFIG_CIFS_SMB2
> if (server->is_smb2)
> return sizeof(struct smb2_hdr);
> #endif
> return sizeof(struct smb_hdr);
> }
>
> But having smth like:
>
> struct TCP_Server_Info {
> ...
> struct cifs_transport_ops *t_ops;
> ...
> }
>
> and then using server->t_ops->header_size() looks better.
>
> I also have an idea to have such ops structures for cifsFileInfo and
> cifsInodeInfo pointers. I think this let us make the code cleaner.
> (Now I use ifdefs and callbacks to share the code between cifs and
> smb2). Thoughts?
>
Yes, that would be much cleaner and more efficient too. Just set the
transport ops when the connection is negotiated and then call the
appropriate one in the appropriate situation.
That also makes it easier to deal with protocol changes in the future.
If you need a different operation for 2.0 vs. 2.2 then you can just
create different ops there and call the right one.
If you look at the NFS client code, then you'll see that that's how it
handles the different protocols too -- see struct nfs_rpc_ops. That
model has worked well over the years. We ought to do something similar
here.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
