On Fri, 24 May 2013 16:15:32 +0400
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2013/5/23 Jeff Layton <jlayton@xxxxxxxxxx>:
> > These look pretty cargo-culty to me, but let's be certain. Leave
> > them in place for now. Pop a WARN if it ever does happen. Also,
> > move to a more standard idiom for setting the "server" pointer.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/cifs/cifssmb.c | 11 +++++------
> > fs/cifs/sess.c | 4 +++-
> > fs/cifs/smb2pdu.c | 20 ++++++++------------
> > 3 files changed, 16 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index a58dc77..c1c2006 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -375,16 +375,15 @@ CIFSSMBNegotiate(const unsigned int xid, struct cifs_ses *ses)
> > int rc = 0;
> > int bytes_returned;
> > int i;
> > - struct TCP_Server_Info *server;
> > + struct TCP_Server_Info *server = ses->server;
> > u16 count;
> > unsigned int secFlags;
> >
> > - if (ses->server)
> > - server = ses->server;
> > - else {
> > - rc = -EIO;
> > - return rc;
> > + if (!server) {
> > + WARN(1, "%s: server is NULL!\n", __func__);
> > + return -EIO;
> > }
> > +
> > rc = smb_init(SMB_COM_NEGOTIATE, 0, NULL /* no tcon yet */ ,
> > (void **) &pSMB, (void **) &pSMBr);
> > if (rc)
> > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> > index 838e224..e8c5dc9 100644
> > --- a/fs/cifs/sess.c
> > +++ b/fs/cifs/sess.c
> > @@ -576,8 +576,10 @@ CIFS_SessSetup(const unsigned int xid, struct cifs_ses *ses,
> > u16 blob_len;
> > char *ntlmsspblob = NULL;
> >
> > - if (ses == NULL)
> > + if (ses == NULL) {
> > + WARN(1, "%s: ses == NULL!", __func__);
> > return -EINVAL;
> > + }
> >
> > type = ses->server->secType;
> > cifs_dbg(FYI, "sess setup type %d\n", type);
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 2b95ce2..3af66aa 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -328,7 +328,7 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
> > struct kvec iov[1];
> > int rc = 0;
> > int resp_buftype;
> > - struct TCP_Server_Info *server;
> > + struct TCP_Server_Info *server = ses->server;
> > unsigned int sec_flags;
> > u16 temp = 0;
> > int blob_offset, blob_length;
> > @@ -337,11 +337,9 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
> >
> > cifs_dbg(FYI, "Negotiate protocol\n");
> >
> > - if (ses->server)
> > - server = ses->server;
> > - else {
> > - rc = -EIO;
> > - return rc;
> > + if (!server) {
> > + WARN(1, "%s: server is NULL!\n", __func__);
> > + return -EIO;
> > }
> >
> > rc = small_smb2_init(SMB2_NEGOTIATE, NULL, (void **) &req);
> > @@ -480,7 +478,7 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
> > int rc = 0;
> > int resp_buftype;
> > __le32 phase = NtLmNegotiate; /* NTLMSSP, if needed, is multistage */
> > - struct TCP_Server_Info *server;
> > + struct TCP_Server_Info *server = ses->server;
> > unsigned int sec_flags;
> > u8 temp = 0;
> > u16 blob_length = 0;
> > @@ -490,11 +488,9 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
> >
> > cifs_dbg(FYI, "Session Setup\n");
> >
> > - if (ses->server)
> > - server = ses->server;
> > - else {
> > - rc = -EIO;
> > - return rc;
> > + if (!server) {
> > + WARN(1, "%s: server is NULL!\n", __func__);
> > + return -EIO;
> > }
> >
> > /*
> > --
> > 1.8.1.4
> >
> > --
> > 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
>
> Agreed. Also we have such checks several other places - may be it's
> good to make them raising a warning too?
>
> Reviewed-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>
Yes, I think so. I just did these as I ran across them, but adding
WARN() calls to similar spots makes sense to me.
--
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
