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?
--
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
