From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter 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 Message-ID: <20150601104237.GU28762@mwanda> References: <1432931336-14434-1-git-send-email-kys@microsoft.com> <1432931359-14473-1-git-send-email-kys@microsoft.com> <1432931359-14473-3-git-send-email-kys@microsoft.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1432931359-14473-3-git-send-email-kys@microsoft.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: "K. Y. Srinivasan" 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" List-Id: linux-scsi@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751735AbbFAKnh (ORCPT ); Mon, 1 Jun 2015 06:43:37 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:41051 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750974AbbFAKn3 (ORCPT ); Mon, 1 Jun 2015 06:43:29 -0400 Date: Mon, 1 Jun 2015 13:42:37 +0300 From: Dan Carpenter To: "K. Y. Srinivasan" 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" Subject: Re: [PATCH 3/6] hv:scsi:Untangle the storage protocol negotiation from the vmbus protocol negotiation. Message-ID: <20150601104237.GU28762@mwanda> References: <1432931336-14434-1-git-send-email-kys@microsoft.com> <1432931359-14473-1-git-send-email-kys@microsoft.com> <1432931359-14473-3-git-send-email-kys@microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432931359-14473-3-git-send-email-kys@microsoft.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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