> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Friday, December 11, 2015 2:41 AM > To: KY Srinivasan <kys@xxxxxxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx; > jbottomley@xxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx; > martin.petersen@xxxxxxxxxx > Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in > storvsc_channel_init() > > 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. Thanks Dan; I will cleanup the code and resend. Regards, K. Y _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
