Sure, that takes care of the static checker warning. But then I had
all sorts of style nits to whinge-bucket about which you didn't ask
for...
On Thu, Apr 06, 2017 at 07:24:07PM +0100, Sachin Prabhu wrote:
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9ae695a..131663b 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -904,10 +904,19 @@ cifs_demultiplex_thread(void *p)
>
> server->lstrp = jiffies;
> if (mid_entry != NULL) {
> + if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) &&
> + mid_entry->mid_state == MID_RESPONSE_RECEIVED &&
> + (server->ops->handle_cancelled_mid))
Extra parens are not required. Please align it like this:
if ((mid_entry->mid_flags & MID_WAIT_CANCELLED) &&
mid_entry->mid_state == MID_RESPONSE_RECEIVED &&
server->ops->handle_cancelled_mid)
> + server->ops->handle_cancelled_mid(
> + mid_entry->resp_buf,
> + server);
> +
> if (!mid_entry->multiRsp || mid_entry->multiEnd)
> mid_entry->callback(mid_entry);
> - } else if (!server->ops->is_oplock_break ||
> - !server->ops->is_oplock_break(buf, server)) {
> + } else if (server->ops->is_oplock_break &&
> + server->ops->is_oplock_break(buf, server)) {
> + cifs_dbg(FYI, "Received oplock break\n");
> + } else {
> cifs_dbg(VFS, "No task to wake, unknown frame received! NumMids %d\n",
> atomic_read(&midCount));
> cifs_dump_mem("Received Data is: ", buf,
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index fd516ea..f50e3ef 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -659,3 +659,51 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server)
> cifs_dbg(FYI, "Can not process oplock break for non-existent connection\n");
> return false;
> }
> +
> +void
> +smb2_cancelled_close_fid(struct work_struct *work)
> +{
> + struct close_cancelled_open *cancelled = container_of(work,
> + struct close_cancelled_open, work);
> +
> + cifs_dbg(VFS, "Close unmatched open\n");
> +
> + SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid,
> + cancelled->fid.volatile_fid);
Don't right align it. Do it like this:
SMB2_close(0, cancelled->tcon, cancelled->fid.persistent_fid,
cancelled->fid.volatile_fid);
> + cifs_put_tcon(cancelled->tcon);
> + kfree(cancelled);
> +}
> +
> +int
> +smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server)
> +{
> + struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer);
> + struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer;
> + struct cifs_tcon *tcon;
> + struct close_cancelled_open *cancelled;
> +
> + if ((sync_hdr->Command != SMB2_CREATE) ||
> + (sync_hdr->Status != STATUS_SUCCESS))
No need for extra parens. Align it like this:
if (sync_hdr->Command != SMB2_CREATE ||
sync_hdr->Status != STATUS_SUCCESS)
return 0;
> + return 0;
> +
> + cancelled = (struct close_cancelled_open *)
> + kzalloc(sizeof(struct close_cancelled_open),
> + GFP_KERNEL);
No need to cast kzalloc. Use sizeof(*cancelled).
cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
> + if (!cancelled)
> + return -ENOMEM;
> +
> + tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId,
> + sync_hdr->TreeId);
> + if (!tcon) {
> + kfree(cancelled);
> + return -ENOENT;
> + }
> +
> + cancelled->fid.persistent_fid = rsp->PersistentFileId;
> + cancelled->fid.volatile_fid = rsp->VolatileFileId;
> + cancelled->tcon = tcon;
> + INIT_WORK(&cancelled->work, smb2_cancelled_close_fid);
> + queue_work(cifsiod_wq, &cancelled->work);
> +
> + return 0;
> +}
[ snip ]
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 7c3bb1b..e3c8a9c 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -115,23 +115,72 @@ smb3_crypto_shash_allocate(struct TCP_Server_Info *server)
> return 0;
> }
>
> -struct cifs_ses *
> -smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id)
> +static struct cifs_ses *
> +_smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id)
Could you name this "locked" or something and get rid of the _
underscore prefix?
> {
> struct cifs_ses *ses;
>
> - spin_lock(&cifs_tcp_ses_lock);
> list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
> if (ses->Suid != ses_id)
> continue;
> - spin_unlock(&cifs_tcp_ses_lock);
> return ses;
> }
> +
> + return NULL;
> +}
> +
> +struct cifs_ses *
> +smb2_find_smb_ses(struct TCP_Server_Info *server, __u64 ses_id)
> +{
> + struct cifs_ses *ses;
> +
> + spin_lock(&cifs_tcp_ses_lock);
> + ses = _smb2_find_smb_ses(server, ses_id);
> spin_unlock(&cifs_tcp_ses_lock);
>
> + return ses;
> +}
> +
> +static struct cifs_tcon *
> +_smb2_find_smb_sess_tcon(struct cifs_ses *ses, __u32 tid)
Just leave out the _ underscore prefix. This is a static function so
there aren't that many users and they presumably know the locking rules.
For smb2_find_smb_ses we have a locked and unlocked version of the
function so it makes sense to put that in the name but here there is
no need I think.
> +{
> + struct list_head *tmp;
> + struct cifs_tcon *tcon;
> +
> + list_for_each(tmp, &ses->tcon_list) {
> + tcon = list_entry(tmp, struct cifs_tcon, tcon_list);
list_for_each_entry()?
> + if (tcon->tid != tid)
> + continue;
> + ++tcon->tc_count;
> + return tcon;
> + }
> +
> return NULL;
> }
>
> +/*
> + * Obtain tcon corresponding to the tid in the given
> + * cifs_ses
> + */
> +
> +struct cifs_tcon *
> +smb2_find_smb_tcon(struct TCP_Server_Info *server, __u64 ses_id, __u32 tid)
> +{
> + struct cifs_ses *ses;
> + struct cifs_tcon *tcon;
> +
> + spin_lock(&cifs_tcp_ses_lock);
> + ses = _smb2_find_smb_ses(server, ses_id);
> + if (!ses) {
> + spin_unlock(&cifs_tcp_ses_lock);
> + return NULL;
> + }
> + tcon = _smb2_find_smb_sess_tcon(ses, tid);
> + spin_unlock(&cifs_tcp_ses_lock);
> +
> + return tcon;
> +}
> +
> int
> smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
> {
> @@ -511,6 +560,7 @@ smb2_mid_entry_alloc(const struct smb2_sync_hdr *shdr,
>
> atomic_inc(&midCount);
> temp->mid_state = MID_REQUEST_ALLOCATED;
> + temp->mid_flags = 0;
No need. We already memset() this to zero.
> return temp;
> }
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 526f053..d82a4ae 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -752,9 +752,12 @@ cifs_send_recv(const unsigned int xid, struct cifs_ses *ses,
>
> rc = wait_for_response(ses->server, midQ);
> if (rc != 0) {
> + cifs_dbg(FYI, "Cancelling wait for mid %lu\n",
> + (unsigned long)midQ->mid);
->mid is u64. Just do this:
cifs_dbg(FYI, "Cancelling wait for mid %llu\n", midQ->mid);
> send_cancel(ses->server, rqst, midQ);
> spin_lock(&GlobalMid_Lock);
> if (midQ->mid_state == MID_REQUEST_SUBMITTED) {
> + midQ->mid_flags |= MID_WAIT_CANCELLED;
> midQ->callback = DeleteMidQEntry;
> spin_unlock(&GlobalMid_Lock);
> add_credits(ses->server, 1, optype);
regards,
dan carpenter
--
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
