On Tue, Jun 19, 2012 at 12:21 PM, Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
> 2012/6/19 Jeff Layton <jlayton@xxxxxxxxxx>:
>> On Sat, 9 Jun 2012 10:26:03 +0400
>> Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
>>
>>> Signed-off-by: Pavel Shilovsky <pshilovsky@xxxxxxxxx>
>>> ---
>>> fs/cifs/cifsglob.h | 5 +++++
>>> fs/cifs/cifsproto.h | 8 ++++----
>>> fs/cifs/connect.c | 16 +++++++++-------
>>> fs/cifs/sess.c | 2 +-
>>> fs/cifs/smb1ops.c | 2 ++
>>> 5 files changed, 21 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>> index 669de88..812f27c 100644
>>> --- a/fs/cifs/cifsglob.h
>>> +++ b/fs/cifs/cifsglob.h
>>> @@ -196,6 +196,11 @@ struct smb_version_operations {
>>> bool (*need_neg)(struct TCP_Server_Info *);
>>> /* negotiate to the server */
>>> int (*negotiate)(const int, struct cifs_ses *);
>>> + /* setup smb sessionn */
>>> + int (*sess_setup)(const int, struct cifs_ses *,
>>> + const struct nls_table *);
>>> + /* close smb session */
>>> + int (*logoff)(const int, struct cifs_ses *);
>>> };
>>>
>>> struct smb_version_values {
>>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>>> index 32b569f..8e6cd38 100644
>>> --- a/fs/cifs/cifsproto.h
>>> +++ b/fs/cifs/cifsproto.h
>>> @@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ ,
>>> extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
>>> struct cifs_ses *ses,
>>> void **request_buf);
>>> -extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
>>> - const struct nls_table *nls_cp);
>>> +extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses,
>>> + const struct nls_table *nls_cp);
>>> extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
>>> extern u64 cifs_UnixTimeToNT(struct timespec);
>>> extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
>>> @@ -179,8 +179,8 @@ void cifs_proc_init(void);
>>> void cifs_proc_clean(void);
>>>
>>> extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses);
>>> -extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
>>> - struct nls_table *nls_info);
>>> +extern int cifs_setup_session(const int xid, struct cifs_ses *ses,
>>> + struct nls_table *nls_info);
>>> extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses);
>>>
>>> extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses,
>>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>>> index 4780342..9679352 100644
>>> --- a/fs/cifs/connect.c
>>> +++ b/fs/cifs/connect.c
>>> @@ -2263,9 +2263,9 @@ cifs_put_smb_ses(struct cifs_ses *ses)
>>> list_del_init(&ses->smb_ses_list);
>>> spin_unlock(&cifs_tcp_ses_lock);
>>>
>>> - if (ses->status == CifsGood) {
>>> + if (ses->status == CifsGood && server->ops->logoff) {
>>> xid = GetXid();
>>> - CIFSSMBLogoff(xid, ses);
>>> + server->ops->logoff(xid, ses);
>>> _FreeXid(xid);
>>> }
>>> sesInfoFree(ses);
>>> @@ -3970,11 +3970,11 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses)
>>> return rc;
>>> }
>>>
>>> -
>>> -int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
>>> - struct nls_table *nls_info)
>>> +int
>>> +cifs_setup_session(const int xid, struct cifs_ses *ses,
>>> + struct nls_table *nls_info)
>>
>
> Thanks for the review.
>
>>
>> While I think this whole "xid" thing is a bogus idea, if we're going to
>> do it, we should at least try to be consistent about the type. _GetXid
>> returns unsigned int but you're turning that into a signed int here.
>>
>> I suggest that we don't do that. Let's declare henceforth that it's
>> unsigned. Any place that treats it as signed is a bug. We probably have
>> a lot of these bugs. I don't expect you to fix them all up, but you
>> should at least try to fix that up in the code that you're touching in
>> this series. We can get the rest later...
>>
>> Sound ok?
>>
>
> cifs_get_smb_ses has
>
> int xid;
>
> and does
>
> xid = GetXid();
>
> that returns 'int' - then it calls cifs_setup_session with this xid.
> It seems no problems with my patch here that converts
> cifs_setup_session to take an int.
>
> But GedXid() is
>
> 42 #define GetXid()>------->------->------->------->------->-------\
> 43 ({>----->------->------->------->------->------->------->-------\
> 44 >-------int __xid = (int)_GetXid();>---->------->------->-------\
> 45 >-------cFYI(1, "CIFS VFS: in %s as Xid: %d with uid: %d",>-----\
> 46 >------- __func__, __xid, current_fsuid());>>------->-------\
> 47 >-------__xid;>->------->------->------->------->------->-------\
> 48 })
>
> so, it assigns _GetXid to 'int' and returns sign int value - this
> confuse me a little. Why don't we return unsigned int here?
uint to int change in GetXid probably was an incorrect fix for some warning
(and probably an old change). Why not simply fix this to
return unsigned int in GetXid to be consistent. As you noted
_GetXid returns unsigned int (and FreeXid wants an
unsigned int). If we do a global fix to change
GetXid to be consistent - we can fix the name at the same
time (to remove camel case) if camel case still bugs people.
Of course - much of the GetXid and cFYI stuff could be removed
if we had dynamic trace points and a howto on how to use them
for cifs - someone started looking at this for various fs three or
four years ago but don't think they finished.
--
Thanks,
Steve
--
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
