From: sagi@grimberg.me (Sagi Grimberg)
Subject: [PATCH] nvmet-rdma: Invoke fatal error on error completion
Date: Sun, 26 Jun 2016 18:57:14 +0300 [thread overview]
Message-ID: <576FFB5A.7070709@grimberg.me> (raw)
In-Reply-To: <20160624071101.GC4252@infradead.org>
>> +static void nvmet_rdma_error_comp(struct nvmet_rdma_queue *queue)
>> +{
>> + if (queue->nvme_sq.ctrl)
>> + nvmet_ctrl_fatal_error(queue->nvme_sq.ctrl);
>> + else
>> + /*
>> + * we didn't setup the controller yet in case
>> + * of admin connect error, just disconnect and
>> + * cleanup the queue
>> + */
>> + nvmet_rdma_queue_disconnect(queue);
>> +}
>
> With such a long comment I'd prefer to have curly braces just to make
> the else visually more obvious
Sure.
>> +
>> + if (unlikely(wc->status != IB_WC_SUCCESS &&
>> + wc->status != IB_WC_WR_FLUSH_ERR)) {
>
> Indenting the second line of a condition by a single tab is always wrong,
> either indent it with two tabs, or so that it aligns with first line.
> The second is probably nicer here:
>
> if (unlikely(wc->status != IB_WC_SUCCESS &&
> wc->status != IB_WC_WR_FLUSH_ERR)) {
OK, I'll do that.
> Given how many !success not !flush_err conditionals we have in various
> drivers I wonder if we should have a helper in the RDMA core, though.
They don't always come together. But I can take a look at it. Given that
it might bring some slight logic shifts, let's do it incrementally.
WARNING: multiple messages have this Message-ID (diff)
From: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
To: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] nvmet-rdma: Invoke fatal error on error completion
Date: Sun, 26 Jun 2016 18:57:14 +0300 [thread overview]
Message-ID: <576FFB5A.7070709@grimberg.me> (raw)
In-Reply-To: <20160624071101.GC4252-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
>> +static void nvmet_rdma_error_comp(struct nvmet_rdma_queue *queue)
>> +{
>> + if (queue->nvme_sq.ctrl)
>> + nvmet_ctrl_fatal_error(queue->nvme_sq.ctrl);
>> + else
>> + /*
>> + * we didn't setup the controller yet in case
>> + * of admin connect error, just disconnect and
>> + * cleanup the queue
>> + */
>> + nvmet_rdma_queue_disconnect(queue);
>> +}
>
> With such a long comment I'd prefer to have curly braces just to make
> the else visually more obvious
Sure.
>> +
>> + if (unlikely(wc->status != IB_WC_SUCCESS &&
>> + wc->status != IB_WC_WR_FLUSH_ERR)) {
>
> Indenting the second line of a condition by a single tab is always wrong,
> either indent it with two tabs, or so that it aligns with first line.
> The second is probably nicer here:
>
> if (unlikely(wc->status != IB_WC_SUCCESS &&
> wc->status != IB_WC_WR_FLUSH_ERR)) {
OK, I'll do that.
> Given how many !success not !flush_err conditionals we have in various
> drivers I wonder if we should have a helper in the RDMA core, though.
They don't always come together. But I can take a look at it. Given that
it might bring some slight logic shifts, let's do it incrementally.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2016-06-26 15:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-23 17:01 [PATCH] nvmet-rdma: Invoke fatal error on error completion Sagi Grimberg
2016-06-23 17:01 ` Sagi Grimberg
2016-06-24 7:11 ` Christoph Hellwig
2016-06-24 7:11 ` Christoph Hellwig
2016-06-26 15:57 ` Sagi Grimberg [this message]
2016-06-26 15:57 ` Sagi Grimberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=576FFB5A.7070709@grimberg.me \
--to=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.