From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754627AbbLKKlg (ORCPT ); Fri, 11 Dec 2015 05:41:36 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:17878 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751271AbbLKKle (ORCPT ); Fri, 11 Dec 2015 05:41:34 -0500 Date: Fri, 11 Dec 2015 13:40:47 +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, martin.petersen@oracle.com Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init() Message-ID: <20151211104047.GG5284@mwanda> References: <1449792829-15406-1-git-send-email-kys@microsoft.com> <1449792860-15447-1-git-send-email-kys@microsoft.com> <1449792860-15447-3-git-send-email-kys@microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449792860-15447-3-git-send-email-kys@microsoft.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote: > @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc) > VM_PKT_DATA_INBAND, > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > if (ret != 0) > - goto cleanup; > + goto done; > > t = wait_for_completion_timeout(&request->wait_event, 5*HZ); > if (t == 0) { > ret = -ETIMEDOUT; > - goto cleanup; > + goto done; > } > > + if (!status_check) > + goto done; See? This goto looks exactly the same as the earlier buggy goto but it's actually correct. Meanwhile if you just used an explicit "return 0;" then it would be easy to understand. I rant about this all the time but it's because it's bad deliberately. It's normal to have bugs, but this deliberate stuff really I can't understand it... > + > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO || > vstor_packet->status != 0) { > ret = -EINVAL; > - goto cleanup; > + goto done; > } > > +done: > + return ret; > +} > + > +static int storvsc_channel_init(struct hv_device *device, bool is_fc) > +{ > + struct storvsc_device *stor_device; > + struct storvsc_cmd_request *request; > + struct vstor_packet *vstor_packet; > + int ret, i; > + int max_chns; > + bool process_sub_channels = false; > + > + stor_device = get_out_stor_device(device); > + if (!stor_device) > + return -ENODEV; > + > + request = &stor_device->init_request; > + vstor_packet = &request->vstor_packet; > + > + /* > + * Now, initiate the vsc/vsp initialization protocol on the open > + * channel > + */ > + memset(request, 0, sizeof(struct storvsc_cmd_request)); > + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION; > + ret = storvsc_execute_vstor_op(device, request, true); > + if (ret) > + goto cleanup; 10 lines earlier there is an explicit "return -ENODEV" so it's not as if writing explicit returns will kill you. regards, dan carpenter From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init() Date: Fri, 11 Dec 2015 13:40:47 +0300 Message-ID: <20151211104047.GG5284@mwanda> References: <1449792829-15406-1-git-send-email-kys@microsoft.com> <1449792860-15447-1-git-send-email-kys@microsoft.com> <1449792860-15447-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: <1449792860-15447-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: martin.petersen@oracle.com, 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 List-Id: linux-scsi@vger.kernel.org On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote: > @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc) > VM_PKT_DATA_INBAND, > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > if (ret != 0) > - goto cleanup; > + goto done; > > t = wait_for_completion_timeout(&request->wait_event, 5*HZ); > if (t == 0) { > ret = -ETIMEDOUT; > - goto cleanup; > + goto done; > } > > + if (!status_check) > + goto done; See? This goto looks exactly the same as the earlier buggy goto but it's actually correct. Meanwhile if you just used an explicit "return 0;" then it would be easy to understand. I rant about this all the time but it's because it's bad deliberately. It's normal to have bugs, but this deliberate stuff really I can't understand it... > + > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO || > vstor_packet->status != 0) { > ret = -EINVAL; > - goto cleanup; > + goto done; > } > > +done: > + return ret; > +} > + > +static int storvsc_channel_init(struct hv_device *device, bool is_fc) > +{ > + struct storvsc_device *stor_device; > + struct storvsc_cmd_request *request; > + struct vstor_packet *vstor_packet; > + int ret, i; > + int max_chns; > + bool process_sub_channels = false; > + > + stor_device = get_out_stor_device(device); > + if (!stor_device) > + return -ENODEV; > + > + request = &stor_device->init_request; > + vstor_packet = &request->vstor_packet; > + > + /* > + * Now, initiate the vsc/vsp initialization protocol on the open > + * channel > + */ > + memset(request, 0, sizeof(struct storvsc_cmd_request)); > + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION; > + ret = storvsc_execute_vstor_op(device, request, true); > + if (ret) > + goto cleanup; 10 lines earlier there is an explicit "return -ENODEV" so it's not as if writing explicit returns will kill you. regards, dan carpenter