From: Dan Carpenter <dan.carpenter@oracle.com>
To: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: linux-scsi@vger.kernel.org, gregkh@linuxfoundation.org,
jasowang@redhat.com, ohering@suse.com, jbottomley@parallels.com,
linux-kernel@vger.kernel.org, hch@infradead.org,
apw@canonical.com, devel@linuxdriverproject.org,
"keith.mange@microsoft.com" <keith.mange@microsoft.com>
Subject: Re: [PATCH 3/6] hv:scsi:Untangle the storage protocol negotiation from the vmbus protocol negotiation.
Date: Mon, 1 Jun 2015 13:42:37 +0300 [thread overview]
Message-ID: <20150601104237.GU28762@mwanda> (raw)
In-Reply-To: <1432931359-14473-3-git-send-email-kys@microsoft.com>
On Fri, May 29, 2015 at 01:29:16PM -0700, K. Y. Srinivasan wrote:
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> + if (vstor_packet->status != 0) {
> + ret = -EINVAL;
> goto cleanup;
> + }
There is not actually any cleanup, goto cleanup is just a do-nothing
goto.
In the original code, we returned success here. That always looked like
a "forgot to set the error code" bug to me, but do-nothing labels always
introduce ambiguous looking "forgot to set the error code" bugs so I can
never be positive.
Could you take a look at the other "goto cleanup;" places in this
function and maybe add a comment, change it to something more clear like
"return 0;" or fix the error code?
regards,
dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, ohering@suse.com,
jbottomley@parallels.com, hch@infradead.org,
linux-scsi@vger.kernel.org, apw@canonical.com,
vkuznets@redhat.com, jasowang@redhat.com,
"keith.mange@microsoft.com" <keith.mange@microsoft.com>
Subject: Re: [PATCH 3/6] hv:scsi:Untangle the storage protocol negotiation from the vmbus protocol negotiation.
Date: Mon, 1 Jun 2015 13:42:37 +0300 [thread overview]
Message-ID: <20150601104237.GU28762@mwanda> (raw)
In-Reply-To: <1432931359-14473-3-git-send-email-kys@microsoft.com>
On Fri, May 29, 2015 at 01:29:16PM -0700, K. Y. Srinivasan wrote:
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> + if (vstor_packet->status != 0) {
> + ret = -EINVAL;
> goto cleanup;
> + }
There is not actually any cleanup, goto cleanup is just a do-nothing
goto.
In the original code, we returned success here. That always looked like
a "forgot to set the error code" bug to me, but do-nothing labels always
introduce ambiguous looking "forgot to set the error code" bugs so I can
never be positive.
Could you take a look at the other "goto cleanup;" places in this
function and maybe add a comment, change it to something more clear like
"return 0;" or fix the error code?
regards,
dan carpenter
next prev parent reply other threads:[~2015-06-01 10:42 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-29 20:28 [PATCH 0/6] scsi: storvsc: Some miscellaneous cleanup K. Y. Srinivasan
2015-05-29 20:29 ` [PATCH 1/6] scsi: storvsc: Rather than look for sets of specific protocol versions, make decisions based on ranges K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:29 ` [PATCH 2/6] scsi: storvsc: Use a single value to track protocol versions K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:34 ` Long Li
2015-05-29 20:29 ` [PATCH 3/6] hv:scsi:Untangle the storage protocol negotiation from the vmbus protocol negotiation K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:37 ` Long Li
2015-06-01 10:42 ` Dan Carpenter [this message]
2015-06-01 10:42 ` Dan Carpenter
2015-06-01 19:17 ` KY Srinivasan
2015-05-29 20:29 ` [PATCH 4/6] scsi: storvsc: use correct defaults for values determined by " K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:38 ` Long Li
2015-05-29 20:29 ` [PATCH 5/6] scsi: storvsc: use storage protocol version to determine storage capabilities K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:38 ` Long Li
2015-05-29 20:29 ` [PATCH 6/6] scsi: storvsc: Allow write_same when host is windows 10 K. Y. Srinivasan
2015-05-29 20:29 ` K. Y. Srinivasan
2015-05-29 20:39 ` Long Li
2015-05-29 20:33 ` [PATCH 1/6] scsi: storvsc: Rather than look for sets of specific protocol versions, make decisions based on ranges Long Li
2015-06-01 10:56 ` Dan Carpenter
2015-06-01 10:56 ` Dan Carpenter
2015-06-01 19:16 ` KY Srinivasan
2015-06-01 19:16 ` KY Srinivasan
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=20150601104237.GU28762@mwanda \
--to=dan.carpenter@oracle.com \
--cc=apw@canonical.com \
--cc=devel@linuxdriverproject.org \
--cc=gregkh@linuxfoundation.org \
--cc=hch@infradead.org \
--cc=jasowang@redhat.com \
--cc=jbottomley@parallels.com \
--cc=keith.mange@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=ohering@suse.com \
/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.